Skip to content

Commit 72a231e

Browse files
alecchendevalecchendev
committed
Swap PublicKey for NodeId in UnsignedChannelAnnouncement
This also adds the macro `get_pubkey_from_node_id` to parse `PublicKey`s back from `NodeId`s for signature verification.
1 parent 760ab65 commit 72a231e

File tree

8 files changed

+75
-52
lines changed

8 files changed

+75
-52
lines changed

‎fuzz/src/router.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -180,14 +180,14 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
180180
},
181181
1 => {
182182
let msg = decode_msg_with_len16!(msgs::UnsignedChannelAnnouncement, 32+8+33*4);
183-
node_pks.insert(msg.node_id_1);
184-
node_pks.insert(msg.node_id_2);
183+
node_pks.insert(PublicKey::from_slice(msg.node_id_1.as_slice()).unwrap());
184+
node_pks.insert(PublicKey::from_slice(msg.node_id_2.as_slice()).unwrap());
185185
let _ = net_graph.update_channel_from_unsigned_announcement::<&FuzzChainSource>(&msg, &None);
186186
},
187187
2 => {
188188
let msg = decode_msg_with_len16!(msgs::UnsignedChannelAnnouncement, 32+8+33*4);
189-
node_pks.insert(msg.node_id_1);
190-
node_pks.insert(msg.node_id_2);
189+
node_pks.insert(PublicKey::from_slice(msg.node_id_1.as_slice()).unwrap());
190+
node_pks.insert(PublicKey::from_slice(msg.node_id_2.as_slice()).unwrap());
191191
let _ = net_graph.update_channel_from_unsigned_announcement(&msg, &Some(&FuzzChainSource { input: Arc::clone(&input) }));
192192
},
193193
3 => {

‎lightning/src/ln/channel.rs

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, LowerBounde
3636
use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS};
3737
use crate::chain::transaction::{OutPoint, TransactionData};
3838
use crate::chain::keysinterface::{WriteableEcdsaChannelSigner, EntropySource, ChannelSigner, SignerProvider, NodeSigner, Recipient};
39+
use crate::routing::gossip::NodeId;
3940
use crate::util::events::ClosureReason;
4041
use crate::util::ser::{Readable, ReadableArgs, Writeable, Writer, VecWriter};
4142
use crate::util::logger::Logger;
@@ -5416,18 +5417,19 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
54165417
return Err(ChannelError::Ignore("Cannot get a ChannelAnnouncement if the channel is not currently usable".to_owned()));
54175418
}
54185419

5419-
let node_id = node_signer.get_node_id(Recipient::Node)
5420-
.map_err(|_| ChannelError::Ignore("Failed to retrieve own public key".to_owned()))?;
5421-
let were_node_one = node_id.serialize()[..] < self.counterparty_node_id.serialize()[..];
5420+
let node_id = NodeId::from_pubkey(&node_signer.get_node_id(Recipient::Node)
5421+
.map_err(|_| ChannelError::Ignore("Failed to retrieve own public key".to_owned()))?);
5422+
let counterparty_node_id = NodeId::from_pubkey(&self.get_counterparty_node_id());
5423+
let were_node_one = node_id.as_slice() < counterparty_node_id.as_slice();
54225424

54235425
let msg = msgs::UnsignedChannelAnnouncement {
54245426
features: channelmanager::provided_channel_features(&user_config),
54255427
chain_hash,
54265428
short_channel_id: self.get_short_channel_id().unwrap(),
5427-
node_id_1: if were_node_one { node_id } else { self.get_counterparty_node_id() },
5428-
node_id_2: if were_node_one { self.get_counterparty_node_id() } else { node_id },
5429-
bitcoin_key_1: if were_node_one { self.get_holder_pubkeys().funding_pubkey } else { self.counterparty_funding_pubkey().clone() },
5430-
bitcoin_key_2: if were_node_one { self.counterparty_funding_pubkey().clone() } else { self.get_holder_pubkeys().funding_pubkey },
5429+
node_id_1: if were_node_one { node_id } else { counterparty_node_id },
5430+
node_id_2: if were_node_one { counterparty_node_id } else { node_id },
5431+
bitcoin_key_1: NodeId::from_pubkey(if were_node_one { &self.get_holder_pubkeys().funding_pubkey } else { self.counterparty_funding_pubkey() }),
5432+
bitcoin_key_2: NodeId::from_pubkey(if were_node_one { self.counterparty_funding_pubkey() } else { &self.get_holder_pubkeys().funding_pubkey }),
54315433
excess_data: Vec::new(),
54325434
};
54335435

