Add reorg test #604

moisesPompilio · 2025-08-08T20:05:35Z

This PR introduces functional tests to validate node behavior under blockchain reorganization scenarios, specifically when opening and closing channels.

Channel open reorg: Ensures the node maintains channel consistency after a reorg occurring immediately after channel creation.
Channel close reorg: Validates both normal close and force close operations when a reorg happens during channel closure, confirming that state remains consistent.

Additionally, the premine_and_distribute_funds helper was optimized. Previously, when many addresses were funded, transactions were sent sequentially, waiting for each to appear in the mempool before sending the next. This was slow. The implementation now uses sendmany from Bitcoin Core to send all outputs in a single transaction, improving speed for all tests relying on this function.

Close #575

ldk-reviews-bot · 2025-08-08T20:05:38Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tnull

Cool, thanks for looking into this! At a high-level this looks pretty much good to me, but I'll need to take a closer look at the test logic still.

I hope the base PR (#602) get's merged soon to fix builds on main. Mind adding a more descriptive commit message in the meantime?

tests/prop_test.rs

moisesPompilio · 2025-08-11T14:22:51Z

Thanks! I was going to make the commit message more descriptive, but got caught up looking into the CI failure. Since PR (#602) fixed it, I forgot to update the message. I initially opened this as a draft to address the issue before marking it ready.

tnull · 2025-08-11T18:27:51Z

This needs a rebase now.

tnull

Thanks! Basically LGTM, just some minor comments/nits.

tnull · 2025-08-13T07:35:24Z

tests/reorg_test.rs

+	macro_rules! reorg {
+		($reorg_depth: expr) => {{
+			invalidate_blocks(bitcoind, $reorg_depth);
+			generate_blocks_and_wait(bitcoind, electrs, $reorg_depth);


I want to note that operating on the same chain and generating/invalidating blocks is only safe/working because proptests are run sequentially as it seems. Fine for this case, but maybe we should inline init_setup_test_reorg to avoid confusion in the future if other tests would try to reuse the same helper?

You’re right. I initially created init_setup_test_reorg when building the test, thinking I’d need many proptest rounds. Since reorgs don’t require that many cases, the performance impact is negligible now, so I’ve removed the helper.

tnull · 2025-08-13T07:36:08Z

tests/reorg_test.rs

+		force_close in prop::bool::ANY,
+	) {
+
+	let (bitcoind, electrsd) = init_setup_test_reorg();


nit: Seems all of this method needs one more indentation to the right. rustfmt didn't catch this as it doesn't operate on macro code.

tnull · 2025-08-13T07:37:13Z

tests/reorg_test.rs

+			assert!(node.list_balances().pending_balances_from_channel_closures.len() > 0);
+			match node.list_balances().pending_balances_from_channel_closures[0] {
+				PendingSweepBalance::BroadcastAwaitingConfirmation { .. } => {},
+				_ => println!("Unexpected balance state!"),


Should we panic here and below rather than just printing that we reached an unexpected state?

Yeah, had to remove it for tests, but added it back now.

tnull · 2025-08-13T07:38:07Z

tests/reorg_test.rs

+
+	// Check balance after close channel
+	nodes.iter().for_each(|node| {
+			assert!(node.list_balances().spendable_onchain_balance_sats > amount - 7000);


Where is this 7000 number coming from? Mind introducing an appropriately named variable for it?

Done! It’s the approximate tx fee to prevent amount check failures.

tnull · 2025-08-13T07:41:00Z

tests/reorg_test.rs

+	}
+
+	let mut node_channels_id = HashMap::new();
+	for index in 0..nodes.len() {


nit: Rather than iterating over the index, can we use iterators? (here and elswhere)

Suggested change

for index in 0..nodes.len() {

for node in nodes.iter() {

They are usually a bit easier to read.

Yeah, I replaced some for index loops with clearer iterator expressions.

tnull

Thanks! One last comment, should otherwise be good to go.

tnull · 2025-08-15T11:35:41Z

tests/reorg_test.rs

+			}};
+		}
+
+		let amount = 2_100_000;


For numerical amounts and fees please always append a _sat/_msat/_btc suffix to make it explicit which unit we're dealing in (here and everywhere else).

- Introduced a property test to verify reorg handling during both channel opening and closing (normal and force close). - Added `invalidate_block` helper to roll back the chain and regenerate blocks to the previous height.

tnull

ACK, will merge if CI passes.

moisesPompilio force-pushed the issue-575 branch 2 times, most recently from a9a00e4 to c28fa3c Compare August 9, 2025 16:13

tnull self-requested a review August 11, 2025 13:10

tnull reviewed Aug 11, 2025

View reviewed changes

tests/prop_test.rs Outdated Show resolved Hide resolved

moisesPompilio force-pushed the issue-575 branch from c28fa3c to 6d9a167 Compare August 11, 2025 14:12

moisesPompilio marked this pull request as ready for review August 11, 2025 14:42

ldk-reviews-bot requested a review from joostjager August 11, 2025 14:42

tnull requested review from tnull and removed request for joostjager August 11, 2025 18:27

moisesPompilio force-pushed the issue-575 branch from 6d9a167 to 78a73de Compare August 11, 2025 18:50

tnull reviewed Aug 13, 2025

View reviewed changes

moisesPompilio force-pushed the issue-575 branch from 78a73de to a4d755f Compare August 15, 2025 02:53

tnull reviewed Aug 15, 2025

View reviewed changes

Add reorg property test and refactor funding logic

e57927a

- Introduced a property test to verify reorg handling during both channel opening and closing (normal and force close). - Added `invalidate_block` helper to roll back the chain and regenerate blocks to the previous height.

moisesPompilio force-pushed the issue-575 branch from a4d755f to e57927a Compare August 15, 2025 13:12

tnull approved these changes Aug 15, 2025

View reviewed changes

tnull merged commit 86cc3d2 into lightningdevkit:main Aug 15, 2025
14 of 15 checks passed

joostjager mentioned this pull request Nov 19, 2025

Add draft changelog for LDK Node v0.7.0 #708

Merged

Add reorg test #604

Add reorg test #604

Uh oh!

Conversation

moisesPompilio commented Aug 8, 2025

Uh oh!

ldk-reviews-bot commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

moisesPompilio commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Aug 11, 2025

Uh oh!

tnull 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!

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!

tnull 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!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ldk-reviews-bot commented Aug 8, 2025 •

edited

Loading

moisesPompilio commented Aug 11, 2025 •

edited

Loading