Don't fail HTLCs in revoked commitment txn until we spend them 2022-05-revoked-txn-edge-cases
authorMatt Corallo <git@bluematt.me>
Sat, 30 Apr 2022 20:30:00 +0000 (20:30 +0000)
committerMatt Corallo <git@bluematt.me>
Tue, 21 Jun 2022 16:14:55 +0000 (16:14 +0000)
When we see a counterparty revoked commitment transaction on-chain
we shouldn't immediately queue up HTLCs present in it for
resolution until we have spent the HTLC outputs in some kind of
claim transaction.

In order to do so, we first have to change the
`fail_unbroadcast_htlcs!()` call to provide it with the HTLCs which
are present in the (revoked) commitment transaction which was
broadcast. However, this is not sufficient - because all of those
HTLCs had their `HTLCSource` removed when the commitment
transaction was revoked, we also have to update
`fail_unbroadcast_htlcs` to check the payment hash and amount when
the `HTLCSource` is `None`.

Somewhat surprisingly, several tests actually explicitly tested for
the old behavior, which required amending to pass with the new
changes.

Finally, this adds a debug assertion when writing `ChannelMonitor`s
to ensure `HTLCSource`s do not leak.

lightning/src/chain/channelmonitor.rs
lightning/src/ln/functional_tests.rs
lightning/src/ln/monitor_tests.rs

