Merge pull request #715 from TheBlueMatt/2020-09-no-rescan-tests

Do not test any block-contents-rescan cases

diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs
index 295c337e29d905ff8e6e5b28ffb4043b76429148..f0a4b648ed50c1c888740d862aeb94c597f8d4c8 100644 (file)
--- a/lightning/src/chain/chaininterface.rs
+++ b/lightning/src/chain/chaininterface.rs
@@ -377,10 +377,22 @@ impl ChainWatchInterface for ChainWatchInterfaceUtil {
 
        fn filter_block(&self, block: &Block) -> Vec<usize> {
                let mut matched_index = Vec::new();
+               let mut matched_txids = HashSet::new();
                {
                        let watched = self.watched.lock().unwrap();
                        for (index, transaction) in block.txdata.iter().enumerate() {
-                               if self.does_match_tx_unguarded(transaction, &watched) {
+                               // A tx matches the filter if it either matches the filter directly (via
+                               // does_match_tx_unguarded) or if it is a descendant of another matched
+                               // transaction within the same block, which we check for in the loop.
+                               let mut matched = self.does_match_tx_unguarded(transaction, &watched);
+                               for input in transaction.input.iter() {
+                                       if matched || matched_txids.contains(&input.previous_output.txid) {
+                                               matched = true;
+                                               break;
+                                       }
+                               }
+                               if matched {
+                                       matched_txids.insert(transaction.txid());
                                        matched_index.push(index);
                                }
                        }

diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
index 1cc41b0f8fa6882f498997eb2840f6ac605f9bdc..5599bfbb9fd763e88c58dd475735ace5a86a0079 100644 (file)
--- a/lightning/src/chain/keysinterface.rs
+++ b/lightning/src/chain/keysinterface.rs
@@ -45,7 +45,7 @@ use ln::msgs::DecodeError;
 /// spend on-chain. The information needed to do this is provided in this enum, including the
 /// outpoint describing which txid and output index is available, the full output which exists at
 /// that txid/index, and any keys or other information required to sign.
-#[derive(Clone, PartialEq)]
+#[derive(Clone, Debug, PartialEq)]
 pub enum SpendableOutputDescriptor {
        /// An output to a script which was provided via KeysInterface, thus you should already know
        /// how to spend it. No keys are provided as rust-lightning was never given any keys - only the

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index a94003c484aaa1dacab7b80b9232e32d78ab6331..9a3c4b793612ed6215f420599ccf657d7895f97f 100644 (file)
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -5037,24 +5037,34 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() {
        check_added_monitors!(nodes[1], 1);
 
        let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
-       assert_eq!(node_txn.len(), 4); // ChannelMonitor: justice tx on revoked commitment, justice tx on revoked HTLC-timeout, adjusted justice tx, ChannelManager: local commitment tx
-       assert_eq!(node_txn[0].input.len(), 2);
-       check_spends!(node_txn[0], revoked_local_txn[0]);
-       check_spends!(node_txn[1], chan_1.3);
+       assert_eq!(node_txn.len(), 3); // ChannelMonitor: bogus justice tx, justice tx on revoked outputs, ChannelManager: local commitment tx
+       // The first transaction generated is bogus - it spends both outputs of revoked_local_txn[0]
+       // including the one already spent by revoked_htlc_txn[0]. That's OK, we'll spend with valid
+       // transactions next...
+       assert_eq!(node_txn[0].input.len(), 3);
+       check_spends!(node_txn[0], revoked_local_txn[0], revoked_htlc_txn[0]);
+
+       assert_eq!(node_txn[1].input.len(), 2);
+       check_spends!(node_txn[1], revoked_local_txn[0], revoked_htlc_txn[0]);
+       if node_txn[1].input[1].previous_output.txid == revoked_htlc_txn[0].txid() {
+               assert_ne!(node_txn[1].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output);
+       } else {
+               assert_eq!(node_txn[1].input[0].previous_output.txid, revoked_htlc_txn[0].txid());
+               assert_ne!(node_txn[1].input[1].previous_output, revoked_htlc_txn[0].input[0].previous_output);
+       }
+
        assert_eq!(node_txn[2].input.len(), 1);
-       check_spends!(node_txn[2], revoked_htlc_txn[0]);
-       assert_eq!(node_txn[3].input.len(), 1);
-       check_spends!(node_txn[3], revoked_local_txn[0]);
+       check_spends!(node_txn[2], chan_1.3);
 
        let header_1 = BlockHeader { version: 0x20000000, prev_blockhash: header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
-       nodes[1].block_notifier.block_connected(&Block { header: header_1, txdata: vec![node_txn[0].clone(), node_txn[2].clone()] }, 1);
+       nodes[1].block_notifier.block_connected(&Block { header: header_1, txdata: vec![node_txn[1].clone()] }, 1);
        connect_blocks(&nodes[1].block_notifier, ANTI_REORG_DELAY - 1, 1, true, header.block_hash());
 
        // Check B's ChannelMonitor was able to generate the right spendable output descriptor
        let spend_txn = check_spendable_outputs!(nodes[1], 1, node_cfgs[1].keys_manager, 100000);
-       assert_eq!(spend_txn.len(), 2);
-       check_spends!(spend_txn[0], node_txn[0]);
-       check_spends!(spend_txn[1], node_txn[2]);
+       assert_eq!(spend_txn.len(), 1);
+       assert_eq!(spend_txn[0].input.len(), 1);
+       check_spends!(spend_txn[0], node_txn[1]);
 }
 
 #[test]
@@ -5072,6 +5082,9 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_success_tx() {
        assert_eq!(revoked_local_txn[0].input.len(), 1);
        assert_eq!(revoked_local_txn[0].input[0].previous_output.txid, chan_1.3.txid());
 
+       // The to-be-revoked commitment tx should have one HTLC and one to_remote output
+       assert_eq!(revoked_local_txn[0].output.len(), 2);
+
        claim_payment(&nodes[0], &vec!(&nodes[1])[..], payment_preimage, 3_000_000);
 
        let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
@@ -5086,6 +5099,10 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_success_tx() {
        assert_eq!(revoked_htlc_txn[0].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
        check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]);
 
+       // Check that the unspent (of two) outputs on revoked_local_txn[0] is a P2WPKH:
+       let unspent_local_txn_output = revoked_htlc_txn[0].input[0].previous_output.vout as usize ^ 1;
+       assert_eq!(revoked_local_txn[0].output[unspent_local_txn_output].script_pubkey.len(), 2 + 20); // P2WPKH
+
        // A will generate justice tx from B's revoked commitment/HTLC tx
        nodes[0].block_notifier.block_connected(&Block { header, txdata: vec![revoked_local_txn[0].clone(), revoked_htlc_txn[0].clone()] }, 1);
        check_closed_broadcast!(nodes[0], false);
@@ -5093,21 +5110,39 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_success_tx() {
 
        let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
        assert_eq!(node_txn.len(), 3); // ChannelMonitor: justice tx on revoked commitment, justice tx on revoked HTLC-success, ChannelManager: local commitment tx
-       assert_eq!(node_txn[2].input.len(), 1);
-       check_spends!(node_txn[2], revoked_htlc_txn[0]);
+
+       // The first transaction generated is bogus - it spends both outputs of revoked_local_txn[0]
+       // including the one already spent by revoked_htlc_txn[0]. That's OK, we'll spend with valid
+       // transactions next...
+       assert_eq!(node_txn[0].input.len(), 2);
+       check_spends!(node_txn[0], revoked_local_txn[0], revoked_htlc_txn[0]);
+       if node_txn[0].input[1].previous_output.txid == revoked_htlc_txn[0].txid() {
+               assert_eq!(node_txn[0].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output);
+       } else {
+               assert_eq!(node_txn[0].input[0].previous_output.txid, revoked_htlc_txn[0].txid());
+               assert_eq!(node_txn[0].input[1].previous_output, revoked_htlc_txn[0].input[0].previous_output);
+       }
+
+       assert_eq!(node_txn[1].input.len(), 1);
+       check_spends!(node_txn[1], revoked_htlc_txn[0]);
+
+       check_spends!(node_txn[2], chan_1.3);
 
        let header_1 = BlockHeader { version: 0x20000000, prev_blockhash: header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
-       nodes[0].block_notifier.block_connected(&Block { header: header_1, txdata: vec![node_txn[0].clone(), node_txn[2].clone()] }, 1);
+       nodes[0].block_notifier.block_connected(&Block { header: header_1, txdata: vec![node_txn[1].clone()] }, 1);
        connect_blocks(&nodes[0].block_notifier, ANTI_REORG_DELAY - 1, 1, true, header.block_hash());
 
+       // Note that nodes[0]'s tx_broadcaster is still locked, so if we get here the channelmonitor
+       // didn't try to generate any new transactions.
+
        // Check A's ChannelMonitor was able to generate the right spendable output descriptor
        let spend_txn = check_spendable_outputs!(nodes[0], 1, node_cfgs[0].keys_manager, 100000);
-       assert_eq!(spend_txn.len(), 5); // Duplicated SpendableOutput due to block rescan after revoked htlc output tracking
+       assert_eq!(spend_txn.len(), 3); // Duplicated SpendableOutput due to block rescan after revoked htlc output tracking
        assert_eq!(spend_txn[0], spend_txn[1]);
-       assert_eq!(spend_txn[0], spend_txn[2]);
+       assert_eq!(spend_txn[0].input.len(), 1);
        check_spends!(spend_txn[0], revoked_local_txn[0]); // spending to_remote output from revoked local tx
-       check_spends!(spend_txn[3], node_txn[0]); // spending justice tx output from revoked local tx htlc received output
-       check_spends!(spend_txn[4], node_txn[2]); // spending justice tx output on htlc success tx
+       assert_ne!(spend_txn[0].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output);
+       check_spends!(spend_txn[2], node_txn[1]); // spending justice tx output on the htlc success tx
 }
 
 #[test]
@@ -7757,21 +7792,23 @@ fn test_bump_penalty_txn_on_revoked_htlcs() {
                check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]);
                assert_eq!(revoked_htlc_txn[1].input.len(), 1);
                assert_eq!(revoked_htlc_txn[1].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+               assert_eq!(revoked_htlc_txn[1].output.len(), 1);
                check_spends!(revoked_htlc_txn[1], revoked_local_txn[0]);
        } else if revoked_htlc_txn[1].input[0].witness.last().unwrap().len() == ACCEPTED_HTLC_SCRIPT_WEIGHT {
                assert_eq!(revoked_htlc_txn[1].input.len(), 1);
                check_spends!(revoked_htlc_txn[1], revoked_local_txn[0]);
                assert_eq!(revoked_htlc_txn[0].input.len(), 1);
                assert_eq!(revoked_htlc_txn[0].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+               assert_eq!(revoked_htlc_txn[0].output.len(), 1);
                check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]);
        }
 
        // Broadcast set of revoked txn on A
-       let header_128 = connect_blocks(&nodes[0].block_notifier, 128, 0, true, header.block_hash());
+       let header_128 = BlockHeader { version: 0x20000000, prev_blockhash: header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+       nodes[0].block_notifier.block_connected(&Block { header: header_128, txdata: vec![revoked_local_txn[0].clone()] }, 128);
        expect_pending_htlcs_forwardable_ignore!(nodes[0]);
-
-       let header_129 = BlockHeader { version: 0x20000000, prev_blockhash: header_128, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
-       nodes[0].block_notifier.block_connected(&Block { header: header_129, txdata: vec![revoked_local_txn[0].clone(), revoked_htlc_txn[0].clone(), revoked_htlc_txn[1].clone()] }, 129);
+       let header_129 = BlockHeader { version: 0x20000000, prev_blockhash: header_128.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+       nodes[0].block_notifier.block_connected(&Block { header: header_129, txdata: vec![revoked_htlc_txn[0].clone(), revoked_htlc_txn[1].clone()] }, 129);
        let first;
        let feerate_1;
        let penalty_txn;
@@ -7779,32 +7816,71 @@ fn test_bump_penalty_txn_on_revoked_htlcs() {
                let mut node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
                assert_eq!(node_txn.len(), 5); // 3 penalty txn on revoked commitment tx + A commitment tx + 1 penalty tnx on revoked HTLC txn
                // Verify claim tx are spending revoked HTLC txn
+
+               // node_txn 0-2 each spend a separate revoked output from revoked_local_txn[0]
+               // Note that node_txn[0] and node_txn[1] are bogus - they double spend the revoked_htlc_txn
+               // which are included in the same block (they are broadcasted because we scan the
+               // transactions linearly and generate claims as we go, they likely should be removed in the
+               // future).
+               assert_eq!(node_txn[0].input.len(), 1);
+               check_spends!(node_txn[0], revoked_local_txn[0]);
+               assert_eq!(node_txn[1].input.len(), 1);
+               check_spends!(node_txn[1], revoked_local_txn[0]);
+               assert_eq!(node_txn[2].input.len(), 1);
+               check_spends!(node_txn[2], revoked_local_txn[0]);
+
+               // Each of the three justice transactions claim a separate (single) output of the three
+               // available, which we check here:
+               assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
+               assert_ne!(node_txn[0].input[0].previous_output, node_txn[2].input[0].previous_output);
+               assert_ne!(node_txn[1].input[0].previous_output, node_txn[2].input[0].previous_output);
+
+               assert_eq!(node_txn[0].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output);
+               assert_eq!(node_txn[1].input[0].previous_output, revoked_htlc_txn[1].input[0].previous_output);
+
+               // node_txn[3] is the local commitment tx broadcast just because (and somewhat in case of
+               // reorgs, though its not clear its ever worth broadcasting conflicting txn like this when
+               // a remote commitment tx has already been confirmed).
+               check_spends!(node_txn[3], chan.3);
+
+               // node_txn[4] spends the revoked outputs from the revoked_htlc_txn (which only have one
+               // output, checked above).
                assert_eq!(node_txn[4].input.len(), 2);
                assert_eq!(node_txn[4].output.len(), 1);
                check_spends!(node_txn[4], revoked_htlc_txn[0], revoked_htlc_txn[1]);
+
                first = node_txn[4].txid();
                // Store both feerates for later comparison
                let fee_1 = revoked_htlc_txn[0].output[0].value + revoked_htlc_txn[1].output[0].value - node_txn[4].output[0].value;
                feerate_1 = fee_1 * 1000 / node_txn[4].get_weight() as u64;
-               penalty_txn = vec![node_txn[0].clone(), node_txn[1].clone(), node_txn[2].clone()];
+               penalty_txn = vec![node_txn[2].clone()];
                node_txn.clear();
        }
 
-       // Connect three more block to see if bumped penalty are issued for HTLC txn
+       // Connect one more block to see if bumped penalty are issued for HTLC txn
        let header_130 = BlockHeader { version: 0x20000000, prev_blockhash: header_129.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
        nodes[0].block_notifier.block_connected(&Block { header: header_130, txdata: penalty_txn }, 130);
+       let header_131 = BlockHeader { version: 0x20000000, prev_blockhash: header_130.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+       nodes[0].block_notifier.block_connected(&Block { header: header_131, txdata: Vec::new() }, 131);
        {
                let mut node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
                assert_eq!(node_txn.len(), 2); // 2 bumped penalty txn on revoked commitment tx
 
                check_spends!(node_txn[0], revoked_local_txn[0]);
                check_spends!(node_txn[1], revoked_local_txn[0]);
+               // Note that these are both bogus - they spend outputs already claimed in block 129:
+               if node_txn[0].input[0].previous_output == revoked_htlc_txn[0].input[0].previous_output  {
+                       assert_eq!(node_txn[1].input[0].previous_output, revoked_htlc_txn[1].input[0].previous_output);
+               } else {
+                       assert_eq!(node_txn[0].input[0].previous_output, revoked_htlc_txn[1].input[0].previous_output);
+                       assert_eq!(node_txn[1].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output);
+               }
 
                node_txn.clear();
        };
 
        // Few more blocks to confirm penalty txn
-       let header_135 = connect_blocks(&nodes[0].block_notifier, 5, 130, true, header_130.block_hash());
+       let header_135 = connect_blocks(&nodes[0].block_notifier, 4, 131, true, header_131.block_hash());
        assert!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
        let header_144 = connect_blocks(&nodes[0].block_notifier, 9, 135, true, header_135);
        let node_txn = {

diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs
index 21b535408b07e255e29e8b86c6e3fe42d14c9a05..60191b886d59222bf7b2310e2f80264942cce70c 100644 (file)
--- a/lightning/src/util/events.rs
+++ b/lightning/src/util/events.rs
@@ -31,6 +31,7 @@ use std::time::Duration;
 /// Note that while Writeable and Readable are implemented for Event, you probably shouldn't use
 /// them directly as they don't round-trip exactly (for example FundingGenerationReady is never
 /// written as it makes no sense to respond to it after reconnecting to peers).
+#[derive(Debug)]
 pub enum Event {
        /// Used to indicate that the client should generate a funding transaction with the given
        /// parameters and then call ChannelManager::funding_transaction_generated.