Fix block connection ordering in a number of functional tests

Many functional tests rely on being able to call block_connected
arbitrarily, jumping back in time to confirm a transaction at a
specific height. Instead, this takes us one step towards having a
well-formed blockchain in the functional tests.

We also take this opportunity to reduce the number of blocks
connected during tests, requiring a number of constant tweaks in
various functional tests.

Co-authored-by: Valentine Wallace <[email protected]>
Co-authored-by: Matt Corallo <[email protected]>

Author: TheBlueMatt

lightning-persister/src/lib.rs

@@ -242,7 +242,7 @@ mod tests {
 		assert_eq!(node_txn.len(), 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);
+		connect_block(&nodes[1], &Block { header, txdata: vec![node_txn[0].clone(), node_txn[0].clone()]}, CHAN_CONFIRM_DEPTH + 1);
 		check_closed_broadcast!(nodes[1], false);
 		check_added_monitors!(nodes[1], 1);
 

lightning/src/ln/channelmanager.rs

@@ -520,7 +520,7 @@ pub(crate) const MAX_LOCAL_BREAKDOWN_TIMEOUT: u16 = 2 * 6 * 24 * 7;
 /// ie the node we forwarded the payment on to should always have enough room to reliably time out
 /// the HTLC via a full update_fail_htlc/commitment_signed dance before we hit the
 /// CLTV_CLAIM_BUFFER point (we static assert that it's at least 3 blocks more).
-const CLTV_EXPIRY_DELTA: u16 = 6 * 12; //TODO?
+const CLTV_EXPIRY_DELTA: u16 = 6 * 6; //TODO?
 pub(super) const CLTV_FAR_FAR_AWAY: u32 = 6 * 24 * 7; //TODO?
 
 // Check that our CLTV_EXPIRY is at least CLTV_CLAIM_BUFFER + ANTI_REORG_DELAY + LATENCY_GRACE_PERIOD_BLOCKS,

lightning/src/ln/functional_test_utils.rs

@@ -44,29 +44,47 @@ 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, node.best_block_info().1, false, Default::default());
+}
+/// 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: node.best_block_hash(), 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 i in height..conf_height {
+		connect_block(node, &block, i);
 		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, conf_height);
 }
 
 pub fn connect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, depth: u32, height: u32, parent: bool, prev_blockhash: BlockHash) -> 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: if parent { prev_blockhash } else { node.best_block_hash() }, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 },
 		txdata: vec![],
 	};
 	connect_block(node, &block, height + 1);
@@ -93,6 +111,21 @@ pub fn disconnect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, header: &Blo
 	node.node.block_disconnected(header);
 	node.blocks.borrow_mut().pop();
 }
+pub fn disconnect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, count: u32) {
+	assert!(node.blocks.borrow_mut().len() as u32 > count); // Cannot disconnect genesis
+	for _ in 0..count {
+		let (block_header, height) = {
+			let blocks = node.blocks.borrow_mut();
+			(blocks[blocks.len() - 1].0, blocks[blocks.len() - 1].1)
+		};
+		disconnect_block(&node, &block_header, height);
+	}
+}
+
+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 {
 	pub tx_broadcaster: test_utils::TestBroadcaster,
@@ -426,8 +459,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_conf.best_block_info().1, false, Default::default());
 	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()));
 }
 
@@ -452,8 +486,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, node_a.best_block_info().1, false, Default::default());
 	create_chan_between_nodes_with_value_confirm_second(node_b, node_a)
 }
 
@@ -853,13 +889,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!($expected_payment_hash, *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!($expected_error_code, error_code.unwrap(), "unexpected error code");
+					assert_eq!($expected_error_data, &error_data.as_ref().unwrap()[..], "unexpected error data");
 				)*
 			},
 			_ => panic!("Unexpected event"),
@@ -1030,7 +1066,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;
@@ -1206,7 +1242,8 @@ pub fn create_network<'a, 'b: 'a, 'c: 'b>(node_count: usize, cfgs: &'b Vec<NodeC
 	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)]