index a61e6afbf26e1e0c12fa84bde8b868f6ae5826d4..3f7a1055b6dc3f1dba4d21387051cbe0751293e9 100644 (file)
@@ -869,6 +869,9 @@ impl<Signer: Sign> Writeable for ChannelMonitorImpl<Signer> {
                        writer.write_all(&txid[..])?;
                        writer.write_all(&byte_utils::be64_to_array(htlc_infos.len() as u64))?;
                        for &(ref htlc_output, ref htlc_source) in htlc_infos.iter() {
+                               debug_assert!(htlc_source.is_none() || Some(**txid) == self.current_counterparty_commitment_txid
+                                               || Some(**txid) == self.prev_counterparty_commitment_txid,
+                                       "HTLC Sources for all revoked commitment transactions should be none!");
                                serialize_htlc_in_commitment!(htlc_output);
                                htlc_source.as_ref().map(|b| b.as_ref()).write(writer)?;
                        }
@@ -1676,9 +1679,14 @@ macro_rules! fail_unbroadcast_htlcs {
                                                        // cannot currently change after channel initialization, so we don't
                                                        // need to here.
                                                        let confirmed_htlcs_iter: &mut Iterator<Item = (&HTLCOutputInCommitment, Option<&HTLCSource>)> = &mut $confirmed_htlcs_list;
+
                                                        let mut matched_htlc = false;
                                                        for (ref broadcast_htlc, ref broadcast_source) in confirmed_htlcs_iter {
-                                                               if broadcast_htlc.transaction_output_index.is_some() && Some(&**source) == *broadcast_source {
+                                                               if broadcast_htlc.transaction_output_index.is_some() &&
+                                                                       (Some(&**source) == *broadcast_source ||
+                                                                        (broadcast_source.is_none() &&
+                                                                         broadcast_htlc.payment_hash == htlc.payment_hash &&
+                                                                         broadcast_htlc.amount_msat == htlc.amount_msat)) {
                                                                        matched_htlc = true;
                                                                        break;
                                                                }
@@ -2102,8 +2110,16 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
                                }
                                self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);
 
-                               fail_unbroadcast_htlcs!(self, "revoked counterparty", commitment_txid, height,
-                                       [].iter().map(|a| *a), logger);
+                               if let Some(per_commitment_data) = per_commitment_option {
+                                       fail_unbroadcast_htlcs!(self, "revoked_counterparty", commitment_txid, height,
+                                               per_commitment_data.iter().map(|(htlc, htlc_source)|
+                                                       (htlc, htlc_source.as_ref().map(|htlc_source| htlc_source.as_ref()))
+                                               ), logger);
+                               } else {
+                                       debug_assert!(false, "We should have per-commitment option for any recognized old commitment txn");
+                                       fail_unbroadcast_htlcs!(self, "revoked counterparty", commitment_txid, height,
+                                               [].iter().map(|reference| *reference), logger);
+                               }
                        }
                } else if let Some(per_commitment_data) = per_commitment_option {
                        // While this isn't useful yet, there is a potential race where if a counterparty
index a716d80677c56a313b1e3fffef985a6a0eddfe89..ddab3353e718fe7a7f25dcdc01354205a999eb2d 100644 (file)
@@ -2515,10 +2515,10 @@ fn claim_htlc_outputs_shared_tx() {
        let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
 
        // Rebalance the network to generate htlc in the two directions
-       send_payment(&nodes[0], &vec!(&nodes[1])[..], 8000000);
+       send_payment(&nodes[0], &[&nodes[1]], 8_000_000);
        // node[0] is gonna to revoke an old state thus node[1] should be able to claim both offered/received HTLC outputs on top of commitment tx
-       let payment_preimage_1 = route_payment(&nodes[0], &vec!(&nodes[1])[..], 3000000).0;
-       let (_payment_preimage_2, payment_hash_2, _) = route_payment(&nodes[1], &vec!(&nodes[0])[..], 3000000);
+       let payment_preimage_1 = route_payment(&nodes[0], &[&nodes[1]], 3_000_000).0;
+       let (_payment_preimage_2, payment_hash_2, _) = route_payment(&nodes[1], &[&nodes[0]], 3_000_000);
 
        // Get the will-be-revoked local txn from node[0]
        let revoked_local_txn = get_local_commitment_txn!(nodes[0], chan_1.2);
@@ -2541,9 +2541,9 @@ fn claim_htlc_outputs_shared_tx() {
                check_added_monitors!(nodes[1], 1);
                check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed);
                connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
-               expect_payment_failed!(nodes[1], payment_hash_2, true);
+               assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 
-               let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
+               let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
                assert_eq!(node_txn.len(), 2); // ChannelMonitor: penalty tx, ChannelManager: local commitment
 
                assert_eq!(node_txn[0].input.len(), 3); // Claim the revoked output + both revoked HTLC outputs
@@ -2560,7 +2560,13 @@ fn claim_htlc_outputs_shared_tx() {
 
                // Next nodes[1] broadcasts its current local tx state:
                assert_eq!(node_txn[1].input.len(), 1);
-               assert_eq!(node_txn[1].input[0].previous_output.txid, chan_1.3.txid()); //Spending funding tx unique txouput, tx broadcasted by ChannelManager
+               check_spends!(node_txn[1], chan_1.3);
+
+               // Finally, mine the penalty transaction and check that we get an HTLC failure after
+               // ANTI_REORG_DELAY confirmations.
+               mine_transaction(&nodes[1], &node_txn[0]);
+               connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
+               expect_payment_failed!(nodes[1], payment_hash_2, true);
        }
        get_announce_close_broadcast_events(&nodes, 0, 1);
        assert_eq!(nodes[0].node.list_channels().len(), 0);
@@ -2579,11 +2585,11 @@ fn claim_htlc_outputs_single_tx() {
        let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
 
        // Rebalance the network to generate htlc in the two directions
-       send_payment(&nodes[0], &vec!(&nodes[1])[..], 8000000);
+       send_payment(&nodes[0], &[&nodes[1]], 8_000_000);
        // node[0] is gonna to revoke an old state thus node[1] should be able to claim both offered/received HTLC outputs on top of commitment tx, but this
        // time as two different claim transactions as we're gonna to timeout htlc with given a high current height
-       let payment_preimage_1 = route_payment(&nodes[0], &vec!(&nodes[1])[..], 3000000).0;
-       let (_payment_preimage_2, payment_hash_2, _payment_secret_2) = route_payment(&nodes[1], &vec!(&nodes[0])[..], 3000000);
+       let payment_preimage_1 = route_payment(&nodes[0], &[&nodes[1]], 3_000_000).0;
+       let (_payment_preimage_2, payment_hash_2, _payment_secret_2) = route_payment(&nodes[1], &[&nodes[0]], 3_000_000);
 
        // Get the will-be-revoked local txn from node[0]
        let revoked_local_txn = get_local_commitment_txn!(nodes[0], chan_1.2);
@@ -2605,9 +2611,9 @@ fn claim_htlc_outputs_single_tx() {
                }
 
                connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
-               expect_payment_failed!(nodes[1], payment_hash_2, true);
+               assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 
-               let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
+               let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
                assert!(node_txn.len() == 9 || node_txn.len() == 10);
 
                // Check the pair local commitment and HTLC-timeout broadcast due to HTLC expiration
@@ -2635,6 +2641,14 @@ fn claim_htlc_outputs_single_tx() {
                assert_eq!(*witness_lens.iter().skip(0).next().unwrap(), 77); // revoked to_local
                assert_eq!(*witness_lens.iter().skip(1).next().unwrap(), OFFERED_HTLC_SCRIPT_WEIGHT); // revoked offered HTLC
                assert_eq!(*witness_lens.iter().skip(2).next().unwrap(), ACCEPTED_HTLC_SCRIPT_WEIGHT); // revoked received HTLC
+
+               // Finally, mine the penalty transactions and check that we get an HTLC failure after
+               // ANTI_REORG_DELAY confirmations.
+               mine_transaction(&nodes[1], &node_txn[2]);
+               mine_transaction(&nodes[1], &node_txn[3]);
+               mine_transaction(&nodes[1], &node_txn[4]);
+               connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
+               expect_payment_failed!(nodes[1], payment_hash_2, true);
        }
        get_announce_close_broadcast_events(&nodes, 0, 1);
        assert_eq!(nodes[0].node.list_channels().len(), 0);
@@ -7282,37 +7296,25 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) {
                check_added_monitors!(nodes[0], 1);
                check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed);
                assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0);
+
                connect_blocks(&nodes[0], TEST_FINAL_CLTV - 1); // Confirm blocks until the HTLC expires
-               timeout_tx.push(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap()[1].clone());
+               timeout_tx = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().drain(..)
+                       .filter(|tx| tx.input[0].previous_output.txid == bs_commitment_tx[0].txid()).collect();
+               check_spends!(timeout_tx[0], bs_commitment_tx[0]);
+               // For both a revoked or non-revoked commitment transaction, after ANTI_REORG_DELAY the
+               // dust HTLC should have been failed.
+               expect_payment_failed!(nodes[0], dust_hash, true);
+
                if !revoked {
-                       expect_payment_failed!(nodes[0], dust_hash, true);
                        assert_eq!(timeout_tx[0].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
-                       // We fail non-dust-HTLC 2 by broadcast of local timeout tx on remote commitment tx
-                       mine_transaction(&nodes[0], &timeout_tx[0]);
-                       assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0);
-                       connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
-                       expect_payment_failed!(nodes[0], non_dust_hash, true);
                } else {
-                       // If revoked, both dust & non-dust HTLCs should have been failed after ANTI_REORG_DELAY confs of revoked
-                       // commitment tx
-                       let events = nodes[0].node.get_and_clear_pending_events();
-                       assert_eq!(events.len(), 2);
-                       let first;
-                       match events[0] {
-                               Event::PaymentPathFailed { payment_hash, .. } => {
-                                       if payment_hash == dust_hash { first = true; }
-                                       else { first = false; }
-                               },
-                               _ => panic!("Unexpected event"),
-                       }
-                       match events[1] {
-                               Event::PaymentPathFailed { payment_hash, .. } => {
-                                       if first { assert_eq!(payment_hash, non_dust_hash); }
-                                       else { assert_eq!(payment_hash, dust_hash); }
-                               },
-                               _ => panic!("Unexpected event"),
-                       }
+                       assert_eq!(timeout_tx[0].lock_time, 0);
                }
+               // We fail non-dust-HTLC 2 by broadcast of local timeout/revocation-claim tx
+               mine_transaction(&nodes[0], &timeout_tx[0]);
+               assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0);
+               connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
+               expect_payment_failed!(nodes[0], non_dust_hash, true);
        }
 }
 
