Require syntactically-valid blockchains in functional and unit tests #846

TheBlueMatt · 2021-03-16T23:10:48Z

This is an expansion of #831 and #838 will be rebased on it soon.

codecov · 2021-03-16T23:28:02Z

Codecov Report

Merging #846 (4ebfa1d) into main (80ee1da) will decrease coverage by 0.02%.
The diff coverage is 98.74%.

@@            Coverage Diff             @@
##             main     #846      +/-   ##
==========================================
- Coverage   90.61%   90.59%   -0.03%     
==========================================
  Files          51       51              
  Lines       27140    27109      -31     
==========================================
- Hits        24593    24559      -34     
- Misses       2547     2550       +3

Impacted Files	Coverage Δ
lightning/src/util/events.rs	`23.89% <0.00%> (ø)`
lightning/src/ln/channelmanager.rs	`83.21% <50.00%> (-0.09%)`	⬇️
lightning-persister/src/lib.rs	`95.61% <100.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`95.41% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`95.21% <100.00%> (+0.19%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.83% <100.00%> (-0.03%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`96.77% <100.00%> (-0.08%)`	⬇️
lightning/src/ln/reorg_tests.rs	`99.56% <100.00%> (-0.03%)`	⬇️
lightning/src/chain/chainmonitor.rs	`92.10% <0.00%> (-0.96%)`	⬇️
lightning/src/ln/onchaintx.rs	`86.79% <0.00%> (-0.19%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80ee1da...4ebfa1d. Read the comment docs.

lightning/src/ln/functional_test_utils.rs

jkczyz · 2021-03-17T01:13:09Z

lightning/src/ln/functional_test_utils.rs

+		let (block_header, height) = {
+			let blocks = node.blocks.lock().unwrap();
+			(blocks[blocks.len() - 1].0, blocks[blocks.len() - 1].1)
+		};


Wondering if this could be simplified a bit without the mutex. Is there a need for it?

Oops, indeed. We just need interior mutability, so I swapped it for a refcell.

jkczyz · 2021-03-17T01:16:24Z

lightning/src/ln/functional_test_utils.rs

 }

 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();


Was removing this intentional?

Yes, though it happened int he wrong commit, I moved it to a later commit and since no previous tests relied on this behavior, removing this for test_unconf_chan to use disconnect_block is nice.

Looks like the commit message for d1d1d1f doesn't reference a valid commit?

Oh oops, its the immediately-preceeding "f" commit - it removes this hunk after "Add assertions for in-order block [dis]connection in ChannelManager"

jkczyz · 2021-03-17T01:23:04Z

lightning/src/ln/channelmanager.rs

-const CLTV_EXPIRY_DELTA: u16 = 6 * 12; //TODO?
+const CLTV_EXPIRY_DELTA: u16 = 6 * 6; //TODO?


Won't this affect more than just the tests? If this was intentional, could you make a separate commit explaining the change?

lightning/src/ln/functional_test_utils.rs

lightning/src/ln/functional_tests.rs

TheBlueMatt · 2021-03-17T04:00:55Z

Fixed a few comments and cleaned up the util method interface more, sadly it required reworking the commits so didn't leave fixup commits in between. Will respond to the further comments tomorrow.

valentinewallace · 2021-03-17T19:09:58Z

lightning/src/ln/functional_test_utils.rs

@@ -44,31 +44,50 @@ 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;


This makes test logs much nicer to read 👍

TheBlueMatt · 2021-03-17T21:04:48Z

Rebased on main/#849 and addressed all the comments.

jkczyz

Liking how functional_test_utils is shaping up! Definitely makes the tests easier to read. Left one related comment.

I couldn't quite follow what some of the fixups corresponded to assuming they were going to be squashed with their parent commits. Left a couple comments.

jkczyz · 2021-03-18T00:02:30Z

lightning/src/ln/functional_test_utils.rs

 }

 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();


Looks like the commit message for d1d1d1f doesn't reference a valid commit?

jkczyz · 2021-03-18T00:03:50Z

lightning/src/ln/functional_tests.rs

-	let mut cfg = UserConfig::default();
-	// This test was written when the CLTV_EXPIRY_DELTA was fixed at 12*6, and expects it.
-	cfg.channel_options.cltv_expiry_delta = 12*6;
-	cfg.channel_options.announced_channel = true;
-	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(cfg.clone()), Some(cfg.clone()), Some(cfg.clone())]);
+	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);


Unclear what 444c154 is a fixup for.

It reverts a hunk from #849 - I could probably not revert it, but I'd need to redo the test changes here to figure out how it used to be.

