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

Do not test any block-contents-rescan cases

Author: TheBlueMatt

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);
 				}
 			}

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

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,28 +5099,50 @@ 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);
 	check_added_monitors!(nodes[0], 1);
 
 	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,54 +7792,95 @@ 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;
 	{
 		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 = {

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.