Clean up and clarify tx broadcast checks in channelmonitor tests

This effecitlvey reverts the refactors in 383bd90a481bc146b3a3b1d8,
however keeps the actully new test code.

It also writes documentation for the super confusing tx test func
and makes it a bit less permissive.

diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs
index e9f49bfbd2f7fb90a3300e68ddcd9c667ca69e0a..a2b3b242b8aa1e47119afbed5dabeb37beb33f13 100644 (file)
--- a/src/ln/channelmanager.rs
+++ b/src/ln/channelmanager.rs
@@ -3093,32 +3093,41 @@ mod tests {
 
        #[derive(PartialEq)]
        enum HTLCType { NONE, TIMEOUT, SUCCESS }
-       #[derive(PartialEq)]
-       enum PenaltyType { NONE, HTLC }
-       fn test_txn_broadcast(node: &Node, chan: &(msgs::ChannelUpdate, msgs::ChannelUpdate, [u8; 32], Transaction), commitment_tx: Option<Transaction>, revoked_tx: Option<Transaction>, has_htlc_tx: HTLCType, has_penalty_tx: PenaltyType) -> Vec<Transaction> {
+       /// Tests that the given node has broadcast transactions for the given Channel
+       ///
+       /// First checks that the latest local commitment tx has been broadcast, unless an explicit
+       /// commitment_tx is provided, which may be used to test that a remote commitment tx was
+       /// broadcast and the revoked outputs were claimed.
+       ///
+       /// Next tests that there is (or is not) a transaction that spends the commitment transaction
+       /// that appears to be the type of HTLC transaction specified in has_htlc_tx.
+       ///
+       /// All broadcast transactions must be accounted for in one of the above three types of we'll
+       /// also fail.
+       fn test_txn_broadcast(node: &Node, chan: &(msgs::ChannelUpdate, msgs::ChannelUpdate, [u8; 32], Transaction), commitment_tx: Option<Transaction>, has_htlc_tx: HTLCType) -> Vec<Transaction> {
                let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap();
-               assert!(node_txn.len() >= if has_htlc_tx == HTLCType::NONE { 0 } else { 1 } + if has_penalty_tx == PenaltyType::NONE { 0 } else { 1 });
+               assert!(node_txn.len() >= if commitment_tx.is_some() { 0 } else { 1 } + if has_htlc_tx == HTLCType::NONE { 0 } else { 1 });
 
                let mut res = Vec::with_capacity(2);
-
-               if let Some(explicit_tx) = commitment_tx {
-                       res.push(explicit_tx.clone());
-               } else {
-                       for tx in node_txn.iter() {
-                               if tx.input.len() == 1 && tx.input[0].previous_output.txid == chan.3.txid() {
-                                       let mut funding_tx_map = HashMap::new();
-                                       funding_tx_map.insert(chan.3.txid(), chan.3.clone());
-                                       tx.verify(&funding_tx_map).unwrap();
+               node_txn.retain(|tx| {
+                       if tx.input.len() == 1 && tx.input[0].previous_output.txid == chan.3.txid() {
+                               let mut funding_tx_map = HashMap::new();
+                               funding_tx_map.insert(chan.3.txid(), chan.3.clone());
+                               tx.verify(&funding_tx_map).unwrap();
+                               if commitment_tx.is_none() {
                                        res.push(tx.clone());
                                }
-                       }
-               }
-               if !revoked_tx.is_some() && !(has_penalty_tx == PenaltyType::HTLC) {
-                       assert_eq!(res.len(), 1);
+                               false
+                       } else { true }
+               });
+               if let Some(explicit_tx) = commitment_tx {
+                       res.push(explicit_tx.clone());
                }
 
+               assert_eq!(res.len(), 1);
+
                if has_htlc_tx != HTLCType::NONE {
-                       for tx in node_txn.iter() {
+                       node_txn.retain(|tx| {
                                if tx.input.len() == 1 && tx.input[0].previous_output.txid == res[0].txid() {
                                        let mut funding_tx_map = HashMap::new();
                                        funding_tx_map.insert(res[0].txid(), res[0].clone());
@@ -3129,29 +3138,32 @@ mod tests {
                                                assert!(tx.lock_time == 0);
                                        }
                                        res.push(tx.clone());
-                                       break;
-                               }
-                       }
+                                       false
+                               } else { true }
+                       });
                        assert_eq!(res.len(), 2);
                }
 
-               if has_penalty_tx == PenaltyType::HTLC {
-                       let revoked_tx = revoked_tx.unwrap();
-                       for tx in node_txn.iter() {
-                               if tx.input.len() == 1 && tx.input[0].previous_output.txid == revoked_tx.txid() {
-                                       let mut funding_tx_map = HashMap::new();
-                                       funding_tx_map.insert(revoked_tx.txid(), revoked_tx.clone());
-                                       tx.verify(&funding_tx_map).unwrap();
-                                       res.push(tx.clone());
-                                       break;
-                               }
-                       }
-                       assert_eq!(res.len(), 1);
-               }
-               node_txn.clear();
+               assert!(node_txn.is_empty());
                res
        }
 
+       /// Tests that the given node has broadcast a claim transaction against the provided revoked
+       /// HTLC transaction.
+       fn test_revoked_htlc_claim_txn_broadcast(node: &Node, revoked_tx: Transaction) {
+               let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap();
+               assert_eq!(node_txn.len(), 1);
+               node_txn.retain(|tx| {
+                       if tx.input.len() == 1 && tx.input[0].previous_output.txid == revoked_tx.txid() {
+                               let mut funding_tx_map = HashMap::new();
+                               funding_tx_map.insert(revoked_tx.txid(), revoked_tx.clone());
+                               tx.verify(&funding_tx_map).unwrap();
+                               false
+                       } else { true }
+               });
+               assert!(node_txn.is_empty());
+       }
+
        fn check_preimage_claim(node: &Node, prev_txn: &Vec<Transaction>) -> Vec<Transaction> {
                let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap();
 
@@ -3225,10 +3237,10 @@ mod tests {
                // Simple case with no pending HTLCs:
                nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), true);
                {
-                       let mut node_txn = test_txn_broadcast(&nodes[1], &chan_1, None, None, HTLCType::NONE, PenaltyType::NONE);
+                       let mut node_txn = test_txn_broadcast(&nodes[1], &chan_1, None, HTLCType::NONE);
                        let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
                        nodes[0].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![node_txn.drain(..).next().unwrap()] }, 1);
-                       test_txn_broadcast(&nodes[0], &chan_1, None, None, HTLCType::NONE, PenaltyType::NONE);
+                       test_txn_broadcast(&nodes[0], &chan_1, None, HTLCType::NONE);
                }
                get_announce_close_broadcast_events(&nodes, 0, 1);
                assert_eq!(nodes[0].node.list_channels().len(), 0);
@@ -3240,10 +3252,10 @@ mod tests {
                // Simple case of one pending HTLC to HTLC-Timeout
                nodes[1].node.peer_disconnected(&nodes[2].node.get_our_node_id(), true);
                {
-                       let mut node_txn = test_txn_broadcast(&nodes[1], &chan_2, None, None, HTLCType::TIMEOUT, PenaltyType::NONE);
+                       let mut node_txn = test_txn_broadcast(&nodes[1], &chan_2, None, HTLCType::TIMEOUT);
                        let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
                        nodes[2].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![node_txn.drain(..).next().unwrap()] }, 1);
-                       test_txn_broadcast(&nodes[2], &chan_2, None, None, HTLCType::NONE, PenaltyType::NONE);
+                       test_txn_broadcast(&nodes[2], &chan_2, None, HTLCType::NONE);
                }
                get_announce_close_broadcast_events(&nodes, 1, 2);
                assert_eq!(nodes[1].node.list_channels().len(), 0);
