Skip to content

Enforce that node_ids are sorted in channel_announcements #4611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TheBlueMatt wants to merge 1 commit into
base: main
Choose a base branch
from
+67 −17
Open

Enforce that node_ids are sorted in channel_announcements #4611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lightning-rapid-gossip-sync/src/processing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
];
Expand Down Expand Up @@ -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,
Expand Down
13 changes: 10 additions & 3 deletions lightning/src/routing/gossip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
};
Expand Down Expand Up @@ -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 {
Comment thread
TheBlueMatt marked this conversation as resolved.
Copy link
Copy Markdown
Collaborator

@ldk-claude-review-bot ldk-claude-review-bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor

@tankyleo tankyleo May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's address this comment ?

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
Expand Down Expand Up @@ -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,
Expand Down
31 changes: 26 additions & 5 deletions lightning/src/routing/scoring.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
36 changes: 29 additions & 7 deletions lightning/src/routing/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(),
}
}
Expand Down Expand Up @@ -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),
Expand Down
Loading