Fix peer storage size cap to account for encryption overhead#4746
Fix peer storage size cap to account for encryption overhead#4746shivv23 wants to merge 3 commits into
Conversation
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 lightningdevkit#1's 65531-byte limit and BOLT lightningdevkit#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.
|
I've assigned @joostjager as a reviewer! |
| counterparty_fulfilled_htlcs: new_hash_map(), | ||
|
|
||
| current_counterparty_commitment_number: 1 << 48, | ||
| current_counterparty_commitment_number: (1 << 48) - 1, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
|
The current state matches my memory exactly. The fix is correctly placed in No new issues found. My previously-flagged concern (the fix being in the production constructor) has been resolved in commit Summary of review:
Minor (non-blocking, previously noted):
No new issues found. |
…er 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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4746 +/- ##
==========================================
+ Coverage 84.51% 86.95% +2.43%
==========================================
Files 137 161 +24
Lines 77446 111636 +34190
Branches 77446 111636 +34190
==========================================
+ Hits 65456 97071 +31615
- Misses 9949 12060 +2111
- Partials 2041 2505 +464
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /// in `peer_channel_encryptor`). | ||
| #[test] | ||
| #[cfg(peer_storage)] | ||
| fn test_peer_storage_blob_within_size_limit() { |
There was a problem hiding this comment.
I'd make sure this test tests right on the boundary. Currently it also passes if I manually set overhead to 0 again.
There was a problem hiding this comment.
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.
| // 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; |
There was a problem hiding this comment.
Seems ok, even though in reality commitment numbers start high and go down. So 0 isn't really the default for a fresh channel. But it is consistent with the dummy holder commitment.
There was a problem hiding this comment.
Agreed that 0 isn't the real-world default — commitment numbers start high and decrement. It is consistent with the other dummy values (e.g., the dummy holder commitment uses commitment number 0), and being a valid 48-bit value that serializes without panicking is the main objective here. Happy to change it to INITIAL_COMMITMENT_NUMBER (= (1 << 48) - 1) if you'd prefer greater realism.
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 lightningdevkit#4746.
4 participants
Description
Fixes #4698:
send_peer_storage()does not account for encryption/wire overhead when capping the peer storage blob size, causing it to emitPeerStoragemessages that can exceed BOLT #1's 65531-byte limit and panic BOLT #8's transport layer.Changes
1.
lightning/src/chain/chainmonitor.rs—send_peer_storage()size capRoot cause: The function caps the sum of monitors'
serialized_length()atMAX_PEER_STORAGE_SIZE(65531), but the message that goes on the wire is ~50 bytes larger due to:Vec<PeerStorageMonitorHolder>CollectionLength prefix added byencode()Fix: Introduced
PEER_STORAGE_OVERHEAD = 2 + 16 + 32and changed the size check from> MAX_PEER_STORAGE_SIZEto> MAX_PEER_STORAGE_SIZE - PEER_STORAGE_OVERHEAD.The constant is deliberately decomposed (2, 16, 32) rather than hardcoded as 50, making the overhead sources auditable.
2.
lightning/src/chain/channelmonitor.rs—dummy_monitor()fixdummy_monitor()creates aChannelMonitorviaChannelMonitor::new(), which setscurrent_counterparty_commitment_number = 1 << 48as a sentinel (INITIAL_COMMITMENT_NUMBER + 1). This 49-bit value overflowsbe48_to_array()on serialization, causing a panic in tests that serialize dummy monitors (e.g., the peer storage regression test).Fix: After construction, override the field to
0— a valid 48-bit value. The production constructor's sentinel is preserved unchanged.3. Regression test
Added
test_peer_storage_blob_within_size_limit(#[cfg(peer_storage)]) which:send_peer_storage()and assertsmsg.data.len() <= MAX_PEER_STORAGE_SIZEmsg.data.len() > MAX_PEER_STORAGE_SIZE - 500to prove the boundary was reached