@@ -3277,7 +3289,7 @@ mod tests {
                // HTLC-Timeout and a nodes[3] claim against it (+ its own announces)
                nodes[2].node.peer_disconnected(&nodes[3].node.get_our_node_id(), true);
                {
-                       let node_txn = test_txn_broadcast(&nodes[2], &chan_3, None, None, HTLCType::TIMEOUT, PenaltyType::NONE);
+                       let node_txn = test_txn_broadcast(&nodes[2], &chan_3, None, HTLCType::TIMEOUT);
 
                        // Claim the payment on nodes[3], giving it knowledge of the preimage
                        claim_funds!(nodes[3], nodes[2], payment_preimage_1);
@@ -3302,7 +3314,7 @@ mod tests {
                                nodes[3].chain_monitor.block_connected_checked(&header, i, &Vec::new()[..], &[0; 0]);
                        }
 
-                       let node_txn = test_txn_broadcast(&nodes[3], &chan_4, None, None, HTLCType::TIMEOUT, PenaltyType::NONE);
+                       let node_txn = test_txn_broadcast(&nodes[3], &chan_4, None, HTLCType::TIMEOUT);
 
                        // Claim the payment on nodes[4], giving it knowledge of the preimage
                        claim_funds!(nodes[4], nodes[3], payment_preimage_2);
@@ -3314,7 +3326,7 @@ mod tests {
                                nodes[4].chain_monitor.block_connected_checked(&header, i, &Vec::new()[..], &[0; 0]);
                        }
 
-                       test_txn_broadcast(&nodes[4], &chan_4, None, None, HTLCType::SUCCESS, PenaltyType::NONE);
+                       test_txn_broadcast(&nodes[4], &chan_4, None, HTLCType::SUCCESS);
 
                        header = BlockHeader { version: 0x20000000, prev_blockhash: header.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
                        nodes[4].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![node_txn[0].clone()] }, TEST_FINAL_CLTV - 5);
@@ -3349,13 +3361,13 @@ mod tests {
                                node_txn[0].verify(&funding_tx_map).unwrap();
                                node_txn.swap_remove(0);
                        }
-                       test_txn_broadcast(&nodes[1], &chan_5, None, None, HTLCType::NONE, PenaltyType::NONE);
+                       test_txn_broadcast(&nodes[1], &chan_5, None, HTLCType::NONE);
 
                        nodes[0].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![revoked_local_txn[0].clone()] }, 1);
-                       let node_txn = test_txn_broadcast(&nodes[0], &chan_5, Some(revoked_local_txn[0].clone()), None, HTLCType::TIMEOUT, PenaltyType::NONE);
+                       let node_txn = test_txn_broadcast(&nodes[0], &chan_5, Some(revoked_local_txn[0].clone()), HTLCType::TIMEOUT);
                        header = BlockHeader { version: 0x20000000, prev_blockhash: header.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
                        nodes[1].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![node_txn[1].clone()] }, 1);
-                       test_txn_broadcast(&nodes[1], &chan_5, None, Some(node_txn[1].clone()), HTLCType::NONE, PenaltyType::HTLC);
+                       test_revoked_htlc_claim_txn_broadcast(&nodes[1], node_txn[1].clone());
                }
                get_announce_close_broadcast_events(&nodes, 0, 1);
                assert_eq!(nodes[0].node.list_channels().len(), 0);