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
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
10 changes: 5 additions & 5 deletions fuzz/src/full_stack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1693,11 +1693,11 @@ fn gossip_exchange_seed() -> Vec<u8> {
// inbound read from peer id 0 of len 255
ext_from_hex("0300ff", &mut test);
// First part of channel_announcement (type 256)
ext_from_hex("0100 00000000000000000000000000000000000000000000000000000000000000b20303030303030303030303030303030303030303030303030303030303030303 00000000000000000000000000000000000000000000000000000000000000b20202020202020202020202020202020202020202020202020202020202020202 00000000000000000000000000000000000000000000000000000000000000b20303030303030303030303030303030303030303030303030303030303030303 00000000000000000000000000000000000000000000000000000000000000b20202020202020202020202020202020202020202020202020202020202", &mut test);
ext_from_hex("0100 00000000000000000000000000000000000000000000000000000000000000b20202020202020202020202020202020202020202020202020202020202020202 00000000000000000000000000000000000000000000000000000000000000b20303030303030303030303030303030303030303030303030303030303030303 00000000000000000000000000000000000000000000000000000000000000b20202020202020202020202020202020202020202020202020202020202020202 00000000000000000000000000000000000000000000000000000000000000b20303030303030303030303030303030303030303030303030303030303", &mut test);
// inbound read from peer id 0 of len 193
ext_from_hex("0300c1", &mut test);
// Last part of channel_announcement and mac
ext_from_hex("020202 00006fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000000000000000002a030303030303030303030303030303030303030303030303030303030303030303020202020202020202020202020202020202020202020202020202020202020202030303030303030303030303030303030303030303030303030303030303030303020202020202020202020202020202020202020202020202020202020202020202 03000000000000000000000000000000", &mut test);
ext_from_hex("030303 00006fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000000000000000002a020202020202020202020202020202020202020202020202020202020202020202030303030303030303030303030303030303030303030303030303030303030303020202020202020202020202020202020202020202020202020202020202020202030303030303030303030303030303030303030303030303030303030303030303 03000000000000000000000000000000", &mut test);

// inbound read from peer id 0 of len 18
ext_from_hex("030012", &mut test);
Expand All @@ -1706,7 +1706,7 @@ fn gossip_exchange_seed() -> Vec<u8> {
// inbound read from peer id 0 of len 154
ext_from_hex("03009a", &mut test);
// channel_update (type 258) and mac
ext_from_hex("0102 00000000000000000000000000000000000000000000000000000000000000a60303030303030303030303030303030303030303030303030303030303030303 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000 000000000000002a0000002c01000028000000000000000000000000000000000000000005f5e100 03000000000000000000000000000000", &mut test);
ext_from_hex("0102 00000000000000000000000000000000000000000000000000000000000000a60202020202020202020202020202020202020202020202020202020202020202 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000 000000000000002a0000002c01000028000000000000000000000000000000000000000005f5e100 03000000000000000000000000000000", &mut test);

// inbound read from peer id 0 of len 18
ext_from_hex("030012", &mut test);
Expand Down Expand Up @@ -2024,8 +2024,8 @@ mod tests {
super::do_test(&test, &(Arc::clone(&logger) as Arc<dyn Logger + MaybeSend + MaybeSync>));

let log_entries = logger.lines.lock().unwrap();
assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Sending message to all peers except Some(PublicKey(0000000000000000000000000000000000000000000000000000000000000002ff00000000000000000000000000000000000000000000000000000000000002)) or the announced channel's counterparties: ChannelAnnouncement { node_signature_1: 3026020200b202200303030303030303030303030303030303030303030303030303030303030303, node_signature_2: 3026020200b202200202020202020202020202020202020202020202020202020202020202020202, bitcoin_signature_1: 3026020200b202200303030303030303030303030303030303030303030303030303030303030303, bitcoin_signature_2: 3026020200b202200202020202020202020202020202020202020202020202020202020202020202, contents: UnsignedChannelAnnouncement { features: [], chain_hash: 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000, short_channel_id: 42, node_id_1: NodeId(030303030303030303030303030303030303030303030303030303030303030303), node_id_2: NodeId(020202020202020202020202020202020202020202020202020202020202020202), bitcoin_key_1: NodeId(030303030303030303030303030303030303030303030303030303030303030303), bitcoin_key_2: NodeId(020202020202020202020202020202020202020202020202020202020202020202), excess_data: [] } }".to_string())), Some(&1));
assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Sending message to all peers except Some(PublicKey(0000000000000000000000000000000000000000000000000000000000000002ff00000000000000000000000000000000000000000000000000000000000002)): ChannelUpdate { signature: 3026020200a602200303030303030303030303030303030303030303030303030303030303030303, contents: UnsignedChannelUpdate { chain_hash: 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000, short_channel_id: 42, timestamp: 44, message_flags: 1, channel_flags: 0, cltv_expiry_delta: 40, htlc_minimum_msat: 0, htlc_maximum_msat: 100000000, fee_base_msat: 0, fee_proportional_millionths: 0, excess_data: [] } }".to_string())), Some(&1));
assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Sending message to all peers except Some(PublicKey(0000000000000000000000000000000000000000000000000000000000000002ff00000000000000000000000000000000000000000000000000000000000002)) or the announced channel's counterparties: ChannelAnnouncement { node_signature_1: 3026020200b202200202020202020202020202020202020202020202020202020202020202020202, node_signature_2: 3026020200b202200303030303030303030303030303030303030303030303030303030303030303, bitcoin_signature_1: 3026020200b202200202020202020202020202020202020202020202020202020202020202020202, bitcoin_signature_2: 3026020200b202200303030303030303030303030303030303030303030303030303030303030303, contents: UnsignedChannelAnnouncement { features: [], chain_hash: 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000, short_channel_id: 42, node_id_1: NodeId(020202020202020202020202020202020202020202020202020202020202020202), node_id_2: NodeId(030303030303030303030303030303030303030303030303030303030303030303), bitcoin_key_1: NodeId(020202020202020202020202020202020202020202020202020202020202020202), bitcoin_key_2: NodeId(030303030303030303030303030303030303030303030303030303030303030303), excess_data: [] } }".to_string())), Some(&1));
assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Sending message to all peers except Some(PublicKey(0000000000000000000000000000000000000000000000000000000000000002ff00000000000000000000000000000000000000000000000000000000000002)): ChannelUpdate { signature: 3026020200a602200202020202020202020202020202020202020202020202020202020202020202, contents: UnsignedChannelUpdate { chain_hash: 6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000, short_channel_id: 42, timestamp: 44, message_flags: 1, channel_flags: 0, cltv_expiry_delta: 40, htlc_minimum_msat: 0, htlc_maximum_msat: 100000000, fee_base_msat: 0, fee_proportional_millionths: 0, excess_data: [] } }".to_string())), Some(&1));
assert_eq!(log_entries.get(&("lightning::ln::peer_handler".to_string(), "Sending message to all peers except Some(PublicKey(0000000000000000000000000000000000000000000000000000000000000002ff00000000000000000000000000000000000000000000000000000000000002)) or the announced node: NodeAnnouncement { signature: 302502012802200303030303030303030303030303030303030303030303030303030303030303, contents: UnsignedNodeAnnouncement { features: [], timestamp: 43, node_id: NodeId(030303030303030303030303030303030303030303030303030303030303030303), rgb: [0, 0, 0], alias: NodeAlias([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]), addresses: [], excess_address_data: [], excess_data: [] } }".to_string())), Some(&1));
}

Expand Down
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
32 changes: 23 additions & 9 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.
Comment thread
TheBlueMatt marked this conversation as resolved.
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 @@ -2831,17 +2838,24 @@ pub(crate) mod tests {
pub(crate) fn get_signed_channel_announcement<F: Fn(&mut UnsignedChannelAnnouncement)>(
f: F, node_1_key: &SecretKey, node_2_key: &SecretKey, secp_ctx: &Secp256k1<secp256k1::All>,
) -> 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();
let node_2_btckey = &SecretKey::from_slice(&[39; 32]).unwrap();

let mut unsigned_announcement = UnsignedChannelAnnouncement {
features: channelmanager::provided_channel_features(&UserConfig::default()),
chain_hash: ChainHash::using_genesis_block(Network::Testnet),
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(
&secp_ctx,
node_1_btckey,
Expand All @@ -2855,8 +2869,8 @@ pub(crate) mod tests {
f(&mut unsigned_announcement);
let msghash = hash_to_message!(&Sha256dHash::hash(&unsigned_announcement.encode()[..])[..]);
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),
bitcoin_signature_2: secp_ctx.sign_ecdsa(&msghash, node_2_btckey),
contents: unsigned_announcement,
Expand Down Expand Up @@ -3126,7 +3140,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