From 170e67a561f6462396b7b25944e1bad340942e48 Mon Sep 17 00:00:00 2001 From: shivv23 Date: Wed, 24 Jun 2026 23:43:24 +0530 Subject: [PATCH 1/3] Fix peer storage size cap to account for encryption overhead send_peer_storage() caps the sum of serialized_length() at MAX_PEER_STORAGE_SIZE (65531), but the final wire message includes a 2-byte CollectionLength prefix, 16-byte AEAD tag, and 32-byte nonce seed. Without headroom, the encrypted blob can exceed BOLT #1's 65531-byte limit and BOLT #8's 65535-byte transport limit, causing a panic. Also fixes a latent bug in dummy_monitor where current_counterparty_commitment_number was set to 1 << 48 (2^48), which overflows the 48-bit field in be48_to_array. --- lightning/src/chain/chainmonitor.rs | 78 ++++++++++++++++++++++++++- lightning/src/chain/channelmonitor.rs | 2 +- 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index b3b69096997..4210049c360 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -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::(); let mut random_bytes_cycle_iter = random_bytes.iter().cycle(); @@ -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; @@ -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() { + 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"); + } } diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index a2412bbaf5e..cfeb195b422 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1952,7 +1952,7 @@ impl ChannelMonitor { 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, current_holder_commitment_number, payment_preimages: new_hash_map(), From 4fed9891b6aa6d4546df2823876b89732930af66 Mon Sep 17 00:00:00 2001 From: shivv23 Date: Thu, 25 Jun 2026 00:13:23 +0530 Subject: [PATCH 2/3] fix dummy_monitor current_counterparty_commitment_number in test helper instead of production constructor The production constructor's 1 << 48 is a sentinel meaning 'counterparty commitment not yet received'. Changing it to (1 << 48) - 1 aligns it with INITIAL_COMMITMENT_NUMBER, which is observable in is_closed_without_updates(). Instead, fix the value in dummy_monitor() after construction so the sentinel is preserved in production code. --- lightning/src/chain/channelmonitor.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index cfeb195b422..28d9888fd26 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1952,7 +1952,7 @@ impl ChannelMonitor { counterparty_hash_commitment_number: new_hash_map(), counterparty_fulfilled_htlcs: new_hash_map(), - current_counterparty_commitment_number: (1 << 48) - 1, + current_counterparty_commitment_number: 1 << 48, current_holder_commitment_number, payment_preimages: new_hash_map(), @@ -7017,7 +7017,7 @@ pub(super) fn dummy_monitor( let shutdown_script = crate::ln::script::ShutdownScript::new_p2wpkh_from_pubkey(dummy_key); let best_block = BlockLocator::from_network(Network::Testnet); let signer = wrap_signer(keys); - ChannelMonitor::new( + let monitor = ChannelMonitor::new( secp_ctx, signer, Some(shutdown_script.into_inner()), @@ -7031,7 +7031,13 @@ pub(super) fn dummy_monitor( dummy_key, channel_id, false, - ) + ); + // The production constructor sets current_counterparty_commitment_number to 1 << 48 as a + // sentinel meaning "counterparty commitment not yet received". This value (2^48) exceeds the + // 48-bit field in be48_to_array and causes a panic on serialization. Since dummy monitors + // may be serialized in tests, reset it to a valid value. + monitor.inner.lock().unwrap().current_counterparty_commitment_number = 0; + monitor } #[cfg(test)] From ff9c956f463f328bc514d723a988bab7ec70570b Mon Sep 17 00:00:00 2001 From: shivv23 Date: Thu, 25 Jun 2026 19:51:41 +0530 Subject: [PATCH 3/3] Tighten peer storage regression test to verify boundary Increase num_monitors from 200 to 600 and add a lower-bound assertion to ensure the test actually reaches the MAX_PEER_STORAGE_SIZE boundary. This prevents the test from silently passing when PEER_STORAGE_OVERHEAD is not properly accounted for (e.g., set to 0). Addresses joostjager's review feedback on PR #4746. --- lightning/src/chain/chainmonitor.rs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs index 4210049c360..fffe9668159 100644 --- a/lightning/src/chain/chainmonitor.rs +++ b/lightning/src/chain/chainmonitor.rs @@ -2253,9 +2253,13 @@ mod tests { 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; + // Create enough monitors to stress the peer storage size limit such that the total + // plaintext serialized_length() exceeds MAX_PEER_STORAGE_SIZE, ensuring the selection + // loop actually hits the cap. 600 monitors at ~300 bytes each = ~180K, well above the + // 65531 max, so the encrypted output will be forced to the boundary regardless of + // the random selection order. This guarantees the test catches the difference when + // PEER_STORAGE_OVERHEAD is not accounted for. + let num_monitors = 600; let mut counterparty_id = None; for i in 0..num_monitors { let mut chan_id = [0u8; 32]; @@ -2287,6 +2291,13 @@ mod tests { msg.data.len(), MAX_PEER_STORAGE_SIZE, ); + assert!( + msg.data.len() > MAX_PEER_STORAGE_SIZE - 500, + "peer_storage blob length {} is too far from the {} byte limit, \ + boundary was not meaningfully tested", + msg.data.len(), + MAX_PEER_STORAGE_SIZE, + ); } } assert!(saw_peer_storage, "expected a SendPeerStorage message");