Require syntactically-valid blockchains in functional and unit tests

fuzz/src/chanmon_consistency.rs

@@ -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 {

fuzz/src/full_stack.rs

@@ -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;

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()]});
 		check_closed_broadcast!(nodes[1], false);
 		check_added_monitors!(nodes[1], 1);
 

lightning/src/chain/channelmonitor.rs

@@ -1501,8 +1501,13 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 					self.lockdown_from_offchain = true;
 					if *should_broadcast {
 						self.broadcast_latest_holder_commitment_txn(broadcaster, logger);
-					} else {
+					} else if !self.holder_tx_signed {
 						log_error!(logger, "You have a toxic holder commitment transaction avaible in channel monitor, read comment in ChannelMonitor::get_latest_holder_commitment_txn to be informed of manual action to take");
+					} else {
+						// If we generated a MonitorEvent::CommitmentTxBroadcasted, the ChannelManager
+						// will still give us a ChannelForceClosed event with !should_broadcast, but we
+						// shouldn't print the scary warning above.
+						log_info!(logger, "Channel off-chain state closed after we broadcasted our latest commitment transaction.");
 					}
 				}
 			}

lightning/src/ln/channelmanager.rs

@@ -3302,6 +3302,10 @@ 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);
 
+		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 +3422,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;
 

lightning/src/ln/functional_test_utils.rs

@@ -44,53 +44,81 @@ use std::sync::Mutex;
 use std::mem;
 use std::collections::HashMap;
 
-pub const CHAN_CONFIRM_DEPTH: u32 = 100;
+pub const CHAN_CONFIRM_DEPTH: u32 = 10;
 
+/// Mine the given transaction in the next block and then mine CHAN_CONFIRM_DEPTH - 1 blocks on
+/// top, giving the given transaction CHAN_CONFIRM_DEPTH confirmations.
 pub fn confirm_transaction<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, tx: &Transaction) {
-	let dummy_tx = Transaction { version: 0, lock_time: 0, input: Vec::new(), output: Vec::new() };
-	let dummy_tx_count = tx.version as usize;
+	confirm_transaction_at(node, tx, node.best_block_info().1 + 1);
+	connect_blocks(node, CHAN_CONFIRM_DEPTH - 1);
+}
+/// Mine a signle block containing the given transaction
+pub fn mine_transaction<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, tx: &Transaction) {
+	let height = node.best_block_info().1 + 1;
+	confirm_transaction_at(node, tx, height);
+}
+/// Mine the given transaction at the given height, mining blocks as required to build to that
+/// height
+pub fn confirm_transaction_at<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, tx: &Transaction, conf_height: u32) {
+	let starting_block = node.best_block_info();
 	let mut block = Block {
-		header: BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 },
-		txdata: vec![dummy_tx; dummy_tx_count],
+		header: BlockHeader { version: 0x20000000, prev_blockhash: starting_block.0, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 },
+		txdata: Vec::new(),
 	};
-	block.txdata.push(tx.clone());
-	connect_block(node, &block, 1);
-	for i in 2..CHAN_CONFIRM_DEPTH {
+	let height = starting_block.1 + 1;
+	assert!(height <= conf_height);
+	for _ in height..conf_height {
+		connect_block(node, &block);
 		block = Block {
 			header: BlockHeader { version: 0x20000000, prev_blockhash: block.header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 },
 			txdata: vec![],
 		};
-		connect_block(node, &block, i);
 	}
+
+	for _ in 0..*node.network_chan_count.borrow() { // Make sure we don't end up with channels at the same short id by offsetting by chan_count
+		block.txdata.push(Transaction { version: 0, lock_time: 0, input: Vec::new(), output: Vec::new() });
+	}
+	block.txdata.push(tx.clone());
+	connect_block(node, &block);
 }
 