@@ -5497,8 +5499,8 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
54975499
&self, node_signer: &NS, announcement: msgs::UnsignedChannelAnnouncement
54985500
) -> Result<msgs::ChannelAnnouncement, ChannelError> where NS::Target: NodeSigner {
54995501
if let Some((their_node_sig, their_bitcoin_sig)) = self.announcement_sigs {
5500-
let our_node_key = node_signer.get_node_id(Recipient::Node)
5501-
.map_err(|_| ChannelError::Ignore("Signer failed to retrieve own public key".to_owned()))?;
5502+
let our_node_key = NodeId::from_pubkey(&node_signer.get_node_id(Recipient::Node)
5503+
.map_err(|_| ChannelError::Ignore("Signer failed to retrieve own public key".to_owned()))?);
55025504
let were_node_one = announcement.node_id_1 == our_node_key;
55035505

55045506
let our_node_sig = node_signer.sign_gossip_message(msgs::UnsignedGossipMessage::ChannelAnnouncement(&announcement))

‎lightning/src/ln/msgs.rs

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ use crate::util::ser::{LengthReadable, Readable, ReadableArgs, Writeable, Writer
4646

4747
use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret};
4848

49+
use crate::routing::gossip::NodeId;
50+
4951
/// 21 million * 10^8 * 1000
5052
pub(crate) const MAX_VALUE_MSAT: u64 = 21_000_000_0000_0000_000;
5153

@@ -698,13 +700,13 @@ pub struct UnsignedChannelAnnouncement {
698700
/// The short channel ID
699701
pub short_channel_id: u64,
700702
/// One of the two `node_id`s which are endpoints of this channel
701-
pub node_id_1: PublicKey,
703+
pub node_id_1: NodeId,
702704
/// The other of the two `node_id`s which are endpoints of this channel
703-
pub node_id_2: PublicKey,
705+
pub node_id_2: NodeId,
704706
/// The funding key for the first node
705-
pub bitcoin_key_1: PublicKey,
707+
pub bitcoin_key_1: NodeId,
706708
/// The funding key for the second node
707-
pub bitcoin_key_2: PublicKey,
709+
pub bitcoin_key_2: NodeId,
708710
pub(crate) excess_data: Vec<u8>,
709711
}
710712
/// A [`channel_announcement`] message to be sent to or received from a peer.
@@ -2053,6 +2055,7 @@ mod tests {
20532055
use crate::ln::features::{ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures};
20542056
use crate::ln::msgs;
20552057
use crate::ln::msgs::{FinalOnionHopData, OptionalField, OnionErrorPacket, OnionHopDataFormat};
2058+
use crate::routing::gossip::NodeId;
20562059
use crate::util::ser::{Writeable, Readable, Hostname};
20572060

20582061
use bitcoin::hashes::hex::FromHex;
@@ -2160,10 +2163,10 @@ mod tests {
21602163
features,
21612164
chain_hash: BlockHash::from_hex("6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000").unwrap(),
21622165
short_channel_id: 2316138423780173,
2163-
node_id_1: pubkey_1,
2164-
node_id_2: pubkey_2,
2165-
bitcoin_key_1: pubkey_3,
2166-
bitcoin_key_2: pubkey_4,
2166+
node_id_1: NodeId::from_pubkey(&pubkey_1),
2167+
node_id_2: NodeId::from_pubkey(&pubkey_2),
2168+
bitcoin_key_1: NodeId::from_pubkey(&pubkey_3),
2169+
bitcoin_key_2: NodeId::from_pubkey(&pubkey_4),
21672170
excess_data: if excess_data { vec![10, 0, 0, 20, 0, 0, 30, 0, 0, 40] } else { Vec::new() },
21682171
};
21692172
let channel_announcement = msgs::ChannelAnnouncement {

‎lightning/src/ln/peer_handler.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ use crate::ln::peer_channel_encryptor::{PeerChannelEncryptor,NextNoiseStep};
2727
use crate::ln::wire;
2828
use crate::ln::wire::Encode;
2929
use crate::onion_message::{CustomOnionMessageContents, CustomOnionMessageHandler, SimpleArcOnionMessenger, SimpleRefOnionMessenger};
30-
use crate::routing::gossip::{NetworkGraph, P2PGossipSync};
30+
use crate::routing::gossip::{NetworkGraph, P2PGossipSync, NodeId};
3131
use crate::util::atomic_counter::AtomicCounter;
3232
use crate::util::events::{MessageSendEvent, MessageSendEventsProvider, OnionMessageProvider};
3333
use crate::util::logger::Logger;
@@ -1467,9 +1467,11 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
14671467
log_gossip!(self.logger, "Skipping broadcast message to {:?} as its outbound buffer is full", peer.their_node_id);
14681468
continue;
14691469
}
1470-
if peer.their_node_id.as_ref() == Some(&msg.contents.node_id_1) ||
1471-
peer.their_node_id.as_ref() == Some(&msg.contents.node_id_2) {
1472-
continue;
1470+
if let Some(their_node_id) = peer.their_node_id {
1471+
let their_node_id = NodeId::from_pubkey(&their_node_id);
1472+
if their_node_id == msg.contents.node_id_1 || their_node_id == msg.contents.node_id_2 {
1473+
continue;
1474+
}
14731475
}
14741476
if except_node.is_some() && peer.their_node_id.as_ref() == except_node {
14751477
continue;

‎lightning/src/routing/gossip.rs

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,22 @@ macro_rules! secp_verify_sig {
326326
};
327327
}
328328

329+
macro_rules! get_pubkey_from_node_id {
330+
( $node_id: expr, $msg_type: expr ) => {
331+
PublicKey::from_slice($node_id.as_slice())
332+
.map_err(|_| LightningError {
333+
err: format!("Invalid public key on {} message", $msg_type),
334+
action: ErrorAction::SendWarningMessage {
335+
msg: msgs::WarningMessage {
336+
channel_id: [0; 32],
337+
data: format!("Invalid public key on {} message", $msg_type),
338+
},
339+
log_level: Level::Trace
340+
}
341+
})?
342+
}
343+
}
344+
329345
impl<G: Deref<Target=NetworkGraph<L>>, C: Deref, L: Deref> RoutingMessageHandler for P2PGossipSync<G, C, L>
330346
where C::Target: chain::Access, L::Target: Logger
331347
{
@@ -1330,10 +1346,10 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
13301346
C::Target: chain::Access,
13311347
{
13321348
let msg_hash = hash_to_message!(&Sha256dHash::hash(&msg.contents.encode()[..])[..]);
1333-
secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.node_signature_1, &msg.contents.node_id_1, "channel_announcement");
1334-
secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.node_signature_2, &msg.contents.node_id_2, "channel_announcement");
1335-
secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.bitcoin_signature_1, &msg.contents.bitcoin_key_1, "channel_announcement");
1336-
secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.bitcoin_signature_2, &msg.contents.bitcoin_key_2, "channel_announcement");
1349+
secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.node_signature_1, &get_pubkey_from_node_id!(msg.contents.node_id_1, "channel_announcement"), "channel_announcement");
1350+
secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.node_signature_2, &get_pubkey_from_node_id!(msg.contents.node_id_2, "channel_announcement"), "channel_announcement");
1351+
secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.bitcoin_signature_1, &get_pubkey_from_node_id!(msg.contents.bitcoin_key_1, "channel_announcement"), "channel_announcement");
1352+
secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.bitcoin_signature_2, &get_pubkey_from_node_id!(msg.contents.bitcoin_key_2, "channel_announcement"), "channel_announcement");
13371353
self.update_channel_from_unsigned_announcement_intern(&msg.contents, Some(msg), chain_access)
13381354
}
13391355

