Add assertions for in-order block [dis]connection in ChannelManager
authorMatt Corallo <git@bluematt.me>
Fri, 5 Mar 2021 16:02:42 +0000 (11:02 -0500)
committerMatt Corallo <git@bluematt.me>
Sat, 20 Mar 2021 03:32:38 +0000 (23:32 -0400)
Sadly the connected-in-order tests have to be skipped in our normal
test suite as many tests violate it. Luckily we can still enforce
it in the tests which run in other crates.

Co-authored-by: Matt Corallo <git@bluematt.me>
Co-authored-by: Jeffrey Czyz <jkczyz@gmail.com>
fuzz/src/chanmon_consistency.rs
fuzz/src/full_stack.rs
lightning-persister/src/lib.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_test_utils.rs
lightning/src/ln/reorg_tests.rs

index 3e205c07183d402fe045110d562f94b084224155..7c5875ac1ef62d31590241228027fd75b954aba2 100644 (file)
@@ -432,7 +432,8 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 
        macro_rules! confirm_txn {
                ($node: expr) => { {
-                       let mut header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+                       let chain_hash = genesis_block(Network::Bitcoin).block_hash();
+                       let mut header = BlockHeader { version: 0x20000000, prev_blockhash: chain_hash, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
                        let txdata: Vec<_> = channel_txn.iter().enumerate().map(|(i, tx)| (i + 1, tx)).collect();
                        $node.block_connected(&header, &txdata, 1);
                        for i in 2..100 {
index a6672a7450b96ba9f51682be3bf885b0fbd1ca50..bffb3e8e214b3a1d6d4d4d75c3e46e4dc6dabfa5 100644 (file)
@@ -161,7 +161,7 @@ struct MoneyLossDetector<'a> {
        peers: &'a RefCell<[bool; 256]>,
        funding_txn: Vec<Transaction>,
        txids_confirmed: HashMap<Txid, usize>,
-       header_hashes: Vec<BlockHash>,
+       header_hashes: Vec<(BlockHash, u32)>,
        height: usize,
        max_height: usize,
        blocks_connected: u32,
@@ -179,7 +179,7 @@ impl<'a> MoneyLossDetector<'a> {
                        peers,
                        funding_txn: Vec::new(),
                        txids_confirmed: HashMap::new(),
-                       header_hashes: vec![Default::default()],
+                       header_hashes: vec![(genesis_block(Network::Bitcoin).block_hash(), 0)],
                        height: 0,
                        max_height: 0,
                        blocks_connected: 0,
@@ -199,23 +199,23 @@ impl<'a> MoneyLossDetector<'a> {
                        }
                }
 
-               let header = BlockHeader { version: 0x20000000, prev_blockhash: self.header_hashes[self.height], merkle_root: Default::default(), time: self.blocks_connected, bits: 42, nonce: 42 };
-               self.height += 1;
                self.blocks_connected += 1;
+               let header = BlockHeader { version: 0x20000000, prev_blockhash: self.header_hashes[self.height].0, merkle_root: Default::default(), time: self.blocks_connected, bits: 42, nonce: 42 };
+               self.height += 1;
                self.manager.block_connected(&header, &txdata, self.height as u32);
                (*self.monitor).block_connected(&header, &txdata, self.height as u32);
                if self.header_hashes.len() > self.height {
-                       self.header_hashes[self.height] = header.block_hash();
+                       self.header_hashes[self.height] = (header.block_hash(), self.blocks_connected);
                } else {
                        assert_eq!(self.header_hashes.len(), self.height);
-                       self.header_hashes.push(header.block_hash());
+                       self.header_hashes.push((header.block_hash(), self.blocks_connected));
                }
                self.max_height = cmp::max(self.height, self.max_height);
        }
 
        fn disconnect_block(&mut self) {
                if self.height > 0 && (self.max_height < 6 || self.height >= self.max_height - 6) {
-                       let header = BlockHeader { version: 0x20000000, prev_blockhash: self.header_hashes[self.height], merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+                       let header = BlockHeader { version: 0x20000000, prev_blockhash: self.header_hashes[self.height - 1].0, merkle_root: Default::default(), time: self.header_hashes[self.height].1, bits: 42, nonce: 42 };
                        self.manager.block_disconnected(&header);
                        self.monitor.block_disconnected(&header, self.height as u32);
                        self.height -= 1;
index 3db95f35a955e3b46113761eae91e26b105c1086..264715c018272c10d6a805ca1b13f550676c3e20 100644 (file)
@@ -247,8 +247,8 @@ mod tests {
                let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
                assert_eq!(node_txn.len(), 1);
 
-               let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
-               connect_block(&nodes[1], &Block { header, txdata: vec![node_txn[0].clone(), node_txn[0].clone()]}, 1);
+               let header = BlockHeader { version: 0x20000000, prev_blockhash: nodes[0].best_block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+               connect_block(&nodes[1], &Block { header, txdata: vec![node_txn[0].clone(), node_txn[0].clone()]}, CHAN_CONFIRM_DEPTH);
                check_closed_broadcast!(nodes[1], false);
                check_added_monitors!(nodes[1], 1);
 
index bb9dbaba774a37ce22c6b1a003cf6acb62a60714..997570ea31a8e27609b629d721b2e7746d08595f 100644 (file)
@@ -3302,6 +3302,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 
                let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 
+
+               // This assertion should be enforced in tests, however we have a number of tests that
+               // were written before this requirement and do not meet it.
+               #[cfg(not(test))]
+               {
+                       assert_eq!(*self.last_block_hash.read().unwrap(), header.prev_blockhash,
+                               "Blocks must be connected in chain-order - the connected header must build on the last connected header");
+                       assert_eq!(self.latest_block_height.load(Ordering::Acquire) as u64, height as u64 - 1,
+                               "Blocks must be connected in chain-order - the connected header must build on the last connected header");
+               }
                self.latest_block_height.store(height as usize, Ordering::Release);
                *self.last_block_hash.write().unwrap() = block_hash;
 
@@ -3418,6 +3428,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                // See the docs for `ChannelManagerReadArgs` for more.
                let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 
+               assert_eq!(*self.last_block_hash.read().unwrap(), header.block_hash(),
+                       "Blocks must be disconnected in chain-order - the disconnected header must be the last connected header");
                self.latest_block_height.fetch_sub(1, Ordering::AcqRel);
                *self.last_block_hash.write().unwrap() = header.prev_blockhash;
 
index 0716fef9ff80a20377c570b9859eb31aaa21a883..58d5fd3554671b078615512b6473bf04fbc3f07d 100644 (file)
@@ -50,7 +50,7 @@ pub fn confirm_transaction<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, tx: &Tran
        let dummy_tx = Transaction { version: 0, lock_time: 0, input: Vec::new(), output: Vec::new() };
        let dummy_tx_count = tx.version as usize;
        let mut block = Block {
-               header: BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 },
+               header: BlockHeader { version: 0x20000000, prev_blockhash: node.best_block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 },
                txdata: vec![dummy_tx; dummy_tx_count],
        };
        block.txdata.push(tx.clone());
@@ -85,12 +85,14 @@ pub fn connect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, block: &Block,
        node.chain_monitor.chain_monitor.block_connected(&block.header, &txdata, height);
        node.node.block_connected(&block.header, &txdata, height);
        node.node.test_process_background_events();
+       node.blocks.borrow_mut().push((block.header, height));
 }
 
 pub fn disconnect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, header: &BlockHeader, height: u32) {
        node.chain_monitor.chain_monitor.block_disconnected(header, height);
        node.node.block_disconnected(header);
        node.node.test_process_background_events();
+       node.blocks.borrow_mut().pop();
 }
 
 pub struct TestChanMonCfg {
@@ -123,6 +125,15 @@ pub struct Node<'a, 'b: 'a, 'c: 'b> {
        pub network_payment_count: Rc<RefCell<u8>>,
        pub network_chan_count: Rc<RefCell<u32>>,
        pub logger: &'c test_utils::TestLogger,
+       pub blocks: RefCell<Vec<(BlockHeader, u32)>>,
+}
+impl<'a, 'b, 'c> Node<'a, 'b, 'c> {
+       pub fn best_block_hash(&self) -> BlockHash {
+               self.blocks.borrow_mut().last().unwrap().0.block_hash()
+       }
+       pub fn best_block_info(&self) -> (BlockHash, u32) {
+               self.blocks.borrow_mut().last().map(|(a, b)| (a.block_hash(), *b)).unwrap()
+       }
 }
 
 impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
@@ -1189,6 +1200,7 @@ pub fn create_network<'a, 'b: 'a, 'c: 'b>(node_count: usize, cfgs: &'b Vec<NodeC
                                 keys_manager: &cfgs[i].keys_manager, node: &chan_mgrs[i], net_graph_msg_handler,
                                 node_seed: cfgs[i].node_seed, network_chan_count: chan_count.clone(),
                                 network_payment_count: payment_count.clone(), logger: cfgs[i].logger,
+                                blocks: RefCell::new(vec![(genesis_block(Network::Testnet).header, 0)])
                })
        }
 
index b19a355d8f1acd6eca7115c79334f4788ee7fed6..7a317f65b9f32b5436be86e182b4ecfc759a454d 100644 (file)
@@ -21,7 +21,9 @@ use util::test_utils;
 use util::ser::{ReadableArgs, Writeable};
 
 use bitcoin::blockdata::block::{Block, BlockHeader};
+use bitcoin::blockdata::constants::genesis_block;
 use bitcoin::hash_types::BlockHash;
+use bitcoin::network::constants::Network;
 
 use std::collections::HashMap;
 use std::mem;
@@ -208,7 +210,7 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool) {
        mem::drop(channel_state);
 
        let mut headers = Vec::new();
-       let mut header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+       let mut header = BlockHeader { version: 0x20000000, prev_blockhash: genesis_block(Network::Testnet).header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
        headers.push(header.clone());
        for _i in 2..100 {
                header = BlockHeader { version: 0x20000000, prev_blockhash: header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
@@ -319,7 +321,7 @@ fn test_set_outpoints_partial_claiming() {
        check_spends!(remote_txn[2], remote_txn[0]);
 
        // Connect blocks on node A to advance height towards TEST_FINAL_CLTV
-       let prev_header_100 = connect_blocks(&nodes[1], 100, 0, false, Default::default());
+       let block_hash_100 = connect_blocks(&nodes[1], 100, 0, false, genesis_block(Network::Testnet).header.block_hash());
        // Provide node A with both preimage
        nodes[0].node.claim_funds(payment_preimage_1, &None, 3_000_000);
        nodes[0].node.claim_funds(payment_preimage_2, &None, 3_000_000);
@@ -328,8 +330,8 @@ fn test_set_outpoints_partial_claiming() {
        nodes[0].node.get_and_clear_pending_msg_events();
 
        // Connect blocks on node A commitment transaction
-       let header = BlockHeader { version: 0x20000000, prev_blockhash: prev_header_100, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
-       connect_block(&nodes[0], &Block { header, txdata: vec![remote_txn[0].clone()] }, 101);
+       let header_101 = BlockHeader { version: 0x20000000, prev_blockhash: block_hash_100, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+       connect_block(&nodes[0], &Block { header: header_101, txdata: vec![remote_txn[0].clone()] }, 101);
        check_closed_broadcast!(nodes[0], false);
        check_added_monitors!(nodes[0], 1);
        // Verify node A broadcast tx claiming both HTLCs
@@ -361,8 +363,8 @@ fn test_set_outpoints_partial_claiming() {
        };
 
        // Broadcast partial claim on node A, should regenerate a claiming tx with HTLC dropped
-       let header = BlockHeader { version: 0x20000000, prev_blockhash: header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
-       connect_block(&nodes[0], &Block { header, txdata: vec![partial_claim_tx.clone()] }, 102);
+       let header_102 = BlockHeader { version: 0x20000000, prev_blockhash: header_101.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+       connect_block(&nodes[0], &Block { header: header_102, txdata: vec![partial_claim_tx.clone()] }, 102);
        {
                let mut node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
                assert_eq!(node_txn.len(), 1);
@@ -373,7 +375,7 @@ fn test_set_outpoints_partial_claiming() {
        nodes[0].node.get_and_clear_pending_msg_events();
 
        // Disconnect last block on node A, should regenerate a claiming tx with HTLC dropped
-       disconnect_block(&nodes[0], &header, 102);
+       disconnect_block(&nodes[0], &header_102, 102);
        {
                let mut node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
                assert_eq!(node_txn.len(), 1);
@@ -383,8 +385,8 @@ fn test_set_outpoints_partial_claiming() {
        }
 
        //// Disconnect one more block and then reconnect multiple no transaction should be generated
-       disconnect_block(&nodes[0], &header, 101);
-       connect_blocks(&nodes[1], 15, 101, false, prev_header_100);
+       disconnect_block(&nodes[0], &header_101, 101);
+       connect_blocks(&nodes[1], 15, 101, false, block_hash_100);
        {
                let mut node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
                assert_eq!(node_txn.len(), 0);