Repository navigation
Conversation
…devkit#1138) `EventQueueSerWrapper` and `PeerStoreSerWrapper` wrote their collection length with `(len as u16)` over unbounded collections. Once a queue held more than 65535 entries the persisted count wrapped (65_536 events wrote a count of 0), so on restart the reader restored a truncated queue and application event notifications were silently lost. Use `lightning::util::ser::CollectionLength` for the length prefix in both the serialization and deserialization wrappers, matching how LDK encodes its own collections. This preserves the existing two-byte encoding for counts below 65535 and extends to larger counts. Add round-trip tests at 65534/65535/65536/65540 entries and a legacy two-byte length-prefix decode test to lock in backward compatibility.
|
I've assigned @tnull as a reviewer! |
Collaborator
|
No, we already have a PR up for this. The issue is borderline wontfix anyways, so we def. don't need two competing PRs for this. Please next time first ask if an issue is up for the taking. Closing as duplicate. |
Author
|
Fair enough — #1145 was up well before mine and covers the same fix. I should have checked for an existing PR first; will do next time. Thanks for the pointer, and sorry for the noise. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1138 — completely, at both truncation sites.
EventQueueSerWrapper::write(src/event.rs) andPeerStoreSerWrapper::write(src/peer_store.rs:151) both write their collection length as a plainu16over an unbounded collection (VecDeque<Event>/ the peerHashMap). Above 65,535 entries the count wraps: a 65,536-event queue serializes to length 0, silently dropping every event on the next restart.This switches both wrappers to
lightning::util::ser::CollectionLength— a bareu16below 65,535 (byte-identical to the current wire format, so existing persisted state still decodes) and0xffff + u64above. No migration needed, and no truncation at any size.Why both sites
#1145 applies the same
CollectionLengthfix tosrc/event.rsonly. This PR fixesevent.rsand the identical defect atpeer_store.rs:151, so #1138 is resolved in one change rather than leaving the sibling bug behind. Happy to coordinate with #1145 — e.g. if the event-queue change lands there first, I'll rebase this down to just thepeer_storedelta — whatever keeps it cleanest for you.Tests
event_queue_round_trips_across_u16_boundary— 65,534 / 65,535 / 65,536 / 65,540 events; fails before (65,536 → length 0), passes after.event_queue_decodes_legacy_u16_length_prefix— the new reader decodes the pre-existing bare-u16prefix, and the writer still emits a 2-byte prefix for small queues (backward-compatible).event/peer_storetests pass;cargo fmt --allclean; no regressions. (A >65,535-entrypeer_storetest is impractical — it would need that many distinct on-curve secp pubkeys — but thepeer_storepath uses the exactCollectionLengthidiom the event-queue boundary tests exercise.)