@@ -1438,9 +1454,6 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
14381454
return Err(LightningError{err: "Channel announcement node had a channel with itself".to_owned(), action: ErrorAction::IgnoreError});
14391455
}
14401456

1441-
let node_one = NodeId::from_pubkey(&msg.node_id_1);
1442-
let node_two = NodeId::from_pubkey(&msg.node_id_2);
1443-
14441457
{
14451458
let channels = self.channels.read().unwrap();
14461459

@@ -1457,7 +1470,7 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
14571470
// We use the Node IDs rather than the bitcoin_keys to check for "equivalence"
14581471
// as we didn't (necessarily) store the bitcoin keys, and we only really care
14591472
// if the peers on the channel changed anyway.
1460-
if node_one == chan.node_one && node_two == chan.node_two {
1473+
if msg.node_id_1 == chan.node_one && msg.node_id_2 == chan.node_two {
14611474
return Err(LightningError {
14621475
err: "Already have chain-validated channel".to_owned(),
14631476
action: ErrorAction::IgnoreDuplicateGossip
@@ -1478,8 +1491,8 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
14781491
let removed_channels = self.removed_channels.lock().unwrap();
14791492
let removed_nodes = self.removed_nodes.lock().unwrap();
14801493
if removed_channels.contains_key(&msg.short_channel_id) ||
1481-
removed_nodes.contains_key(&node_one) ||
1482-
removed_nodes.contains_key(&node_two) {
1494+
removed_nodes.contains_key(&msg.node_id_1) ||
1495+
removed_nodes.contains_key(&msg.node_id_2) {
14831496
return Err(LightningError{
14841497
err: format!("Channel with SCID {} or one of its nodes was removed from our network graph recently", &msg.short_channel_id),
14851498
action: ErrorAction::IgnoreAndLog(Level::Gossip)});
@@ -1495,7 +1508,7 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
14951508
match chain_access.get_utxo(&msg.chain_hash, msg.short_channel_id) {
14961509
Ok(TxOut { value, script_pubkey }) => {
14971510
let expected_script =
1498-
make_funding_redeemscript(&msg.bitcoin_key_1, &msg.bitcoin_key_2).to_v0_p2wsh();
1511+
make_funding_redeemscript(&get_pubkey_from_node_id!(msg.bitcoin_key_1, "channel_announcement"), &get_pubkey_from_node_id!(msg.bitcoin_key_2, "channel_announcement")).to_v0_p2wsh();
14991512
if script_pubkey != expected_script {
15001513
return Err(LightningError{err: format!("Channel announcement key ({}) didn't match on-chain script ({})", expected_script.to_hex(), script_pubkey.to_hex()), action: ErrorAction::IgnoreError});
15011514
}
@@ -1522,9 +1535,9 @@ impl<L: Deref> NetworkGraph<L> where L::Target: Logger {
15221535

15231536
let chan_info = ChannelInfo {
15241537
features: msg.features.clone(),
1525-
node_one,
1538+
node_one: msg.node_id_1,
15261539
one_to_two: None,
1527-
node_two,
1540+
node_two: msg.node_id_2,
15281541
two_to_one: None,
15291542
capacity_sats: utxo_value,
15301543
announcement_message: if msg.excess_data.len() <= MAX_EXCESS_BYTES_FOR_RELAY
@@ -1987,10 +2000,10 @@ mod tests {
19872000
features: channelmanager::provided_channel_features(&UserConfig::default()),
19882001
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
19892002
short_channel_id: 0,
1990-
node_id_1,
1991-
node_id_2,
1992-
bitcoin_key_1: PublicKey::from_secret_key(&secp_ctx, node_1_btckey),
1993-
bitcoin_key_2: PublicKey::from_secret_key(&secp_ctx, node_2_btckey),
2003+
node_id_1: NodeId::from_pubkey(&node_id_1),
2004+
node_id_2: NodeId::from_pubkey(&node_id_2),
2005+
bitcoin_key_1: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, node_1_btckey)),
2006+
bitcoin_key_2: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, node_2_btckey)),
19942007
excess_data: Vec::new(),
19952008
};
19962009
f(&mut unsigned_announcement);

‎lightning/src/routing/scoring.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1779,10 +1779,10 @@ mod tests {
17791779
features: channelmanager::provided_channel_features(&UserConfig::default()),
17801780
chain_hash: genesis_hash,
17811781
short_channel_id,
1782-
node_id_1: PublicKey::from_secret_key(&secp_ctx, &node_1_key),
1783-
node_id_2: PublicKey::from_secret_key(&secp_ctx, &node_2_key),
1784-
bitcoin_key_1: PublicKey::from_secret_key(&secp_ctx, &node_1_secret),
1785-
bitcoin_key_2: PublicKey::from_secret_key(&secp_ctx, &node_2_secret),
1782+
node_id_1: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_1_key)),
1783+
node_id_2: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_2_key)),
1784+
bitcoin_key_1: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_1_secret)),
1785+
bitcoin_key_2: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_2_secret)),
17861786
excess_data: Vec::new(),
17871787
};
17881788
let msghash = hash_to_message!(&Sha256dHash::hash(&unsigned_announcement.encode()[..])[..]);

