Enforce that node_ids are sorted in channel_announcements

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.

Author: TheBlueMatt

lightning-rapid-gossip-sync/src/processing.rs

@@ -549,7 +549,7 @@ mod tests {
 			108, 101, 46, 99, 111, 109, 1, 187, 19, 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
 			1, 5, 57, 13, 3, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 0, 2, 23, 48, 62, 77, 75, 108,
 			209, 54, 16, 50, 202, 155, 210, 174, 185, 217, 0, 170, 77, 69, 217, 234, 216, 10, 201,
-			66, 51, 116, 196, 81, 167, 37, 77, 7, 102, 0, 0, 2, 25, 48, 0, 0, 0, 1, 0, 0, 1, 0, 1,
+			66, 51, 116, 196, 81, 167, 37, 77, 7, 102, 0, 0, 2, 25, 48, 0, 0, 0, 1, 0, 0, 1, 1, 0,
 			0, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 			0, 1, 0, 0, 1,
 		];
@@ -669,7 +669,7 @@ mod tests {
 			0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 			0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 			0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
-			0, 0, 0, 1, 0, 0, 1, 0, 255, 128, 0, 0, 0, 0, 0, 0, 1, 0, 147, 42, 23, 23, 23, 23, 23,
+			0, 0, 0, 1, 0, 0, 1, 1, 255, 128, 0, 0, 0, 0, 0, 0, 0, 0, 147, 42, 23, 23, 23, 23, 23,
 			23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23,
 			23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23,
 			23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23, 23,

lightning/src/routing/gossip.rs

@@ -2014,9 +2014,9 @@ impl<L: Logger> NetworkGraph<L> {
 		&self, short_channel_id: u64, capacity_sats: Option<u64>, timestamp: u64,
 		features: ChannelFeatures, node_id_1: NodeId, node_id_2: NodeId,
 	) -> Result<(), LightningError> {
-		if node_id_1 == node_id_2 {
+		if node_id_1 >= node_id_2 {
 			return Err(LightningError {
-				err: "Channel announcement node had a channel with itself".to_owned(),
+				err: "node_ids in channel_announcements must be sorted".to_owned(),
 				action: ErrorAction::IgnoreError,
 			});
 		};
@@ -2123,6 +2123,13 @@ impl<L: Logger> NetworkGraph<L> {
 	) -> Result<(), LightningError> {
 		let channels = self.channels.read().unwrap();
 
+		if msg.node_id_1 >= msg.node_id_2 {
+			return Err(LightningError {
+				err: "node_ids in channel_announcements must be sorted".to_owned(),
+				action: ErrorAction::IgnoreError,
+			});
+		}
+
 		if let Some(chan) = channels.get(&msg.short_channel_id) {
 			if chan.capacity_sats.is_some() {
 				// If we'd previously looked up the channel on-chain and checked the script
@@ -3126,7 +3133,7 @@ pub(crate) mod tests {
 			.handle_channel_announcement(Some(node_1_pubkey), &channel_to_itself_announcement)
 		{
 			Ok(_) => panic!(),
-			Err(e) => assert_eq!(e.err, "Channel announcement node had a channel with itself"),
+			Err(e) => assert_eq!(e.err, "node_ids in channel_announcements must be sorted"),
 		};
 
 		// Test that channel announcements with the wrong chain hash are ignored (network graph is testnet,

lightning/src/routing/scoring.rs

@@ -2705,20 +2705,28 @@ mod tests {
 		let node_1_secret = &SecretKey::from_slice(&[39; 32]).unwrap();
 		let node_2_secret = &SecretKey::from_slice(&[40; 32]).unwrap();
 		let secp_ctx = Secp256k1::new();
+		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 node_signer_1 = &node_1_key;
+		let mut node_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 node_signer_1, &mut node_signer_2);
+		}
 		let unsigned_announcement = UnsignedChannelAnnouncement {
 			features: channelmanager::provided_channel_features(&UserConfig::default()),
 			chain_hash: genesis_hash,
 			short_channel_id,
-			node_id_1: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_1_key)),
-			node_id_2: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_2_key)),
+			node_id_1,
+			node_id_2,
 			bitcoin_key_1: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_1_secret)),
 			bitcoin_key_2: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_2_secret)),
 			excess_data: Vec::new(),
 		};
 		let msghash = hash_to_message!(&Sha256dHash::hash(&unsigned_announcement.encode()[..])[..]);
 		let signed_announcement = 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, node_signer_1),