-pub fn connect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, depth: u32, height: u32, parent: bool, prev_blockhash: BlockHash) -> BlockHash {
+pub fn connect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, depth: u32) -> BlockHash {
 	let mut block = Block {
-		header: BlockHeader { version: 0x2000000, prev_blockhash: if parent { prev_blockhash } else { Default::default() }, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 },
+		header: BlockHeader { version: 0x2000000, prev_blockhash: node.best_block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 },
 		txdata: vec![],
 	};
-	connect_block(node, &block, height + 1);
-	for i in 2..depth + 1 {
+	connect_block(node, &block);
+	for _ in 2..depth + 1 {
 		block = Block {
 			header: BlockHeader { version: 0x20000000, prev_blockhash: block.header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 },
 			txdata: vec![],
 		};
-		connect_block(node, &block, height + i);
+		connect_block(node, &block);
 	}
 	block.header.block_hash()
 }
 
-pub fn connect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, block: &Block, height: u32) {
+pub fn connect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, block: &Block) {
 	let txdata: Vec<_> = block.txdata.iter().enumerate().collect();
+	let height = node.best_block_info().1 + 1;
 	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();
+pub fn disconnect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, count: u32) {
+	for _ in 0..count {
+		let orig_header = node.blocks.borrow_mut().pop().unwrap();
+		assert!(orig_header.1 > 0); // Cannot disconnect genesis
+		node.chain_monitor.chain_monitor.block_disconnected(&orig_header.0, orig_header.1);
+		node.node.block_disconnected(&orig_header.0);
+	}
+}
+
+pub fn disconnect_all_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>) {
+	let count = node.blocks.borrow_mut().len() as u32 - 1;
+	disconnect_blocks(node, count);
 }
 
 pub struct TestChanMonCfg {
@@ -123,6 +151,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> {
@@ -416,8 +453,9 @@ pub fn create_chan_between_nodes_with_value_init<'a, 'b, 'c>(node_a: &Node<'a, '
 	tx
 }
 
-pub fn create_chan_between_nodes_with_value_confirm_first<'a, 'b, 'c, 'd>(node_recv: &'a Node<'b, 'c, 'c>, node_conf: &'a Node<'b, 'c, 'd>, tx: &Transaction) {
-	confirm_transaction(node_conf, tx);
+pub fn create_chan_between_nodes_with_value_confirm_first<'a, 'b, 'c, 'd>(node_recv: &'a Node<'b, 'c, 'c>, node_conf: &'a Node<'b, 'c, 'd>, tx: &Transaction, conf_height: u32) {
+	confirm_transaction_at(node_conf, tx, conf_height);
+	connect_blocks(node_conf, CHAN_CONFIRM_DEPTH - 1);
 	node_recv.node.handle_funding_locked(&node_conf.node.get_our_node_id(), &get_event_msg!(node_conf, MessageSendEvent::SendFundingLocked, node_recv.node.get_our_node_id()));
 }
 
@@ -442,8 +480,10 @@ pub fn create_chan_between_nodes_with_value_confirm_second<'a, 'b, 'c>(node_recv
 }
 
 pub fn create_chan_between_nodes_with_value_confirm<'a, 'b, 'c, 'd>(node_a: &'a Node<'b, 'c, 'd>, node_b: &'a Node<'b, 'c, 'd>, tx: &Transaction) -> ((msgs::FundingLocked, msgs::AnnouncementSignatures), [u8; 32]) {
-	create_chan_between_nodes_with_value_confirm_first(node_a, node_b, tx);
-	confirm_transaction(node_a, tx);
+	let conf_height = std::cmp::max(node_a.best_block_info().1 + 1, node_b.best_block_info().1 + 1);
+	create_chan_between_nodes_with_value_confirm_first(node_a, node_b, tx, conf_height);
+	confirm_transaction_at(node_a, tx, conf_height);
+	connect_blocks(node_a, CHAN_CONFIRM_DEPTH - 1);
 	create_chan_between_nodes_with_value_confirm_second(node_b, node_a)
 }
 
@@ -843,13 +883,13 @@ macro_rules! expect_payment_failed {
 		assert_eq!(events.len(), 1);
 		match events[0] {
 			Event::PaymentFailed { ref payment_hash, rejected_by_dest, ref error_code, ref error_data } => {
-				assert_eq!(*payment_hash, $expected_payment_hash);
-				assert_eq!(rejected_by_dest, $rejected_by_dest);
-				assert!(error_code.is_some());
-				assert!(error_data.is_some());
+				assert_eq!(*payment_hash, $expected_payment_hash, "unexpected payment_hash");
+				assert_eq!(rejected_by_dest, $rejected_by_dest, "unexpected rejected_by_dest value");
+				assert!(error_code.is_some(), "expected error_code.is_some() = true");
+				assert!(error_data.is_some(), "expected error_data.is_some() = true");
 				$(
-					assert_eq!(error_code.unwrap(), $expected_error_code);
-					assert_eq!(&error_data.as_ref().unwrap()[..], $expected_error_data);
+					assert_eq!(error_code.unwrap(), $expected_error_code, "unexpected error code");
+					assert_eq!(&error_data.as_ref().unwrap()[..], $expected_error_data, "unexpected error data");
 				)*
 			},
 			_ => panic!("Unexpected event"),
@@ -1020,7 +1060,7 @@ pub fn claim_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route:
 	claim_payment_along_route(origin_node, expected_route, false, our_payment_preimage, expected_amount);
 }
 
-pub const TEST_FINAL_CLTV: u32 = 32;
+pub const TEST_FINAL_CLTV: u32 = 50;
 
 pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) -> (PaymentPreimage, PaymentHash) {
 	let net_graph_msg_handler = &origin_node.net_graph_msg_handler;
@@ -1161,6 +1201,9 @@ pub fn create_node_chanmgrs<'a, 'b>(node_count: usize, cfgs: &'a Vec<NodeCfg<'b>
 	let mut chanmgrs = Vec::new();
 	for i in 0..node_count {
 		let mut default_config = UserConfig::default();
+		// Set cltv_expiry_delta slightly lower to keep the final CLTV values inside one byte in our
+		// tests so that our script-length checks don't fail (see ACCEPTED_HTLC_SCRIPT_WEIGHT).
+		default_config.channel_options.cltv_expiry_delta = 6*6;
 		default_config.channel_options.announced_channel = true;
 		default_config.peer_channel_config_limits.force_announced_channel_preference = false;
 		default_config.own_channel_config.our_htlc_minimum_msat = 1000; // sanitization being done by the sender, to exerce receiver logic we need to lift of limit
@@ -1189,13 +1232,15 @@ 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)])
 		})
 	}
 
 	nodes
 }
 
-pub const ACCEPTED_HTLC_SCRIPT_WEIGHT: usize = 138; //Here we have a diff due to HTLC CLTV expiry being < 2^15 in test
+// Note that the following only works for CLTV values up to 128
+pub const ACCEPTED_HTLC_SCRIPT_WEIGHT: usize = 137; //Here we have a diff due to HTLC CLTV expiry being < 2^15 in test
 pub const OFFERED_HTLC_SCRIPT_WEIGHT: usize = 133;
 
 #[derive(PartialEq)]