lightning/src/ln/functional_test_utils.rs

TheBlueMatt · 2021-03-18T16:02:14Z

Rebased after merge of 848.

jkczyz

LGTM aside from one minor comment.

lightning/src/ln/functional_test_utils.rs

TheBlueMatt · 2021-03-19T00:38:36Z

Note this depends on #849 which needs to go first.

It can be quite useful in debugging, and potentially also so for users.

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 <[email protected]> Co-authored-by: Jeffrey Czyz <[email protected]>

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]>

This expands the assertions on block ordering to apply to `#[cfg(test)]` builds in addition to normal builds, requiring that unit and functional tests have syntactically-valid (ie the previous block hash pointer and the heights match the blocks) blockchains. This requires a reasonably nontrivial diff in the functional tests however it is mostly straightforward changes.

See comment in the code. This commit exists only to aid reviewers.

See comment in the code, This commit exists only to aid reviewers.

This also reduces some needless clones and indirections.

TheBlueMatt · 2021-03-20T03:45:40Z

Squashed. The diff from val's last ACK rebased onto main (3613425a available on my 2021-03-80fcdd3-rebased branch) is the function argument drops that Jeff suggested. The only diff from the last review by jeff is one function drop and

$ git diff-tree -U1 fb1ecd6  a61289af0b717c70b00ac880cd155933420596b2 lightning/src/ln/functional_test_utils.rs
diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index 2485f3d3..e7f61c6a 100644
--- a/lightning/src/ln/functional_test_utils.rs
+++ b/lightning/src/ln/functional_test_utils.rs
@@ -1203,2 +1203,5 @@ pub fn create_node_chanmgrs<'a, 'b>(node_count: usize, cfgs: &'a Vec<NodeCfg<'b>
 		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;

jkczyz self-requested a review March 17, 2021 00:18

jkczyz reviewed Mar 17, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-03-test-chains branch from 6e5ecba to 80fcdd3 Compare March 17, 2021 04:00

valentinewallace approved these changes Mar 17, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-03-test-chains branch 2 times, most recently from 5eed67e to 48c1cba Compare March 17, 2021 21:04

jkczyz reviewed Mar 18, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-03-test-chains branch 2 times, most recently from e0df57f to 19ed011 Compare March 18, 2021 16:02

jkczyz approved these changes Mar 18, 2021

View reviewed changes

lightning/src/ln/functional_test_utils.rs Outdated Show resolved Hide resolved

TheBlueMatt mentioned this pull request Mar 19, 2021

Make Channel's block connection API more electrum-friendly #838

Merged

TheBlueMatt added 2 commits March 19, 2021 22:57

Clean up log warning in the event ChannelMonitor force-closed

d7402b7

Add Debug to derive list in MessageSendEvent

0ac839d

It can be quite useful in debugging, and potentially also so for users.

TheBlueMatt force-pushed the 2021-03-test-chains branch from fb1ecd6 to a61289a Compare March 20, 2021 02:57

TheBlueMatt and others added 8 commits March 19, 2021 23:32

[test] Demonstrate that the next commit is trivially safe

4266518

See comment in the code. This commit exists only to aid reviewers.

[tests] Drop redundant parameters from connect_blocks

f25a46c

[tests] Demonstrate that the commit is trivially safe

580190f

See comment in the code, This commit exists only to aid reviewers.

Drop height parameter from [dis]connect_block in functional tests

4fc05af

[functional tests] Drop unused disconnect_block utility

4ebfa1d

This also reduces some needless clones and indirections.

TheBlueMatt force-pushed the 2021-03-test-chains branch from a61289a to 4ebfa1d Compare March 20, 2021 03:33

TheBlueMatt merged commit 8a8c75a into lightningdevkit:main Mar 20, 2021

dunxen mentioned this pull request Mar 21, 2021

Derive Debug for MessageSendEvent #796

Closed

		const CLTV_EXPIRY_DELTA: u16 = 6 * 12; //TODO?
		const CLTV_EXPIRY_DELTA: u16 = 6 * 6; //TODO?

Require syntactically-valid blockchains in functional and unit tests #846

Require syntactically-valid blockchains in functional and unit tests #846

Uh oh!

Conversation

TheBlueMatt commented Mar 16, 2021

Uh oh!

codecov bot commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Mar 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Mar 18, 2021

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Mar 19, 2021

Uh oh!

TheBlueMatt commented Mar 20, 2021

Uh oh!

Uh oh!

codecov bot commented Mar 16, 2021 •

edited

Loading

TheBlueMatt commented Mar 17, 2021 •

edited

Loading