Use sign_holder_htlc_transaction to sign non-anchors holder HTLCs
authorWilmer Paulino <wilmer@wilmerpaulino.com>
Fri, 13 Oct 2023 20:52:23 +0000 (13:52 -0700)
committerWilmer Paulino <wilmer@wilmerpaulino.com>
Fri, 20 Oct 2023 22:32:13 +0000 (15:32 -0700)
We want to ensure we use fresh random signatures to prevent certain
classes of transaction replacement attacks at the bitcoin P2P layer.
This was already covered for commitment transactions and zero fee holder
HTLC transactions, but was missing for holder HTLC transactions on
non-anchors channels.

We can easily do this by reusing the existing
`EcdsaChannelSigner::sign_holder_htlc_transaction` method and
circumventing the existing `holder_htlc_sigs/prev_holder_htlc_sigs`
caches, which will be removed in a later commit anyway.

lightning/src/chain/onchaintx.rs
lightning/src/ln/chan_utils.rs
lightning/src/ln/monitor_tests.rs

index 857b6b9a6b0801cc20f625097d66731f45159ed8..b26d4d8724f89bba1470ad8b400c70902f0e8f32 100644 (file)
@@ -23,6 +23,7 @@ use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature};
 use bitcoin::secp256k1;
 
 use crate::chain::chaininterface::compute_feerate_sat_per_1000_weight;
+use crate::events::bump_transaction::{ChannelDerivationParameters, HTLCDescriptor};
 use crate::sign::{ChannelSigner, EntropySource, SignerProvider};
 use crate::ln::msgs::DecodeError;
 use crate::ln::PaymentPreimage;
