Assert on correct registeration of outputs index

We remove test_no_failure_dust_htlc_local_commitment from our test
framework as this test deliberately throwing junk transaction in
our monitoring parsing code is hitting new assertions.

This test was added in #333, but it sounds as an oversight as the
correctness intention of this test (i.e verifying lack of dust
HTLCs canceling back in case of junk commitment transaction) doesn't
currently break.

Author: Antoine Riard

lightning/src/chain/channelmonitor.rs

@@ -1772,6 +1772,20 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			let idx_and_scripts = txouts.iter().map(|o| (o.0, o.1.script_pubkey.clone())).collect();
 			self.outputs_to_watch.insert(txid.clone(), idx_and_scripts).is_none()
 		});
+		#[cfg(test)]
+		{
+		        // If we see a transaction for which we registered outputs previously,
+			// make sure the registered scriptpubkey at the expected index match
+			// the actual transaction output one. We failed this case before #653.
+			for tx in &txn_matched {
+				if let Some(outputs) = self.get_outputs_to_watch().get(&tx.txid()) {
+					for idx_and_script in outputs.iter() {
+						assert!((idx_and_script.0 as usize) < tx.output.len());
+						assert_eq!(tx.output[idx_and_script.0 as usize].script_pubkey, idx_and_script.1);
+					}
+				}
+			}
+		}
 		watch_outputs
 	}
 
@@ -1821,6 +1835,17 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			if let Some(outputs) = self.get_outputs_to_watch().get(&input.previous_output.txid) {
 				for (idx, _script_pubkey) in outputs.iter() {
 					if *idx == input.previous_output.vout {
+						#[cfg(test)]
+						{
+						        // If the expected script is a known type, check that the witness
+						        // appears to be spending the correct type (ie that the match would
+						        // actually succeed in BIP 158/159-style filters).
+						        if _script_pubkey.is_v0_p2wsh() {
+						                assert_eq!(&bitcoin::Address::p2wsh(&Script::from(input.witness.last().unwrap().clone()), bitcoin::Network::Bitcoin).script_pubkey(), _script_pubkey);
+						        } else if _script_pubkey.is_v0_p2wpkh() {
+						                assert_eq!(&bitcoin::Address::p2wpkh(&bitcoin::PublicKey::from_slice(&input.witness.last().unwrap()).unwrap(), bitcoin::Network::Bitcoin).unwrap().script_pubkey(), _script_pubkey);
+						        } else { panic!(); }
+						}
 						return true;
 					}
 				}

lightning/src/ln/functional_tests.rs

@@ -7157,60 +7157,6 @@ fn test_failure_delay_dust_htlc_local_commitment() {
 	do_test_failure_delay_dust_htlc_local_commitment(false);
 }
 
-#[test]
-fn test_no_failure_dust_htlc_local_commitment() {
-	// Transaction filters for failing back dust htlc based on local commitment txn infos has been
-	// prone to error, we test here that a dummy transaction don't fail them.
-
-	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(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
-
-	// Rebalance a bit
-	send_payment(&nodes[0], &vec!(&nodes[1])[..], 8000000, 8_000_000);
-
-	let as_dust_limit = nodes[0].node.channel_state.lock().unwrap().by_id.get(&chan.2).unwrap().holder_dust_limit_satoshis;
-	let bs_dust_limit = nodes[1].node.channel_state.lock().unwrap().by_id.get(&chan.2).unwrap().holder_dust_limit_satoshis;
-
-	// We route 2 dust-HTLCs between A and B
-	let (preimage_1, _) = route_payment(&nodes[0], &[&nodes[1]], bs_dust_limit*1000);
-	let (preimage_2, _) = route_payment(&nodes[1], &[&nodes[0]], as_dust_limit*1000);
-
-	// Build a dummy invalid transaction trying to spend a commitment tx
-	let input = TxIn {
-		previous_output: BitcoinOutPoint { txid: chan.3.txid(), vout: 0 },
-		script_sig: Script::new(),
-		sequence: 0,
-		witness: Vec::new(),
-	};
-
-	let outp = TxOut {
-		script_pubkey: Builder::new().push_opcode(opcodes::all::OP_RETURN).into_script(),
-		value: 10000,
-	};
-
-	let dummy_tx = Transaction {
-		version: 2,
-		lock_time: 0,
-		input: vec![input],
-		output: vec![outp]
-	};
-
-	let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
-	nodes[0].chain_monitor.chain_monitor.block_connected(&header, &[(0, &dummy_tx)], 1);
-	assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0);
-	assert_eq!(nodes[0].node.get_and_clear_pending_msg_events().len(), 0);
-	// We broadcast a few more block to check everything is all right
-	connect_blocks(&nodes[0], 20, 1, true, header.block_hash());
-	assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 0);
-	assert_eq!(nodes[0].node.get_and_clear_pending_msg_events().len(), 0);
-
-	claim_payment(&nodes[0], &vec!(&nodes[1])[..], preimage_1, bs_dust_limit*1000);
-	claim_payment(&nodes[1], &vec!(&nodes[0])[..], preimage_2, as_dust_limit*1000);
-}
-
 fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) {
 	// Outbound HTLC-failure updates must be cancelled if we get a reorg before we reach ANTI_REORG_DELAY.
 	// Broadcast of revoked remote commitment tx, trigger failure-update of dust/non-dust HTLCs