‎lightning/src/routing/test_utils.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,15 @@ use bitcoin::secp256k1::{Secp256k1, All};
2727
use crate::prelude::*;
2828
use crate::sync::{self, Arc};
2929

30+
use crate::routing::gossip::NodeId;
31+
3032
// Using the same keys for LN and BTC ids
3133
pub(super) fn add_channel(
3234
gossip_sync: &P2PGossipSync<Arc<NetworkGraph<Arc<test_utils::TestLogger>>>, Arc<test_utils::TestChainSource>, Arc<test_utils::TestLogger>>,
3335
secp_ctx: &Secp256k1<All>, node_1_privkey: &SecretKey, node_2_privkey: &SecretKey, features: ChannelFeatures, short_channel_id: u64
3436
) {
35-
let node_id_1 = PublicKey::from_secret_key(&secp_ctx, node_1_privkey);
36-
let node_id_2 = PublicKey::from_secret_key(&secp_ctx, node_2_privkey);
37+
let node_id_1 = NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, node_1_privkey));
38+
let node_id_2 = NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, node_2_privkey));
3739

3840
let unsigned_announcement = UnsignedChannelAnnouncement {
3941
features,

‎lightning/src/util/test_utils.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ use crate::ln::{msgs, wire};
2323
use crate::ln::msgs::LightningError;
2424
use crate::ln::script::ShutdownScript;
2525
use crate::routing::gossip::NetworkGraph;
26+
use crate::routing::gossip::NodeId;
2627
use crate::routing::router::{find_route, InFlightHtlcs, Route, RouteHop, RouteParameters, Router, ScorerAccountingForInFlightHtlcs};
2728
use crate::routing::scoring::FixedPenaltyScorer;
2829
use crate::util::config::UserConfig;
@@ -435,10 +436,10 @@ fn get_dummy_channel_announcement(short_chan_id: u64) -> msgs::ChannelAnnounceme
435436
features: ChannelFeatures::empty(),
436437
chain_hash: genesis_block(network).header.block_hash(),
437438
short_channel_id: short_chan_id,
438-
node_id_1: PublicKey::from_secret_key(&secp_ctx, &node_1_privkey),
439-
node_id_2: PublicKey::from_secret_key(&secp_ctx, &node_2_privkey),
440-
bitcoin_key_1: PublicKey::from_secret_key(&secp_ctx, &node_1_btckey),
441-
bitcoin_key_2: PublicKey::from_secret_key(&secp_ctx, &node_2_btckey),
439+
node_id_1: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_1_privkey)),
440+
node_id_2: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_2_privkey)),
441+
bitcoin_key_1: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_1_btckey)),
442+
bitcoin_key_2: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_2_btckey)),
442443
excess_data: Vec::new(),
443444
};
444445

0 commit comments

Comments
 (0)