index 492a65537701750f7a97c56ac6cb287da395aac0..7c46f7b6d283a10d7ab4f5b97e45a611c1f2698b 100644 (file)
@@ -94,6 +94,55 @@ fn test_spendable_output<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, spendable_t
        } else { panic!(); }
 }
 
+#[test]
+fn revoked_output_htlc_resolution_timing() {
+       // Tests that HTLCs which were present in a broadcasted remote revoked commitment transaction
+       // are resolved only after a spend of the HTLC output reaches six confirmations. Preivously
+       // they would resolve after the revoked commitment transaction itself reaches six
+       // confirmations.
+       let chanmon_cfgs = create_chanmon_cfgs(2);
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+       let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 500_000_000, InitFeatures::known(), InitFeatures::known());
+
+       let payment_hash_1 = route_payment(&nodes[1], &[&nodes[0]], 1_000_000).1;
+
+       // Get a commitment transaction which contains the HTLC we care about, but which we'll revoke
+       // before forwarding.
+       let revoked_local_txn = get_local_commitment_txn!(nodes[0], chan.2);
+       assert_eq!(revoked_local_txn.len(), 1);
+
+       // Route a dust payment to revoke the above commitment transaction
+       route_payment(&nodes[0], &[&nodes[1]], 1_000);
+
+       // Confirm the revoked commitment transaction, closing the channel.
+       mine_transaction(&nodes[1], &revoked_local_txn[0]);
+       check_added_monitors!(nodes[1], 1);
+       check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed);
+       check_closed_broadcast!(nodes[1], true);
+
+       let bs_spend_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
+       assert_eq!(bs_spend_txn.len(), 2);
+       check_spends!(bs_spend_txn[0], revoked_local_txn[0]);
+       check_spends!(bs_spend_txn[1], chan.3);
+
+       // After the commitment transaction confirms, we should still wait on the HTLC spend
+       // transaction to confirm before resolving the HTLC.
+       connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
+       assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+       assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
+
+       // Spend the HTLC output, generating a HTLC failure event after ANTI_REORG_DELAY confirmations.
+       mine_transaction(&nodes[1], &bs_spend_txn[0]);
+       assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+       assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
+
+       connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1);
+       expect_payment_failed!(nodes[1], payment_hash_1, true);
+}
+
 #[test]
 fn chanmon_claim_value_coop_close() {
        // Tests `get_claimable_balances` returns the correct values across a simple cooperative claim.