+			node_signature_2: secp_ctx.sign_ecdsa(&msghash, node_signer_2),
 			bitcoin_signature_1: secp_ctx.sign_ecdsa(&msghash, &node_1_secret),
 			bitcoin_signature_2: secp_ctx.sign_ecdsa(&msghash, &node_2_secret),
 			contents: unsigned_announcement,
@@ -2732,10 +2740,23 @@ mod tests {
 
 	fn update_channel(
 		network_graph: &mut NetworkGraph<&TestLogger>, short_channel_id: u64, node_key: SecretKey,
-		channel_flags: u8, htlc_maximum_msat: u64, timestamp: u32,
+		mut channel_flags: u8, htlc_maximum_msat: u64, timestamp: u32,
 	) {
 		let genesis_hash = ChainHash::using_genesis_block(Network::Testnet);
 		let secp_ctx = Secp256k1::new();
+		let node_id = NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_key));
+		// `add_channel` may have swapped the node order to satisfy the spec's sorted node_ids
+		// requirement, so override `channel_flags` bit 0 to match the actual node position.
+		{
+			let read_only = network_graph.read_only();
+			if let Some(channel) = read_only.channel(short_channel_id) {
+				if channel.node_one == node_id {
+					channel_flags &= !1;
+				} else {
+					channel_flags |= 1;
+				}
+			}
+		}
 		let unsigned_update = UnsignedChannelUpdate {
 			chain_hash: genesis_hash,
 			short_channel_id,

lightning/src/routing/test_utils.rs

@@ -36,8 +36,14 @@ pub(crate) fn channel_announcement(
 	node_1_privkey: &SecretKey, node_2_privkey: &SecretKey, features: ChannelFeatures,
 	short_channel_id: u64, secp_ctx: &Secp256k1<All>,
 ) -> ChannelAnnouncement {
-	let node_id_1 = NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, node_1_privkey));
-	let node_id_2 = NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, node_2_privkey));
+	let mut node_id_1 = NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, node_1_privkey));
+	let mut node_id_2 = NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, node_2_privkey));
+	let mut signer_1 = node_1_privkey;
+	let mut signer_2 = node_2_privkey;
+	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 unsigned_announcement = UnsignedChannelAnnouncement {
 		features,
@@ -52,10 +58,10 @@ pub(crate) fn channel_announcement(
 
 	let msghash = hash_to_message!(&Sha256dHash::hash(&unsigned_announcement.encode()[..])[..]);
 	ChannelAnnouncement {
-		node_signature_1: secp_ctx.sign_ecdsa(&msghash, node_1_privkey),
-		node_signature_2: secp_ctx.sign_ecdsa(&msghash, node_2_privkey),
-		bitcoin_signature_1: secp_ctx.sign_ecdsa(&msghash, node_1_privkey),
-		bitcoin_signature_2: secp_ctx.sign_ecdsa(&msghash, node_2_privkey),
+		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, signer_1),
+		bitcoin_signature_2: secp_ctx.sign_ecdsa(&msghash, signer_2),
 		contents: unsigned_announcement.clone(),
 	}
 }
@@ -119,9 +125,25 @@ pub(crate) fn add_or_update_node(
 
 pub(crate) fn update_channel(
 	gossip_sync: &P2PGossipSync<Arc<NetworkGraph<Arc<test_utils::TestLogger>>>, Arc<test_utils::TestChainSource>, Arc<test_utils::TestLogger>>,
-	secp_ctx: &Secp256k1<All>, node_privkey: &SecretKey, update: UnsignedChannelUpdate
+	secp_ctx: &Secp256k1<All>, node_privkey: &SecretKey, mut update: UnsignedChannelUpdate
 ) {
 	let node_pubkey = PublicKey::from_secret_key(&secp_ctx, node_privkey);
+	let node_id = NodeId::from_pubkey(&node_pubkey);
+
+	// `channel_announcement` may have swapped the node order to satisfy the spec's sorted node_ids
+	// requirement, so override `channel_flags` bit 0 to match the actual node position recorded in
+	// the network graph.
+	{
+		let network_graph = gossip_sync.network_graph().read_only();
+		if let Some(channel) = network_graph.channel(update.short_channel_id) {
+			if channel.node_one == node_id {
+				update.channel_flags &= !1;
+			} else {
+				update.channel_flags |= 1;
+			}
+		}
+	}
+
 	let msghash = hash_to_message!(&Sha256dHash::hash(&update.encode()[..])[..]);
 	let valid_channel_update = ChannelUpdate {
 		signature: secp_ctx.sign_ecdsa(&msghash, node_privkey),