From 27ee1150dd39dc2fe72ad7f4b0a040fe510fa0da Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Thu, 8 Oct 2020 20:59:21 -0400 Subject: [PATCH] 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. --- lightning/src/chain/channelmonitor.rs | 25 +++++++++++++ lightning/src/ln/functional_tests.rs | 54 --------------------------- 2 files changed, 25 insertions(+), 54 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index e0f9d607..d8433ef5 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1772,6 +1772,20 @@ impl ChannelMonitor { 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 ChannelMonitor { 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; } } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 29765947..bf18a244 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/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 -- 2.30.2