From b2c5e3aedb00c9431ad9514a8200e773eec0ebf3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 5 Mar 2021 11:02:42 -0500 Subject: [PATCH] Add assertions for in-order block [dis]connection in ChannelManager 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 Co-authored-by: Jeffrey Czyz --- fuzz/src/chanmon_consistency.rs | 3 ++- fuzz/src/full_stack.rs | 14 +++++++------- lightning-persister/src/lib.rs | 4 ++-- lightning/src/ln/channelmanager.rs | 12 ++++++++++++ lightning/src/ln/functional_test_utils.rs | 14 +++++++++++++- lightning/src/ln/reorg_tests.rs | 20 +++++++++++--------- 6 files changed, 47 insertions(+), 20 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 3e205c071..7c5875ac1 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -432,7 +432,8 @@ pub fn do_test(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 { diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index a6672a745..bffb3e8e2 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -161,7 +161,7 @@ struct MoneyLossDetector<'a> { peers: &'a RefCell<[bool; 256]>, funding_txn: Vec, txids_confirmed: HashMap, - header_hashes: Vec, + 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; diff --git a/lightning-persister/src/lib.rs b/lightning-persister/src/lib.rs index 3db95f35a..264715c01 100644 --- a/lightning-persister/src/lib.rs +++ b/lightning-persister/src/lib.rs @@ -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); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index bb9dbaba7..997570ea3 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3302,6 +3302,16 @@ impl 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 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; diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 0716fef9f..58d5fd355 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -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>, pub network_chan_count: Rc>, pub logger: &'c test_utils::TestLogger, + pub blocks: RefCell>, +} +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