@@ -1157,35 +1158,43 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
        }
 
        pub(crate) fn get_fully_signed_htlc_tx(&mut self, outp: &::bitcoin::OutPoint, preimage: &Option<PaymentPreimage>) -> Option<Transaction> {
-               let mut htlc_tx = None;
-               let commitment_txid = self.holder_commitment.trust().txid();
-               // Check if the HTLC spends from the current holder commitment
-               if commitment_txid == outp.txid {
-                       self.sign_latest_holder_htlcs();
-                       if let &Some(ref htlc_sigs) = &self.holder_htlc_sigs {
-                               let &(ref htlc_idx, ref htlc_sig) = htlc_sigs[outp.vout as usize].as_ref().unwrap();
-                               let trusted_tx = self.holder_commitment.trust();
-                               let counterparty_htlc_sig = self.holder_commitment.counterparty_htlc_sigs[*htlc_idx];
-                               htlc_tx = Some(trusted_tx
-                                       .get_signed_htlc_tx(&self.channel_transaction_parameters.as_holder_broadcastable(), *htlc_idx, &counterparty_htlc_sig, htlc_sig, preimage));
-                       }
-               }
-               // If the HTLC doesn't spend the current holder commitment, check if it spends the previous one
-               if htlc_tx.is_none() && self.prev_holder_commitment.is_some() {
-                       let commitment_txid = self.prev_holder_commitment.as_ref().unwrap().trust().txid();
-                       if commitment_txid == outp.txid {
-                               self.sign_prev_holder_htlcs();
-                               if let &Some(ref htlc_sigs) = &self.prev_holder_htlc_sigs {
-                                       let &(ref htlc_idx, ref htlc_sig) = htlc_sigs[outp.vout as usize].as_ref().unwrap();
-                                       let holder_commitment = self.prev_holder_commitment.as_ref().unwrap();
-                                       let trusted_tx = holder_commitment.trust();
-                                       let counterparty_htlc_sig = holder_commitment.counterparty_htlc_sigs[*htlc_idx];
-                                       htlc_tx = Some(trusted_tx
-                                               .get_signed_htlc_tx(&self.channel_transaction_parameters.as_holder_broadcastable(), *htlc_idx, &counterparty_htlc_sig, htlc_sig, preimage));
-                               }
+               let get_signed_htlc_tx = |holder_commitment: &HolderCommitmentTransaction| {
+                       let trusted_tx = holder_commitment.trust();
+                       if trusted_tx.txid() != outp.txid {
+                               return None;
                        }
-               }
-               htlc_tx
+                       let (htlc_idx, htlc) = trusted_tx.htlcs().iter().enumerate()
+                               .find(|(_, htlc)| htlc.transaction_output_index.unwrap() == outp.vout)
+                               .unwrap();
+                       let counterparty_htlc_sig = holder_commitment.counterparty_htlc_sigs[htlc_idx];
+                       let mut htlc_tx = trusted_tx.build_unsigned_htlc_tx(
+                               &self.channel_transaction_parameters.as_holder_broadcastable(), htlc_idx, preimage,
+                       );
+
+                       let htlc_descriptor = HTLCDescriptor {
+                               channel_derivation_parameters: ChannelDerivationParameters {
+                                       value_satoshis: self.channel_value_satoshis,
+                                       keys_id: self.channel_keys_id,
+                                       transaction_parameters: self.channel_transaction_parameters.clone(),
+                               },
+                               commitment_txid: trusted_tx.txid(),
+                               per_commitment_number: trusted_tx.commitment_number(),
+                               per_commitment_point: trusted_tx.per_commitment_point(),
+                               feerate_per_kw: trusted_tx.feerate_per_kw(),
+                               htlc: htlc.clone(),
+                               preimage: preimage.clone(),
+                               counterparty_sig: counterparty_htlc_sig.clone(),
+                       };
+                       let htlc_sig = self.signer.sign_holder_htlc_transaction(&htlc_tx, 0, &htlc_descriptor, &self.secp_ctx).unwrap();
+                       htlc_tx.input[0].witness = trusted_tx.build_htlc_input_witness(
+                               htlc_idx, &counterparty_htlc_sig, &htlc_sig, preimage,
+                       );
+                       Some(htlc_tx)
+               };
+
+               // Check if the HTLC spends from the current holder commitment first, or the previous.
+               get_signed_htlc_tx(&self.holder_commitment)
+                       .or_else(|| self.prev_holder_commitment.as_ref().and_then(|prev_holder_commitment| get_signed_htlc_tx(prev_holder_commitment)))
        }
 
        pub(crate) fn generate_external_htlc_claim(
index d1489e2716836f928d5bdc04450f3d8722d4fbc9..1c068025b8f0853340dfc54e89ba347411cdf20a 100644 (file)
@@ -1599,6 +1599,11 @@ impl CommitmentTransaction {
                self.commitment_number
        }
 
+       /// The per commitment point used by the broadcaster.
+       pub fn per_commitment_point(&self) -> PublicKey {
+               self.keys.per_commitment_point
+       }
+
        /// The value to be sent to the broadcaster
        pub fn to_broadcaster_value_sat(&self) -> u64 {
                self.to_broadcaster_value_sat
@@ -1720,26 +1725,40 @@ impl<'a> TrustedCommitmentTransaction<'a> {
                Ok(ret)
        }
 
-       /// Gets a signed HTLC transaction given a preimage (for !htlc.offered) and the holder HTLC transaction signature.
-       pub(crate) fn get_signed_htlc_tx(&self, channel_parameters: &DirectedChannelTransactionParameters, htlc_index: usize, counterparty_signature: &Signature, signature: &Signature, preimage: &Option<PaymentPreimage>) -> Transaction {
-               let inner = self.inner;
-               let keys = &inner.keys;
-               let txid = inner.built.txid;
-               let this_htlc = &inner.htlcs[htlc_index];
+       /// Builds the second-level holder HTLC transaction for the HTLC with index `htlc_index`.
+       pub(crate) fn build_unsigned_htlc_tx(
+               &self, channel_parameters: &DirectedChannelTransactionParameters, htlc_index: usize,
+               preimage: &Option<PaymentPreimage>,
+       ) -> Transaction {
+               let keys = &self.inner.keys;
+               let this_htlc = &self.inner.htlcs[htlc_index];
                assert!(this_htlc.transaction_output_index.is_some());
                // if we don't have preimage for an HTLC-Success, we can't generate an HTLC transaction.
                if !this_htlc.offered && preimage.is_none() { unreachable!(); }
                // Further, we should never be provided the preimage for an HTLC-Timeout transaction.
                if  this_htlc.offered && preimage.is_some() { unreachable!(); }
 
-               let mut htlc_tx = build_htlc_transaction(&txid, inner.feerate_per_kw, channel_parameters.contest_delay(), &this_htlc, &self.channel_type_features, &keys.broadcaster_delayed_payment_key, &keys.revocation_key);
+               build_htlc_transaction(
+                       &self.inner.built.txid, self.inner.feerate_per_kw, channel_parameters.contest_delay(), &this_htlc,
+                       &self.channel_type_features, &keys.broadcaster_delayed_payment_key, &keys.revocation_key
+               )
+       }
 
-               let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc, &self.channel_type_features, &keys.broadcaster_htlc_key, &keys.countersignatory_htlc_key, &keys.revocation_key);
 
-               htlc_tx.input[0].witness = chan_utils::build_htlc_input_witness(
-                       signature, counterparty_signature, preimage, &htlc_redeemscript, &self.channel_type_features,
+       /// Builds the witness required to spend the input for the HTLC with index `htlc_index` in a
+       /// second-level holder HTLC transaction.
+       pub(crate) fn build_htlc_input_witness(
+               &self, htlc_index: usize, counterparty_signature: &Signature, signature: &Signature,
+               preimage: &Option<PaymentPreimage>
+       ) -> Witness {
+               let keys = &self.inner.keys;
+               let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(
+                       &self.inner.htlcs[htlc_index], &self.channel_type_features, &keys.broadcaster_htlc_key,
+                       &keys.countersignatory_htlc_key, &keys.revocation_key
                );
-               htlc_tx
+               chan_utils::build_htlc_input_witness(
+                       signature, counterparty_signature, preimage, &htlc_redeemscript, &self.channel_type_features,
+               )
        }
 
        /// Returns the index of the revokeable output, i.e. the `to_local` output sending funds to
index 0399ed38251ba7bc155af80323add37b86c9eef2..5be498e918b1227a153ca7d9a44d90db1e651cc9 100644 (file)
@@ -2715,3 +2715,113 @@ fn test_anchors_monitor_fixes_counterparty_payment_script_on_reload() {
        do_test_anchors_monitor_fixes_counterparty_payment_script_on_reload(false);
        do_test_anchors_monitor_fixes_counterparty_payment_script_on_reload(true);
 }
+
+#[cfg(not(feature = "_test_vectors"))]
+fn do_test_monitor_claims_with_random_signatures(anchors: bool, confirm_counterparty_commitment: bool) {
+       // Tests that our monitor claims will always use fresh random signatures (ensuring a unique
+       // wtxid) to prevent certain classes of transaction replacement at the bitcoin P2P layer.
+       let chanmon_cfgs = create_chanmon_cfgs(2);
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let mut user_config = test_default_channel_config();
+       if anchors {
+               user_config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
+               user_config.manually_accept_inbound_channels = true;
+       }
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(user_config), Some(user_config)]);
+       let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+       let coinbase_tx = Transaction {
+               version: 2,
+               lock_time: PackedLockTime::ZERO,
+               input: vec![TxIn { ..Default::default() }],
+               output: vec![
+                       TxOut {
+                               value: Amount::ONE_BTC.to_sat(),
+                               script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(),
+                       },
+               ],
+       };
+       if anchors {
+               nodes[0].wallet_source.add_utxo(bitcoin::OutPoint { txid: coinbase_tx.txid(), vout: 0 }, coinbase_tx.output[0].value);
+       }
+
+       // Open a channel and route a payment. We'll let it timeout to claim it.
+       let (_, _, chan_id, funding_tx) = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
+       route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
+
+       let (closing_node, other_node) = if confirm_counterparty_commitment {
+               (&nodes[1], &nodes[0])
+       } else {
+               (&nodes[0], &nodes[1])
+       };
+
+       closing_node.node.force_close_broadcasting_latest_txn(&chan_id, &other_node.node.get_our_node_id()).unwrap();
+
+       // The commitment transaction comes first.
+       let commitment_tx = {
+               let mut txn = closing_node.tx_broadcaster.unique_txn_broadcast();
+               assert_eq!(txn.len(), 1);
+               check_spends!(txn[0], funding_tx);
+               txn.pop().unwrap()
+       };
+
+       mine_transaction(closing_node, &commitment_tx);
+       check_added_monitors!(closing_node, 1);
+       check_closed_broadcast!(closing_node, true);
+       check_closed_event!(closing_node, 1, ClosureReason::HolderForceClosed, [other_node.node.get_our_node_id()], 1_000_000);
+
+       mine_transaction(other_node, &commitment_tx);
+       check_added_monitors!(other_node, 1);
+       check_closed_broadcast!(other_node, true);
+       check_closed_event!(other_node, 1, ClosureReason::CommitmentTxConfirmed, [closing_node.node.get_our_node_id()], 1_000_000);
+
+       // If we update the best block to the new height before providing the confirmed transactions,
+       // we'll see another broadcast of the commitment transaction.
+       if anchors && !confirm_counterparty_commitment && nodes[0].connect_style.borrow().updates_best_block_first() {
+               let _ = nodes[0].tx_broadcaster.txn_broadcast();
+       }
+
+       // Then comes the HTLC timeout transaction.
+       if confirm_counterparty_commitment {
+               connect_blocks(&nodes[0], 5);
+               test_spendable_output(&nodes[0], &commitment_tx, false);
+               connect_blocks(&nodes[0], TEST_FINAL_CLTV - 5);
+       } else {
+               connect_blocks(&nodes[0], TEST_FINAL_CLTV);
+       }
+       if anchors && !confirm_counterparty_commitment {
+               handle_bump_htlc_event(&nodes[0], 1);
+       }
+       let htlc_timeout_tx = {
+               let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
+               assert_eq!(txn.len(), 1);
+               let tx = if txn[0].input[0].previous_output.txid == commitment_tx.txid() {
+                       txn[0].clone()
+               } else {
+                       txn[1].clone()
+               };
+               check_spends!(tx, commitment_tx, coinbase_tx);
+               tx
+       };
+
+       // Check we rebroadcast it with a different wtxid.
+       nodes[0].chain_monitor.chain_monitor.rebroadcast_pending_claims();
+       if anchors && !confirm_counterparty_commitment {
+               handle_bump_htlc_event(&nodes[0], 1);
+       }
+       {
+               let mut txn = nodes[0].tx_broadcaster.txn_broadcast();
+               assert_eq!(txn.len(), 1);
+               assert_eq!(txn[0].txid(), htlc_timeout_tx.txid());
+               assert_ne!(txn[0].wtxid(), htlc_timeout_tx.wtxid());
+       }
+}
+
+#[cfg(not(feature = "_test_vectors"))]
+#[test]
+fn test_monitor_claims_with_random_signatures() {
+       do_test_monitor_claims_with_random_signatures(false, false);
+       do_test_monitor_claims_with_random_signatures(false, true);
+       do_test_monitor_claims_with_random_signatures(true, false);
+       do_test_monitor_claims_with_random_signatures(true, true);
+}