Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 77 additions & 1 deletion lightning/src/chain/chainmonitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -986,6 +986,12 @@ where
let random_bytes = self._entropy_source.get_secure_random_bytes();

const MAX_PEER_STORAGE_SIZE: usize = 65531;
// The overhead between the sum of `serialized_length()` for the included monitors and
// the final `PeerStorage{}.data` wire size. This accounts for:
// - 2 bytes: CollectionLength vec length prefix from `encode()`
// - 16 bytes: ChaCha20Poly1305 AEAD tag
// - 32 bytes: random_bytes nonce seed appended during encryption
const PEER_STORAGE_OVERHEAD: usize = 2 + 16 + 32;
const USIZE_LEN: usize = core::mem::size_of::<usize>();
let mut random_bytes_cycle_iter = random_bytes.iter().cycle();

Expand Down Expand Up @@ -1042,7 +1048,7 @@ where

let serialized_length = peer_storage_monitor.serialized_length();

if current_size + serialized_length > MAX_PEER_STORAGE_SIZE {
if current_size + serialized_length > MAX_PEER_STORAGE_SIZE - PEER_STORAGE_OVERHEAD {
continue;
} else {
current_size += serialized_length;
Expand Down Expand Up @@ -2215,4 +2221,74 @@ mod tests {
assert_eq!(chain_monitor_a.pending_operation_count(), 0);
assert_eq!(chain_monitor_b.pending_operation_count(), 0);
}

/// Regression test for peer-storage blob size accounting.
///
/// `send_peer_storage` caps the *plaintext* payload at `MAX_PEER_STORAGE_SIZE` (65531), but the
/// message put on the wire is the *encrypted* blob, which is ~50 bytes larger: a 2-byte
/// CollectionLength vec prefix, a 16-byte ChaCha20Poly1305 AEAD tag, and a 32-byte nonce seed.
/// Without accounting for this overhead, the emitted `PeerStorage` message can exceed BOLT #1's
/// 65531-byte limit (and even BOLT #8's 65535-byte transport limit, which would panic the node
/// in `peer_channel_encryptor`).
#[test]
#[cfg(peer_storage)]
fn test_peer_storage_blob_within_size_limit() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd make sure this test tests right on the boundary. Currently it also passes if I manually set overhead to 0 again.

@shivv23 shivv23 Jun 25, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With 200 monitors the total serialized_length() was insufficient to reach the cap, so the test passed regardless of the overhead value. I've increased num_monitors to 600 and added a lower-bound assertion (msg.data.len() > MAX_PEER_STORAGE_SIZE - 500) to verify the boundary is actually exercised. Fixed in ff9c956.

const MAX_PEER_STORAGE_SIZE: usize = 65531;

let broadcaster = TestBroadcaster::new(Network::Testnet);
let fee_est = TestFeeEstimator::new(253);
let logger = TestLogger::new();
let persister = TestPersister::new();
let chain_source = TestChainSource::new(Network::Testnet);
let keys = TestKeysInterface::new(&[0; 32], Network::Testnet);

let chain_monitor = ChainMonitor::new(
Some(&chain_source),
&broadcaster,
&logger,
&fee_est,
&persister,
&keys,
keys.get_peer_storage_key(),
false,
);

// Create enough monitors to stress the peer storage size limit. Each dummy monitor
// is ~500 bytes, so ~200 monitors is enough to exceed 65531.
let num_monitors = 200;
let mut counterparty_id = None;
for i in 0..num_monitors {
let mut chan_id = [0u8; 32];
chan_id[0] = (i / 256) as u8;
chan_id[1] = (i % 256) as u8;
let chan = ChannelId::from_bytes(chan_id);
let monitor = crate::chain::channelmonitor::dummy_monitor(chan, |keys| {
TestChannelSigner::new(DynSigner::new(keys))
});
if i == 0 {
counterparty_id = Some(monitor.get_counterparty_node_id());
}
assert!(Watch::watch_channel(&chain_monitor, chan, monitor).is_ok());
}

// Trigger send_peer_storage for the first monitor's counterparty. This internally
// iterates over monitors and should cap the total encrypted size.
let node_id = counterparty_id.unwrap();
chain_monitor.send_peer_storage(node_id);

let msg_events = chain_monitor.get_and_clear_pending_msg_events();
let mut saw_peer_storage = false;
for ev in msg_events {
if let MessageSendEvent::SendPeerStorage { msg, .. } = ev {
saw_peer_storage = true;
assert!(
msg.data.len() <= MAX_PEER_STORAGE_SIZE,
"peer_storage blob length {} exceeds BOLT #1 limit of {}",
msg.data.len(),
MAX_PEER_STORAGE_SIZE,
);
}
}
assert!(saw_peer_storage, "expected a SendPeerStorage message");
}
}
2 changes: 1 addition & 1 deletion lightning/src/chain/channelmonitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1952,7 +1952,7 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
counterparty_hash_commitment_number: new_hash_map(),
counterparty_fulfilled_htlcs: new_hash_map(),

current_counterparty_commitment_number: 1 << 48,
current_counterparty_commitment_number: (1 << 48) - 1,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is in the production ChannelMonitor::new() constructor (line 1872), not in dummy_monitor() as the PR description claims. dummy_monitor() (line 6973) merely calls ChannelMonitor::new(), so this alters the initial value for every monitor created in production.

The original 1 << 48 is a deliberate sentinel (INITIAL_COMMITMENT_NUMBER + 1) meaning "the initial counterparty commitment has not been provided yet", distinct from INITIAL_COMMITMENT_NUMBER = (1 << 48) - 1 which means "at the initial counterparty commitment". This distinction is observable in is_closed_without_updates() (line 4435, == INITIAL_COMMITMENT_NUMBER).

In normal operation the value is always overwritten by provide_initial_counterparty_commitment_tx() before the monitor is ever serialized (see the invariant documented at channel.rs:6531-6535), so 1 << 48 is never actually written via be48_to_array in production — the panic only occurs because dummy_monitor() skips provide_initial_counterparty_commitment_tx().

Recommend fixing this in dummy_monitor() (e.g. by providing an initial counterparty commitment, or setting the field there) rather than weakening the documented sentinel/invariant in the production constructor. At minimum the PR description should be corrected, since this is a behavioral change to production code, not a test helper.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ldk-claude-review-bot thanks for the thorough review. Point by point:

1. current_counterparty_commitment_number sentinel in production constructor — Correct, this should not have been changed in ChannelMonitor::new(). Reverted back to 1 << 48 and moved the fix into dummy_monitor() instead, where it sets the field to 0 after construction. The production sentinel is now preserved.

2. Tightness of the cap — The math is intentional: data.len() = sum(serialized_length) + 2 (CollectionLength) + 16 (tag) + 32 (nonce) = sum + 50. With the cap at 65531 - 50 = 65481, the wire message is exactly 2 + 2 + 65531 = 65535 — at, not over, LN_MAX_MSG_LEN.

3. #[cfg(peer_storage)] gating — This is required because send_peer_storage() itself is #[cfg(peer_storage)], so the test must be gated identically to compile. It runs in CI when RUSTFLAGS="--cfg peer_storage" is set.

current_holder_commitment_number,

payment_preimages: new_hash_map(),
Expand Down
Loading