Enforce that node_ids are sorted in channel_announcements

[TheBlueMatt]

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.

[ldk-reviews-bot]

👋 Thanks for assigning @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[ldk-claude-review-bot]

No new issues found.

Review Summary

I thoroughly reviewed the entire PR diff, all modified files, and the surrounding code paths (validation flow, signature verification, UTXO checking, all three channel announcement test helpers, both update_channel test helpers, RGS processing, and the full_stack fuzzer test data).

The core change (== to >= in pre_channel_announcement_validation_check and add_channel_from_partial_announcement) is correct and consistent with the BOLT spec update. All test helpers properly sort node_ids and swap signers. The update_channel helpers in both test_utils.rs and scoring.rs correctly auto-correct channel_flags bit 0 by looking up the actual node position in the network graph, and the lock scoping is correct. The RGS binary test data changes are consistent with direction bit adjustments needed when node ordering changes. The full_stack fuzzer hex data correctly reorders node IDs to be sorted.

Previously flagged (from prior review, not repeated)

• lightning/src/routing/gossip.rs:2173 — update_channel_from_unsigned_announcement_intern still uses == (not >=) for the node_id check, creating an inconsistency with the >= in pre_channel_announcement_validation_check. Not a bug (the pre-check catches it first), but the error message "Channel announcement node had a channel with itself" is now misleading since it can only trigger for the bitcoin_key_1 == bitcoin_key_2 case.

No new issues found

[TheBlueMatt]

Drafting until claude fixes tests.

[codecov]

Codecov Report

❌ Patch coverage is 89.55224% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.66%. Comparing base (44828f7) to head (ac7e4e8).
⚠️ Report is 63 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/routing/gossip.rs	76.19%	3 Missing and 2 partials ⚠️
lightning/src/routing/scoring.rs	95.45%	1 Missing ⚠️
lightning/src/routing/test_utils.rs	95.45%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4611      +/-   ##
==========================================
+ Coverage   86.12%   86.66%   +0.54%     
==========================================
  Files         157      159       +2     
  Lines      108922   110612    +1690     
  Branches   108922   110612    +1690     
==========================================
+ Hits        93812    95865    +2053     
+ Misses      12495    12221     -274     
+ Partials     2615     2526      -89     

Flag	Coverage Δ
fuzzing-fake-hashes	6.77% <0.00%> (?)
fuzzing-real-hashes	23.28% <0.00%> (?)
tests	86.25% <89.55%> (+0.12%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tankyleo]

LGTM, just one nit from claude above

[TheBlueMatt]

Updated the other test utility:

$ git diff-tree -U1 271ac9149 86897b079
diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs
index f45cb0ae50..3dda719414 100644
--- a/lightning/src/routing/gossip.rs
+++ b/lightning/src/routing/gossip.rs
@@ -2840,4 +2840,11 @@ pub(crate) mod tests {
 	) -> ChannelAnnouncement {
-		let node_id_1 = PublicKey::from_secret_key(&secp_ctx, node_1_key);
-		let node_id_2 = PublicKey::from_secret_key(&secp_ctx, node_2_key);
+		let mut node_id_1 = NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, node_1_key));
+		let mut node_id_2 = NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, node_2_key));
+		let mut signer_1 = node_1_key;
+		let mut signer_2 = node_2_key;
+		if node_id_1 > node_id_2 {
+			core::mem::swap(&mut node_id_1, &mut node_id_2);
+			core::mem::swap(&mut signer_1, &mut signer_2);
+		}
+
 		let node_1_btckey = &SecretKey::from_slice(&[40; 32]).unwrap();
@@ -2849,4 +2856,4 @@ pub(crate) mod tests {
 			short_channel_id: 0,
-			node_id_1: NodeId::from_pubkey(&node_id_1),
-			node_id_2: NodeId::from_pubkey(&node_id_2),
+			node_id_1,
+			node_id_2,
 			bitcoin_key_1: NodeId::from_pubkey(&PublicKey::from_secret_key(
@@ -2864,4 +2871,4 @@ pub(crate) mod tests {
 		ChannelAnnouncement {
-			node_signature_1: secp_ctx.sign_ecdsa(&msghash, node_1_key),
-			node_signature_2: secp_ctx.sign_ecdsa(&msghash, node_2_key),
+			node_signature_1: secp_ctx.sign_ecdsa(&msghash, signer_1),
+			node_signature_2: secp_ctx.sign_ecdsa(&msghash, signer_2),
 			bitcoin_signature_1: secp_ctx.sign_ecdsa(&msghash, node_1_btckey),

[TheBlueMatt]

Oops, also had claude fix the full_stack fuzz regression test.

[elnosh]

LGTM

[tankyleo]

Codex had this to say WDYT ?

• Medium: persisted pre-upgrade unsorted channel announcements can still be relayed. The new validation rejects newly processed announcements, but lightning/src/routing/gossip.rs:1680 still loads serialized ChannelInfo unchanged, and initial gossip sync clones chan.announcement_message directly at
  lightning/src/routing/gossip.rs:575. A node upgrading with a graph saved by a version that accepted unsorted node_id_1/node_id_2 can keep advertising an announcement this commit would now reject. Consider dropping/suppressing unsorted stored announcement_messages, or rejecting/migrating such
  channels on read.