Enforce that node_ids are sorted in channel_announcements#4611
Enforce that node_ids are sorted in channel_announcements#4611TheBlueMatt wants to merge 1 commit into
Conversation
|
🎉 This PR is now ready for review! |
Review SummaryInline comments posted
Previously flagged (from prior review, not repeated)
No new bugs foundThe core validation logic ( |
|
Drafting until claude fixes tests. |
We already enforced that nodes can't have a chanel with themselves, but the spec was updated to require strict ordering at lightning/bolts#1333 so we enforce this as well. Test fixes by claude.
TheBlueMatt
force-pushed
the
2026-05-sorted-nodes
branch
from
798f08f to
271ac91
Compare
| ) -> Result<(), LightningError> { | ||
| let channels = self.channels.read().unwrap(); | ||
|
|
||
| if msg.node_id_1 >= msg.node_id_2 { |
There was a problem hiding this comment.
Nit: The get_signed_channel_announcement test helper (defined at line 2838) was not updated to sort node_id_1/node_id_2, unlike the analogous helpers in test_utils.rs (channel_announcement) and scoring.rs (add_channel) which were both updated in this PR.
All callers implicitly rely on the key values ([42; 32]/[41; 32] and [99; 32]/[98; 32] in utxo.rs) happening to produce already-sorted node_ids. If any caller ever uses keys that produce unsorted node_ids, they'll get a cryptic "node_ids in channel_announcements must be sorted" error instead of the expected test behavior.
Consider adding sorting logic to get_signed_channel_announcement as well (while preserving the ability to create intentionally invalid announcements via the f closure for negative tests like the "channel with itself" test at line 3131).
There was a problem hiding this comment.
Let's address this comment ?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4611 +/- ##
==========================================
+ Coverage 86.11% 86.12% +0.01%
==========================================
Files 157 157
Lines 108841 108982 +141
Branches 108841 108982 +141
==========================================
+ Hits 93725 93859 +134
- Misses 12497 12504 +7
Partials 2619 2619
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tankyleo
left a comment
tankyleo
left a comment
There was a problem hiding this comment.
LGTM, just one nit from claude above
| ) -> Result<(), LightningError> { | ||
| let channels = self.channels.read().unwrap(); | ||
|
|
||
| if msg.node_id_1 >= msg.node_id_2 { |
There was a problem hiding this comment.
Let's address this comment ?
4 participants
We already enforced that nodes can't have a chanel with themselves, but the spec was updated to require strict ordering at lightning/bolts#1333 so we enforce this as well.