From 3cba654e327c1fe7336d2515c7b07fb023fc52be Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Wed, 26 Feb 2020 18:18:27 -0500 Subject: [PATCH] Watch outputs of revoked HTLC-transactions Bumping of justice txn on revoked HTLC-Success/HTLC-timeout is triggered until our claim is confirmed onchain with at least ANTI_REORG_DELAY_SAFE. Before this patch, we weren't tracking them in check_spend_remote_htlc, leading us to infinite bumps. Fix #411 Small fixes by Matt Corallo --- lightning/src/ln/channelmonitor.rs | 20 +++++++++++--------- lightning/src/ln/functional_test_utils.rs | 6 +++--- lightning/src/ln/functional_tests.rs | 14 +++++++++++--- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index c1452457..514a95d2 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1668,22 +1668,22 @@ impl ChannelMonitor { } /// Attempts to claim a remote HTLC-Success/HTLC-Timeout's outputs using the revocation key - fn check_spend_remote_htlc(&mut self, tx: &Transaction, commitment_number: u64, height: u32) -> Vec { - //TODO: send back new outputs to guarantee pending_claim_request consistency + fn check_spend_remote_htlc(&mut self, tx: &Transaction, commitment_number: u64, height: u32) -> (Vec, Option<(Sha256dHash, Vec)>) { + let htlc_txid = tx.txid(); if tx.input.len() != 1 || tx.output.len() != 1 || tx.input[0].witness.len() != 5 { - return Vec::new() + return (Vec::new(), None) } macro_rules! ignore_error { ( $thing : expr ) => { match $thing { Ok(a) => a, - Err(_) => return Vec::new() + Err(_) => return (Vec::new(), None) } }; } - let secret = if let Some(secret) = self.get_secret(commitment_number) { secret } else { return Vec::new(); }; + let secret = if let Some(secret) = self.get_secret(commitment_number) { secret } else { return (Vec::new(), None); }; let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret)); let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key); let (revocation_pubkey, revocation_key) = match self.key_storage { @@ -1694,16 +1694,15 @@ impl ChannelMonitor { Storage::Watchtower { .. } => { unimplemented!() } }; let delayed_key = match self.their_delayed_payment_base_key { - None => return Vec::new(), + None => return (Vec::new(), None), Some(their_delayed_payment_base_key) => ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &per_commitment_point, &their_delayed_payment_base_key)), }; let redeemscript = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, self.our_to_self_delay, &delayed_key); - let htlc_txid = tx.txid(); //TODO: This is gonna be a performance bottleneck for watchtowers! log_trace!(self, "Remote HTLC broadcast {}:{}", htlc_txid, 0); let witness_data = InputMaterial::Revoked { witness_script: redeemscript, pubkey: Some(revocation_pubkey), key: revocation_key, is_htlc: false, amount: tx.output[0].value }; let claimable_outpoints = vec!(ClaimRequest { absolute_timelock: height + self.our_to_self_delay as u32, aggregable: true, outpoint: BitcoinOutPoint { txid: htlc_txid, vout: 0}, witness_data }); - claimable_outpoints + (claimable_outpoints, Some((htlc_txid, tx.output.clone()))) } fn broadcast_by_local_state(&self, local_tx: &LocalSignedTx, delayed_payment_base_key: &SecretKey) -> (Vec, Vec, Vec) { @@ -2019,8 +2018,11 @@ impl ChannelMonitor { } } else { if let Some(&(commitment_number, _)) = self.remote_commitment_txn_on_chain.get(&prevout.txid) { - let mut new_outpoints = self.check_spend_remote_htlc(&tx, commitment_number, height); + let (mut new_outpoints, new_outputs_option) = self.check_spend_remote_htlc(&tx, commitment_number, height); claimable_outpoints.append(&mut new_outpoints); + if let Some(new_outputs) = new_outputs_option { + watch_outputs.push(new_outputs); + } } } } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index ba918f82..bd8394d1 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1098,9 +1098,9 @@ pub fn test_txn_broadcast<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, chan: &(msgs::Cha /// HTLC transaction. pub fn test_revoked_htlc_claim_txn_broadcast<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, revoked_tx: Transaction, commitment_revoked_tx: Transaction) { let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap(); - // We should issue a 2nd transaction if one htlc is dropped from initial claiming tx - // but sometimes not as feerate is too-low - if node_txn.len() != 1 && node_txn.len() != 2 { assert!(false); } + // We may issue multiple claiming transaction on revoked outputs due to block rescan + // for revoked htlc outputs + if node_txn.len() != 1 && node_txn.len() != 2 && node_txn.len() != 3 { assert!(false); } node_txn.retain(|tx| { if tx.input.len() == 1 && tx.input[0].previous_output.txid == revoked_tx.txid() { check_spends!(tx, revoked_tx); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 38b9471e..bd424409 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2127,8 +2127,10 @@ fn test_justice_tx() { test_txn_broadcast(&nodes[1], &chan_5, None, HTLCType::NONE); nodes[0].block_notifier.block_connected(&Block { header, txdata: vec![revoked_local_txn[0].clone()] }, 1); + // Verify broadcast of revoked HTLC-timeout let node_txn = test_txn_broadcast(&nodes[0], &chan_5, Some(revoked_local_txn[0].clone()), HTLCType::TIMEOUT); header = BlockHeader { version: 0x20000000, prev_blockhash: header.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; + // Broadcast revoked HTLC-timeout on node 1 nodes[1].block_notifier.block_connected(&Block { header, txdata: vec![node_txn[1].clone()] }, 1); test_revoked_htlc_claim_txn_broadcast(&nodes[1], node_txn[1].clone(), revoked_local_txn[0].clone()); } @@ -4238,7 +4240,7 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_success_tx() { // Check A's ChannelMonitor was able to generate the right spendable output descriptor let spend_txn = check_spendable_outputs!(nodes[0], 1); - assert_eq!(spend_txn.len(), 4); + assert_eq!(spend_txn.len(), 5); // Duplicated SpendableOutput due to block rescan after revoked htlc output tracking assert_eq!(spend_txn[0], spend_txn[2]); check_spends!(spend_txn[0], revoked_local_txn[0]); // spending to_remote output from revoked local tx check_spends!(spend_txn[1], node_txn[0]); // spending justice tx output from revoked local tx htlc received output @@ -6998,7 +7000,7 @@ fn test_bump_penalty_txn_on_revoked_htlcs() { assert_eq!(node_txn[0].input.len(), 2); check_spends!(node_txn[0], revoked_htlc_txn[0], revoked_htlc_txn[1]); - //// Verify bumped tx is different and 25% bump heuristic + // Verify bumped tx is different and 25% bump heuristic assert_ne!(first, node_txn[0].txid()); let fee_2 = revoked_htlc_txn[0].output[0].value + revoked_htlc_txn[1].output[0].value - node_txn[0].output[0].value; let feerate_2 = fee_2 * 1000 / node_txn[0].get_weight() as u64; @@ -7013,7 +7015,13 @@ fn test_bump_penalty_txn_on_revoked_htlcs() { connect_blocks(&nodes[0].block_notifier, 20, 145, true, header_145.bitcoin_hash()); { let mut node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert_eq!(node_txn.len(), 1); //TODO: fix check_spend_remote_htlc lack of watch output + // We verify than no new transaction has been broadcast because previously + // we were buggy on this exact behavior by not tracking for monitoring remote HTLC outputs (see #411) + // which means we wouldn't see a spend of them by a justice tx and bumped justice tx + // were generated forever instead of safe cleaning after confirmation and ANTI_REORG_SAFE_DELAY blocks. + // Enforce spending of revoked htlc output by claiming transaction remove request as expected and dry + // up bumped justice generation. + assert_eq!(node_txn.len(), 0); node_txn.clear(); } check_closed_broadcast!(nodes[0], false); -- 2.30.2