From: Alec Chen Date: Mon, 6 Feb 2023 20:41:18 +0000 (-0600) Subject: Swap `PublicKey` for `NodeId` in `UnsignedChannelAnnouncement` X-Git-Tag: v0.0.114-beta~27^2~1 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=1fd95496d1038352535a74065d2b06559e2192fb;p=rust-lightning Swap `PublicKey` for `NodeId` in `UnsignedChannelAnnouncement` Adds the macro `get_pubkey_from_node_id` to parse `PublicKey`s back from `NodeId`s for signature verification, as well as `make_funding_redeemscript_from_slices` to avoid parsing back and forth between types. --- diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs index 86538bbb6..72c834e72 100644 --- a/fuzz/src/router.rs +++ b/fuzz/src/router.rs @@ -149,6 +149,15 @@ pub fn do_test(data: &[u8], out: Out) { } } + macro_rules! get_pubkey_from_node_id { + ($node_id: expr ) => { + match PublicKey::from_slice($node_id.as_slice()) { + Ok(pk) => pk, + Err(_) => return, + } + } + } + macro_rules! get_pubkey { () => { match PublicKey::from_slice(get_slice!(33)) { @@ -180,14 +189,14 @@ pub fn do_test(data: &[u8], out: Out) { }, 1 => { let msg = decode_msg_with_len16!(msgs::UnsignedChannelAnnouncement, 32+8+33*4); - node_pks.insert(msg.node_id_1); - node_pks.insert(msg.node_id_2); + node_pks.insert(get_pubkey_from_node_id!(msg.node_id_1)); + node_pks.insert(get_pubkey_from_node_id!(msg.node_id_2)); let _ = net_graph.update_channel_from_unsigned_announcement::<&FuzzChainSource>(&msg, &None); }, 2 => { let msg = decode_msg_with_len16!(msgs::UnsignedChannelAnnouncement, 32+8+33*4); - node_pks.insert(msg.node_id_1); - node_pks.insert(msg.node_id_2); + node_pks.insert(get_pubkey_from_node_id!(msg.node_id_1)); + node_pks.insert(get_pubkey_from_node_id!(msg.node_id_2)); let _ = net_graph.update_channel_from_unsigned_announcement(&msg, &Some(&FuzzChainSource { input: Arc::clone(&input) })); }, 3 => { diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 31881d05d..f1f4cb8cc 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -23,6 +23,7 @@ use bitcoin::hash_types::{Txid, PubkeyHash}; use crate::ln::{PaymentHash, PaymentPreimage}; use crate::ln::msgs::DecodeError; +use crate::routing::gossip::NodeId; use crate::util::ser::{Readable, Writeable, Writer}; use crate::util::transaction_utils; @@ -660,13 +661,17 @@ pub fn make_funding_redeemscript(broadcaster: &PublicKey, countersignatory: &Pub let broadcaster_funding_key = broadcaster.serialize(); let countersignatory_funding_key = countersignatory.serialize(); + make_funding_redeemscript_from_slices(&broadcaster_funding_key, &countersignatory_funding_key) +} + +pub(crate) fn make_funding_redeemscript_from_slices(broadcaster_funding_key: &[u8], countersignatory_funding_key: &[u8]) -> Script { let builder = Builder::new().push_opcode(opcodes::all::OP_PUSHNUM_2); if broadcaster_funding_key[..] < countersignatory_funding_key[..] { - builder.push_slice(&broadcaster_funding_key) - .push_slice(&countersignatory_funding_key) + builder.push_slice(broadcaster_funding_key) + .push_slice(countersignatory_funding_key) } else { - builder.push_slice(&countersignatory_funding_key) - .push_slice(&broadcaster_funding_key) + builder.push_slice(countersignatory_funding_key) + .push_slice(broadcaster_funding_key) }.push_opcode(opcodes::all::OP_PUSHNUM_2).push_opcode(opcodes::all::OP_CHECKMULTISIG).into_script() } diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index d9ec57d5a..62a2b7b51 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -36,6 +36,7 @@ use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, LowerBounde use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS}; use crate::chain::transaction::{OutPoint, TransactionData}; use crate::chain::keysinterface::{WriteableEcdsaChannelSigner, EntropySource, ChannelSigner, SignerProvider, NodeSigner, Recipient}; +use crate::routing::gossip::NodeId; use crate::util::events::ClosureReason; use crate::util::ser::{Readable, ReadableArgs, Writeable, Writer, VecWriter}; use crate::util::logger::Logger; @@ -5416,18 +5417,19 @@ impl Channel { return Err(ChannelError::Ignore("Cannot get a ChannelAnnouncement if the channel is not currently usable".to_owned())); } - let node_id = node_signer.get_node_id(Recipient::Node) - .map_err(|_| ChannelError::Ignore("Failed to retrieve own public key".to_owned()))?; - let were_node_one = node_id.serialize()[..] < self.counterparty_node_id.serialize()[..]; + let node_id = NodeId::from_pubkey(&node_signer.get_node_id(Recipient::Node) + .map_err(|_| ChannelError::Ignore("Failed to retrieve own public key".to_owned()))?); + let counterparty_node_id = NodeId::from_pubkey(&self.get_counterparty_node_id()); + let were_node_one = node_id.as_slice() < counterparty_node_id.as_slice(); let msg = msgs::UnsignedChannelAnnouncement { features: channelmanager::provided_channel_features(&user_config), chain_hash, short_channel_id: self.get_short_channel_id().unwrap(), - node_id_1: if were_node_one { node_id } else { self.get_counterparty_node_id() }, - node_id_2: if were_node_one { self.get_counterparty_node_id() } else { node_id }, - bitcoin_key_1: if were_node_one { self.get_holder_pubkeys().funding_pubkey } else { self.counterparty_funding_pubkey().clone() }, - bitcoin_key_2: if were_node_one { self.counterparty_funding_pubkey().clone() } else { self.get_holder_pubkeys().funding_pubkey }, + node_id_1: if were_node_one { node_id } else { counterparty_node_id }, + node_id_2: if were_node_one { counterparty_node_id } else { node_id }, + bitcoin_key_1: NodeId::from_pubkey(if were_node_one { &self.get_holder_pubkeys().funding_pubkey } else { self.counterparty_funding_pubkey() }), + bitcoin_key_2: NodeId::from_pubkey(if were_node_one { self.counterparty_funding_pubkey() } else { &self.get_holder_pubkeys().funding_pubkey }), excess_data: Vec::new(), }; @@ -5497,8 +5499,8 @@ impl Channel { &self, node_signer: &NS, announcement: msgs::UnsignedChannelAnnouncement ) -> Result where NS::Target: NodeSigner { if let Some((their_node_sig, their_bitcoin_sig)) = self.announcement_sigs { - let our_node_key = node_signer.get_node_id(Recipient::Node) - .map_err(|_| ChannelError::Ignore("Signer failed to retrieve own public key".to_owned()))?; + let our_node_key = NodeId::from_pubkey(&node_signer.get_node_id(Recipient::Node) + .map_err(|_| ChannelError::Ignore("Signer failed to retrieve own public key".to_owned()))?); let were_node_one = announcement.node_id_1 == our_node_key; let our_node_sig = node_signer.sign_gossip_message(msgs::UnsignedGossipMessage::ChannelAnnouncement(&announcement)) diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 40817aa3c..942bd82e9 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -46,6 +46,8 @@ use crate::util::ser::{LengthReadable, Readable, ReadableArgs, Writeable, Writer use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret}; +use crate::routing::gossip::NodeId; + /// 21 million * 10^8 * 1000 pub(crate) const MAX_VALUE_MSAT: u64 = 21_000_000_0000_0000_000; @@ -698,13 +700,13 @@ pub struct UnsignedChannelAnnouncement { /// The short channel ID pub short_channel_id: u64, /// One of the two `node_id`s which are endpoints of this channel - pub node_id_1: PublicKey, + pub node_id_1: NodeId, /// The other of the two `node_id`s which are endpoints of this channel - pub node_id_2: PublicKey, + pub node_id_2: NodeId, /// The funding key for the first node - pub bitcoin_key_1: PublicKey, + pub bitcoin_key_1: NodeId, /// The funding key for the second node - pub bitcoin_key_2: PublicKey, + pub bitcoin_key_2: NodeId, pub(crate) excess_data: Vec, } /// A [`channel_announcement`] message to be sent to or received from a peer. @@ -2053,6 +2055,7 @@ mod tests { use crate::ln::features::{ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures}; use crate::ln::msgs; use crate::ln::msgs::{FinalOnionHopData, OptionalField, OnionErrorPacket, OnionHopDataFormat}; + use crate::routing::gossip::NodeId; use crate::util::ser::{Writeable, Readable, Hostname}; use bitcoin::hashes::hex::FromHex; @@ -2160,10 +2163,10 @@ mod tests { features, chain_hash: BlockHash::from_hex("6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000").unwrap(), short_channel_id: 2316138423780173, - node_id_1: pubkey_1, - node_id_2: pubkey_2, - bitcoin_key_1: pubkey_3, - bitcoin_key_2: pubkey_4, + node_id_1: NodeId::from_pubkey(&pubkey_1), + node_id_2: NodeId::from_pubkey(&pubkey_2), + bitcoin_key_1: NodeId::from_pubkey(&pubkey_3), + bitcoin_key_2: NodeId::from_pubkey(&pubkey_4), excess_data: if excess_data { vec![10, 0, 0, 20, 0, 0, 30, 0, 0, 40] } else { Vec::new() }, }; let channel_announcement = msgs::ChannelAnnouncement { diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index a63f764cd..f593e23c8 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -27,7 +27,7 @@ use crate::ln::peer_channel_encryptor::{PeerChannelEncryptor,NextNoiseStep}; use crate::ln::wire; use crate::ln::wire::Encode; use crate::onion_message::{CustomOnionMessageContents, CustomOnionMessageHandler, SimpleArcOnionMessenger, SimpleRefOnionMessenger}; -use crate::routing::gossip::{NetworkGraph, P2PGossipSync}; +use crate::routing::gossip::{NetworkGraph, P2PGossipSync, NodeId}; use crate::util::atomic_counter::AtomicCounter; use crate::util::events::{MessageSendEvent, MessageSendEventsProvider, OnionMessageProvider}; use crate::util::logger::Logger; @@ -1467,9 +1467,11 @@ impl { + PublicKey::from_slice($node_id.as_slice()) + .map_err(|_| LightningError { + err: format!("Invalid public key on {} message", $msg_type), + action: ErrorAction::SendWarningMessage { + msg: msgs::WarningMessage { + channel_id: [0; 32], + data: format!("Invalid public key on {} message", $msg_type), + }, + log_level: Level::Trace + } + })? + } +} + impl>, C: Deref, L: Deref> RoutingMessageHandler for P2PGossipSync where C::Target: chain::Access, L::Target: Logger { @@ -1330,10 +1346,10 @@ impl NetworkGraph where L::Target: Logger { C::Target: chain::Access, { let msg_hash = hash_to_message!(&Sha256dHash::hash(&msg.contents.encode()[..])[..]); - secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.node_signature_1, &msg.contents.node_id_1, "channel_announcement"); - secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.node_signature_2, &msg.contents.node_id_2, "channel_announcement"); - secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.bitcoin_signature_1, &msg.contents.bitcoin_key_1, "channel_announcement"); - secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.bitcoin_signature_2, &msg.contents.bitcoin_key_2, "channel_announcement"); + 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"); + 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"); + 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"); + 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"); self.update_channel_from_unsigned_announcement_intern(&msg.contents, Some(msg), chain_access) } @@ -1438,9 +1454,6 @@ impl NetworkGraph where L::Target: Logger { return Err(LightningError{err: "Channel announcement node had a channel with itself".to_owned(), action: ErrorAction::IgnoreError}); } - let node_one = NodeId::from_pubkey(&msg.node_id_1); - let node_two = NodeId::from_pubkey(&msg.node_id_2); - { let channels = self.channels.read().unwrap(); @@ -1457,7 +1470,7 @@ impl NetworkGraph where L::Target: Logger { // We use the Node IDs rather than the bitcoin_keys to check for "equivalence" // as we didn't (necessarily) store the bitcoin keys, and we only really care // if the peers on the channel changed anyway. - if node_one == chan.node_one && node_two == chan.node_two { + if msg.node_id_1 == chan.node_one && msg.node_id_2 == chan.node_two { return Err(LightningError { err: "Already have chain-validated channel".to_owned(), action: ErrorAction::IgnoreDuplicateGossip @@ -1478,8 +1491,8 @@ impl NetworkGraph where L::Target: Logger { let removed_channels = self.removed_channels.lock().unwrap(); let removed_nodes = self.removed_nodes.lock().unwrap(); if removed_channels.contains_key(&msg.short_channel_id) || - removed_nodes.contains_key(&node_one) || - removed_nodes.contains_key(&node_two) { + removed_nodes.contains_key(&msg.node_id_1) || + removed_nodes.contains_key(&msg.node_id_2) { return Err(LightningError{ err: format!("Channel with SCID {} or one of its nodes was removed from our network graph recently", &msg.short_channel_id), action: ErrorAction::IgnoreAndLog(Level::Gossip)}); @@ -1495,7 +1508,7 @@ impl NetworkGraph where L::Target: Logger { match chain_access.get_utxo(&msg.chain_hash, msg.short_channel_id) { Ok(TxOut { value, script_pubkey }) => { let expected_script = - make_funding_redeemscript(&msg.bitcoin_key_1, &msg.bitcoin_key_2).to_v0_p2wsh(); + make_funding_redeemscript_from_slices(msg.bitcoin_key_1.as_slice(), msg.bitcoin_key_2.as_slice()).to_v0_p2wsh(); if script_pubkey != expected_script { 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}); } @@ -1522,9 +1535,9 @@ impl NetworkGraph where L::Target: Logger { let chan_info = ChannelInfo { features: msg.features.clone(), - node_one, + node_one: msg.node_id_1, one_to_two: None, - node_two, + node_two: msg.node_id_2, two_to_one: None, capacity_sats: utxo_value, announcement_message: if msg.excess_data.len() <= MAX_EXCESS_BYTES_FOR_RELAY @@ -1987,10 +2000,10 @@ mod tests { features: channelmanager::provided_channel_features(&UserConfig::default()), chain_hash: genesis_block(Network::Testnet).header.block_hash(), short_channel_id: 0, - node_id_1, - node_id_2, - bitcoin_key_1: PublicKey::from_secret_key(&secp_ctx, node_1_btckey), - bitcoin_key_2: PublicKey::from_secret_key(&secp_ctx, node_2_btckey), + node_id_1: NodeId::from_pubkey(&node_id_1), + node_id_2: NodeId::from_pubkey(&node_id_2), + bitcoin_key_1: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, node_1_btckey)), + bitcoin_key_2: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, node_2_btckey)), excess_data: Vec::new(), }; f(&mut unsigned_announcement); diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs index 5cc9f2965..f16dd2d5b 100644 --- a/lightning/src/routing/scoring.rs +++ b/lightning/src/routing/scoring.rs @@ -1779,10 +1779,10 @@ mod tests { features: channelmanager::provided_channel_features(&UserConfig::default()), chain_hash: genesis_hash, short_channel_id, - node_id_1: PublicKey::from_secret_key(&secp_ctx, &node_1_key), - node_id_2: PublicKey::from_secret_key(&secp_ctx, &node_2_key), - bitcoin_key_1: PublicKey::from_secret_key(&secp_ctx, &node_1_secret), - bitcoin_key_2: PublicKey::from_secret_key(&secp_ctx, &node_2_secret), + 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)), + 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()[..])[..]); diff --git a/lightning/src/routing/test_utils.rs b/lightning/src/routing/test_utils.rs index a5d2af43c..8666a65ec 100644 --- a/lightning/src/routing/test_utils.rs +++ b/lightning/src/routing/test_utils.rs @@ -27,13 +27,15 @@ use bitcoin::secp256k1::{Secp256k1, All}; use crate::prelude::*; use crate::sync::{self, Arc}; +use crate::routing::gossip::NodeId; + // Using the same keys for LN and BTC ids pub(super) fn add_channel( gossip_sync: &P2PGossipSync>>, Arc, Arc>, secp_ctx: &Secp256k1, node_1_privkey: &SecretKey, node_2_privkey: &SecretKey, features: ChannelFeatures, short_channel_id: u64 ) { - let node_id_1 = PublicKey::from_secret_key(&secp_ctx, node_1_privkey); - let node_id_2 = PublicKey::from_secret_key(&secp_ctx, node_2_privkey); + 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 unsigned_announcement = UnsignedChannelAnnouncement { features, diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index f9a2eafb8..6dd775ea3 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -23,6 +23,7 @@ use crate::ln::{msgs, wire}; use crate::ln::msgs::LightningError; use crate::ln::script::ShutdownScript; use crate::routing::gossip::NetworkGraph; +use crate::routing::gossip::NodeId; use crate::routing::router::{find_route, InFlightHtlcs, Route, RouteHop, RouteParameters, Router, ScorerAccountingForInFlightHtlcs}; use crate::routing::scoring::FixedPenaltyScorer; use crate::util::config::UserConfig; @@ -435,10 +436,10 @@ fn get_dummy_channel_announcement(short_chan_id: u64) -> msgs::ChannelAnnounceme features: ChannelFeatures::empty(), chain_hash: genesis_block(network).header.block_hash(), short_channel_id: short_chan_id, - node_id_1: PublicKey::from_secret_key(&secp_ctx, &node_1_privkey), - node_id_2: PublicKey::from_secret_key(&secp_ctx, &node_2_privkey), - bitcoin_key_1: PublicKey::from_secret_key(&secp_ctx, &node_1_btckey), - bitcoin_key_2: PublicKey::from_secret_key(&secp_ctx, &node_2_btckey), + node_id_1: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_1_privkey)), + node_id_2: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_2_privkey)), + bitcoin_key_1: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_1_btckey)), + bitcoin_key_2: NodeId::from_pubkey(&PublicKey::from_secret_key(&secp_ctx, &node_2_btckey)), excess_data: Vec::new(), };