From 33b745f54e7d3ed7b34c40f27e9b2945f50b5d87 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Mon, 25 Sep 2023 16:53:42 -0700 Subject: [PATCH 1/5] Use correct to_remote script in counterparty commitments While our commitment transactions did use the correct `to_remote` script, the `ChannelMonitor`'s was not as it is tracked separately. This would lead to users never receiving an `Event::SpendableOutputs` with a `StaticPaymentOutput` descriptor to claim the funds. Luckily, any users affected which had channel closures confirmed by a counterparty commitment just need to replay the closing transaction to receive the event. --- lightning/src/chain/channelmonitor.rs | 11 ++++++----- lightning/src/ln/chan_utils.rs | 12 +++++++++++- lightning/src/ln/monitor_tests.rs | 10 +++++++--- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 7088d32d160..b535e4b6cad 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -22,12 +22,11 @@ use bitcoin::blockdata::block::BlockHeader; use bitcoin::blockdata::transaction::{OutPoint as BitcoinOutPoint, TxOut, Transaction}; -use bitcoin::blockdata::script::{Script, Builder}; -use bitcoin::blockdata::opcodes; +use bitcoin::blockdata::script::Script; use bitcoin::hashes::Hash; use bitcoin::hashes::sha256::Hash as Sha256; -use bitcoin::hash_types::{Txid, BlockHash, WPubkeyHash}; +use bitcoin::hash_types::{Txid, BlockHash}; use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature}; use bitcoin::secp256k1::{SecretKey, PublicKey}; @@ -1141,8 +1140,9 @@ impl ChannelMonitor { best_block: BestBlock, counterparty_node_id: PublicKey) -> ChannelMonitor { assert!(commitment_transaction_number_obscure_factor <= (1 << 48)); - let payment_key_hash = WPubkeyHash::hash(&keys.pubkeys().payment_point.serialize()); - let counterparty_payment_script = Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&payment_key_hash[..]).into_script(); + let counterparty_payment_script = chan_utils::get_counterparty_payment_script( + &channel_parameters.channel_type_features, &keys.pubkeys().payment_point + ); let counterparty_channel_parameters = channel_parameters.counterparty_parameters.as_ref().unwrap(); let counterparty_delayed_payment_base_key = counterparty_channel_parameters.pubkeys.delayed_payment_basepoint; @@ -2263,6 +2263,7 @@ macro_rules! fail_unbroadcast_htlcs { #[cfg(test)] pub fn deliberately_bogus_accepted_htlc_witness_program() -> Vec { + use bitcoin::blockdata::opcodes; let mut ret = [opcodes::all::OP_NOP.to_u8(); 136]; ret[131] = opcodes::all::OP_DROP.to_u8(); ret[132] = opcodes::all::OP_DROP.to_u8(); diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 7521da01f97..6e683b2fb9e 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -19,7 +19,7 @@ use bitcoin::util::address::Payload; use bitcoin::hashes::{Hash, HashEngine}; use bitcoin::hashes::sha256::Hash as Sha256; use bitcoin::hashes::ripemd160::Hash as Ripemd160; -use bitcoin::hash_types::{Txid, PubkeyHash}; +use bitcoin::hash_types::{Txid, PubkeyHash, WPubkeyHash}; use crate::chain::chaininterface::fee_for_weight; use crate::chain::package::WEIGHT_REVOKED_OUTPUT; @@ -556,6 +556,16 @@ pub fn get_revokeable_redeemscript(revocation_key: &PublicKey, contest_delay: u1 res } +/// Returns the script for the counterparty's output on a holder's commitment transaction based on +/// the channel type. +pub fn get_counterparty_payment_script(channel_type_features: &ChannelTypeFeatures, payment_key: &PublicKey) -> Script { + if channel_type_features.supports_anchors_zero_fee_htlc_tx() { + get_to_countersignatory_with_anchors_redeemscript(payment_key).to_v0_p2wsh() + } else { + Script::new_v0_p2wpkh(&WPubkeyHash::hash(&payment_key.serialize())) + } +} + /// Information about an HTLC as it appears in a commitment transaction #[derive(Clone, Debug, PartialEq, Eq)] pub struct HTLCOutputInCommitment { diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index f0cc872db0a..3a300bf0757 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -2287,8 +2287,8 @@ fn test_anchors_aggregated_revoked_htlc_tx() { assert!(nodes[1].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty()); let spendable_output_events = nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events(); - assert_eq!(spendable_output_events.len(), 2); - for event in spendable_output_events.iter() { + assert_eq!(spendable_output_events.len(), 4); + for event in spendable_output_events { if let Event::SpendableOutputs { outputs, channel_id } = event { assert_eq!(outputs.len(), 1); assert!(vec![chan_b.2, chan_a.2].contains(&channel_id.unwrap())); @@ -2296,7 +2296,11 @@ fn test_anchors_aggregated_revoked_htlc_tx() { &[&outputs[0]], Vec::new(), Script::new_op_return(&[]), 253, None, &Secp256k1::new(), ).unwrap(); - check_spends!(spend_tx, revoked_claim_transactions.get(&spend_tx.input[0].previous_output.txid).unwrap()); + if let SpendableOutputDescriptor::StaticPaymentOutput(_) = &outputs[0] { + check_spends!(spend_tx, &revoked_commitment_a, &revoked_commitment_b); + } else { + check_spends!(spend_tx, revoked_claim_transactions.get(&spend_tx.input[0].previous_output.txid).unwrap()); + } } else { panic!("unexpected event"); } From fa2a2efef47b5da48ac7a239f3d8a835a7f28164 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Mon, 25 Sep 2023 16:55:22 -0700 Subject: [PATCH 2/5] Support signing to_remote anchors variant for StaticPaymentOutput `to_remote` outputs on commitment transactions with anchor outputs have an additional `1 CSV` constraint on its spending condition, transitioning away from the previous P2WPKH script to a P2WSH. Since our `ChannelMonitor` was never updated to track the proper `to_remote` script on anchor outputs channels, we also missed updating our signer to handle the new script changes. --- lightning/src/chain/channelmonitor.rs | 1 + lightning/src/ln/chan_utils.rs | 6 +- lightning/src/sign/mod.rs | 81 +++++++++++++++++++++------ 3 files changed, 69 insertions(+), 19 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index b535e4b6cad..95f68ba22f6 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -4043,6 +4043,7 @@ impl ChannelMonitorImpl { output: outp.clone(), channel_keys_id: self.channel_keys_id, channel_value_satoshis: self.channel_value_satoshis, + channel_transaction_parameters: Some(self.onchain_tx_handler.channel_transaction_parameters.clone()), })); } if self.shutdown_script.as_ref() == Some(&outp.script_pubkey) { diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 6e683b2fb9e..d1489e27168 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -475,7 +475,7 @@ impl_writeable_tlv_based!(TxCreationKeys, { }); /// One counterparty's public keys which do not change over the life of a channel. -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, Hash, PartialEq, Eq)] pub struct ChannelPublicKeys { /// The public key which is used to sign all commitment transactions, as it appears in the /// on-chain channel lock-in 2-of-2 multisig output. @@ -863,7 +863,7 @@ pub fn build_anchor_input_witness(funding_key: &PublicKey, funding_sig: &Signatu /// /// Normally, this is converted to the broadcaster/countersignatory-organized DirectedChannelTransactionParameters /// before use, via the as_holder_broadcastable and as_counterparty_broadcastable functions. -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, Hash, PartialEq, Eq)] pub struct ChannelTransactionParameters { /// Holder public keys pub holder_pubkeys: ChannelPublicKeys, @@ -883,7 +883,7 @@ pub struct ChannelTransactionParameters { } /// Late-bound per-channel counterparty data used to build transactions. -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, Hash, PartialEq, Eq)] pub struct CounterpartyChannelTransactionParameters { /// Counter-party public keys pub pubkeys: ChannelPublicKeys, diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index 5b42796a94f..04652166f05 100644 --- a/lightning/src/sign/mod.rs +++ b/lightning/src/sign/mod.rs @@ -121,20 +121,38 @@ pub struct StaticPaymentOutputDescriptor { pub channel_keys_id: [u8; 32], /// The value of the channel which this transactions spends. pub channel_value_satoshis: u64, + /// The necessary channel parameters that need to be provided to the re-derived signer through + /// [`ChannelSigner::provide_channel_parameters`]. + /// + /// Added as optional, but always `Some` if the descriptor was produced in v0.0.117 or later. + pub channel_transaction_parameters: Option, } impl StaticPaymentOutputDescriptor { /// The maximum length a well-formed witness spending one of these should have. /// Note: If you have the grind_signatures feature enabled, this will be at least 1 byte /// shorter. - // Calculated as 1 byte legnth + 73 byte signature, 1 byte empty vec push, 1 byte length plus - // redeemscript push length. - pub const MAX_WITNESS_LENGTH: usize = 1 + 73 + 34; + pub fn max_witness_length(&self) -> usize { + if self.channel_transaction_parameters.as_ref() + .map(|channel_params| channel_params.channel_type_features.supports_anchors_zero_fee_htlc_tx()) + .unwrap_or(false) + { + let witness_script_weight = 1 /* pubkey push */ + 33 /* pubkey */ + + 1 /* OP_CHECKSIGVERIFY */ + 1 /* OP_1 */ + 1 /* OP_CHECKSEQUENCEVERIFY */; + 1 /* num witness items */ + 1 /* sig push */ + 73 /* sig including sighash flag */ + + 1 /* witness script push */ + witness_script_weight + } else { + // Calculated as 1 byte legnth + 73 byte signature, 1 byte empty vec push, 1 byte length plus + // redeemscript push length. + 1 + 73 + 34 + } + } } impl_writeable_tlv_based!(StaticPaymentOutputDescriptor, { (0, outpoint, required), (2, output, required), (4, channel_keys_id, required), (6, channel_value_satoshis, required), + (7, channel_transaction_parameters, option), }); /// Describes the necessary information to spend a spendable output. @@ -201,15 +219,23 @@ pub enum SpendableOutputDescriptor { /// [`DelayedPaymentOutputDescriptor::to_self_delay`] contained here to /// [`chan_utils::get_revokeable_redeemscript`]. DelayedPaymentOutput(DelayedPaymentOutputDescriptor), - /// An output to a P2WPKH, spendable exclusively by our payment key (i.e., the private key - /// which corresponds to the `payment_point` in [`ChannelSigner::pubkeys`]). The witness - /// in the spending input is, thus, simply: + /// An output spendable exclusively by our payment key (i.e., the private key that corresponds + /// to the `payment_point` in [`ChannelSigner::pubkeys`]). The output type depends on the + /// channel type negotiated. + /// + /// On an anchor outputs channel, the witness in the spending input is: + /// ```bitcoin + /// + /// ``` + /// + /// Otherwise, it is: /// ```bitcoin /// /// ``` /// /// These are generally the result of our counterparty having broadcast the current state, - /// allowing us to claim the non-HTLC-encumbered outputs immediately. + /// allowing us to claim the non-HTLC-encumbered outputs immediately, or after one confirmation + /// in the case of anchor outputs channels. StaticPaymentOutput(StaticPaymentOutputDescriptor), } @@ -280,13 +306,22 @@ impl SpendableOutputDescriptor { match outp { SpendableOutputDescriptor::StaticPaymentOutput(descriptor) => { if !output_set.insert(descriptor.outpoint) { return Err(()); } + let sequence = + if descriptor.channel_transaction_parameters.as_ref() + .map(|channel_params| channel_params.channel_type_features.supports_anchors_zero_fee_htlc_tx()) + .unwrap_or(false) + { + Sequence::from_consensus(1) + } else { + Sequence::ZERO + }; input.push(TxIn { previous_output: descriptor.outpoint.into_bitcoin_outpoint(), script_sig: Script::new(), - sequence: Sequence::ZERO, + sequence, witness: Witness::new(), }); - witness_weight += StaticPaymentOutputDescriptor::MAX_WITNESS_LENGTH; + witness_weight += descriptor.max_witness_length(); #[cfg(feature = "grind_signatures")] { witness_weight -= 1; } // Guarantees a low R signature input_value += descriptor.output.value; @@ -891,18 +926,30 @@ impl InMemorySigner { if !spend_tx.input[input_idx].script_sig.is_empty() { return Err(()); } if spend_tx.input[input_idx].previous_output != descriptor.outpoint.into_bitcoin_outpoint() { return Err(()); } - let remotepubkey = self.pubkeys().payment_point; - let witness_script = bitcoin::Address::p2pkh(&::bitcoin::PublicKey{compressed: true, inner: remotepubkey}, Network::Testnet).script_pubkey(); + let remotepubkey = bitcoin::PublicKey::new(self.pubkeys().payment_point); + let witness_script = if self.channel_type_features().supports_anchors_zero_fee_htlc_tx() { + chan_utils::get_to_countersignatory_with_anchors_redeemscript(&remotepubkey.inner) + } else { + Script::new_p2pkh(&remotepubkey.pubkey_hash()) + }; let sighash = hash_to_message!(&sighash::SighashCache::new(spend_tx).segwit_signature_hash(input_idx, &witness_script, descriptor.output.value, EcdsaSighashType::All).unwrap()[..]); let remotesig = sign_with_aux_rand(secp_ctx, &sighash, &self.payment_key, &self); - let payment_script = bitcoin::Address::p2wpkh(&::bitcoin::PublicKey{compressed: true, inner: remotepubkey}, Network::Bitcoin).unwrap().script_pubkey(); + let payment_script = if self.channel_type_features().supports_anchors_zero_fee_htlc_tx() { + witness_script.to_v0_p2wsh() + } else { + Script::new_v0_p2wpkh(&remotepubkey.wpubkey_hash().unwrap()) + }; if payment_script != descriptor.output.script_pubkey { return Err(()); } let mut witness = Vec::with_capacity(2); witness.push(remotesig.serialize_der().to_vec()); witness[0].push(EcdsaSighashType::All as u8); - witness.push(remotepubkey.serialize().to_vec()); + if self.channel_type_features().supports_anchors_zero_fee_htlc_tx() { + witness.push(witness_script.to_bytes()); + } else { + witness.push(remotepubkey.to_bytes()); + } Ok(witness) } @@ -1353,9 +1400,11 @@ impl KeysManager { SpendableOutputDescriptor::StaticPaymentOutput(descriptor) => { let input_idx = psbt.unsigned_tx.input.iter().position(|i| i.previous_output == descriptor.outpoint.into_bitcoin_outpoint()).ok_or(())?; if keys_cache.is_none() || keys_cache.as_ref().unwrap().1 != descriptor.channel_keys_id { - keys_cache = Some(( - self.derive_channel_keys(descriptor.channel_value_satoshis, &descriptor.channel_keys_id), - descriptor.channel_keys_id)); + let mut signer = self.derive_channel_keys(descriptor.channel_value_satoshis, &descriptor.channel_keys_id); + if let Some(channel_params) = descriptor.channel_transaction_parameters.as_ref() { + signer.provide_channel_parameters(channel_params); + } + keys_cache = Some((signer, descriptor.channel_keys_id)); } let witness = Witness::from_vec(keys_cache.as_ref().unwrap().0.sign_counterparty_payment_input(&psbt.unsigned_tx, input_idx, &descriptor, &secp_ctx)?); psbt.inputs[input_idx].final_script_witness = Some(witness); From 3299d88595d6014dafa6f897a970330ee734e104 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 28 Sep 2023 09:07:10 -0700 Subject: [PATCH 3/5] Fix off-by-one max witness estimate for P2WPKH StaticPaymentDescriptor We were not accounting for the extra byte denoting the number of items in the witness stack. --- lightning/src/events/bump_transaction.rs | 12 +++--------- lightning/src/sign/mod.rs | 10 +++++++--- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs index 35e9da60544..55c12d23e74 100644 --- a/lightning/src/events/bump_transaction.rs +++ b/lightning/src/events/bump_transaction.rs @@ -26,7 +26,7 @@ use crate::ln::chan_utils::{ use crate::ln::features::ChannelTypeFeatures; use crate::ln::PaymentPreimage; use crate::prelude::*; -use crate::sign::{EcdsaChannelSigner, SignerProvider, WriteableEcdsaChannelSigner}; +use crate::sign::{EcdsaChannelSigner, SignerProvider, WriteableEcdsaChannelSigner, P2WPKH_WITNESS_WEIGHT}; use crate::sync::Mutex; use crate::util::logger::Logger; @@ -384,12 +384,6 @@ pub struct Utxo { } impl Utxo { - const P2WPKH_WITNESS_WEIGHT: u64 = 1 /* num stack items */ + - 1 /* sig length */ + - 73 /* sig including sighash flag */ + - 1 /* pubkey length */ + - 33 /* pubkey */; - /// Returns a `Utxo` with the `satisfaction_weight` estimate for a legacy P2PKH output. pub fn new_p2pkh(outpoint: OutPoint, value: u64, pubkey_hash: &PubkeyHash) -> Self { let script_sig_size = 1 /* script_sig length */ + @@ -419,7 +413,7 @@ impl Utxo { value, script_pubkey: Script::new_p2sh(&Script::new_v0_p2wpkh(pubkey_hash).script_hash()), }, - satisfaction_weight: script_sig_size * WITNESS_SCALE_FACTOR as u64 + Self::P2WPKH_WITNESS_WEIGHT, + satisfaction_weight: script_sig_size * WITNESS_SCALE_FACTOR as u64 + P2WPKH_WITNESS_WEIGHT, } } @@ -431,7 +425,7 @@ impl Utxo { value, script_pubkey: Script::new_v0_p2wpkh(pubkey_hash), }, - satisfaction_weight: EMPTY_SCRIPT_SIG_WEIGHT + Self::P2WPKH_WITNESS_WEIGHT, + satisfaction_weight: EMPTY_SCRIPT_SIG_WEIGHT + P2WPKH_WITNESS_WEIGHT, } } } diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index 04652166f05..9a30253049d 100644 --- a/lightning/src/sign/mod.rs +++ b/lightning/src/sign/mod.rs @@ -107,6 +107,12 @@ impl_writeable_tlv_based!(DelayedPaymentOutputDescriptor, { (12, channel_value_satoshis, required), }); +pub(crate) const P2WPKH_WITNESS_WEIGHT: u64 = 1 /* num stack items */ + + 1 /* sig length */ + + 73 /* sig including sighash flag */ + + 1 /* pubkey length */ + + 33 /* pubkey */; + /// Information about a spendable output to our "payment key". /// /// See [`SpendableOutputDescriptor::StaticPaymentOutput`] for more details on how to spend this. @@ -141,9 +147,7 @@ impl StaticPaymentOutputDescriptor { 1 /* num witness items */ + 1 /* sig push */ + 73 /* sig including sighash flag */ + 1 /* witness script push */ + witness_script_weight } else { - // Calculated as 1 byte legnth + 73 byte signature, 1 byte empty vec push, 1 byte length plus - // redeemscript push length. - 1 + 73 + 34 + P2WPKH_WITNESS_WEIGHT as usize } } } From 9f3bb7d7a54ca24e42d358a9f0f78cf8944d3f98 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 28 Sep 2023 12:12:19 -0700 Subject: [PATCH 4/5] Fix incorrect anchors `counterparty_payment_script` upon deserialization --- lightning/src/chain/channelmonitor.rs | 23 ++++++- lightning/src/ln/monitor_tests.rs | 87 +++++++++++++++++++++++++++ lightning/src/util/test_utils.rs | 11 +++- 3 files changed, 119 insertions(+), 2 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 95f68ba22f6..53ac85c2932 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1696,6 +1696,16 @@ impl ChannelMonitor { Vec::new() } } + + #[cfg(test)] + pub fn get_counterparty_payment_script(&self) -> Script{ + self.inner.lock().unwrap().counterparty_payment_script.clone() + } + + #[cfg(test)] + pub fn set_counterparty_payment_script(&self, script: Script) { + self.inner.lock().unwrap().counterparty_payment_script = script; + } } impl ChannelMonitorImpl { @@ -4146,7 +4156,7 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP 1 => { None }, _ => return Err(DecodeError::InvalidValue), }; - let counterparty_payment_script = Readable::read(reader)?; + let mut counterparty_payment_script: Script = Readable::read(reader)?; let shutdown_script = { let script =