Splice fixes for `chanmon_consistency` fuzz target by wpaulino · Pull Request #4655 · lightningdevkit/rust-lightning

wpaulino · 2026-06-01T22:17:34Z

While this target found several issues in our production code, there were also issues with the fuzz target itself, which this PR addresses. It fixes the following payloads from #4363:

00a6a61b1b1b211b211e1b1b1b1b211b211b211e1b1b261b1b211b1ba1
e8a3a3201ba31b201ba3a3201ba3a3201b201ba3a3201b040d80c113ff
a3cd00a2a1181021181118101810211811181000bb10181021ffff38
ffa1ffffa0414da373ff
a2cd00a2a118102118602700cda2a000a100733a3a6b73a3ffb4ffb7a1
ffa1ffacab211b11baff
88a4a6ffacadab21bcff

ldk-reviews-bot · 2026-06-01T22:17:37Z

👋 Thanks for assigning @TheBlueMatt 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.

ldk-claude-review-bot · 2026-06-01T22:30:10Z

+	for (idx, node) in nodes.iter().enumerate() {
+		if node.broadcaster.txn_broadcasted.borrow().is_empty() {
+			continue;
+		}
+
+		let pending_events = node.get_and_clear_pending_events();


The verification is one-directional: for each broadcast tx, you check a matching SpliceNegotiated event exists, but you don't verify the reverse — that every pending SpliceNegotiated event has a corresponding broadcast tx. A node with no broadcasts (skipped at line 2017) but with dangling SpliceNegotiated events passes silently.

Consider also checking nodes that have no broadcasts but do have pending SpliceNegotiated events, as that would indicate the splice tx was not broadcast despite an event being generated.

Suggested change

for (idx, node) in nodes.iter().enumerate() {

if node.broadcaster.txn_broadcasted.borrow().is_empty() {

continue;

}

let pending_events = node.get_and_clear_pending_events();

for (idx, node) in nodes.iter().enumerate() {

let pending_events = node.get_and_clear_pending_events();

if node.broadcaster.txn_broadcasted.borrow().is_empty() {

assert!(

!pending_events.iter().any(|e| matches!(e, events::Event::SpliceNegotiated { .. })),

"node {} has pending SpliceNegotiated event(s) but no broadcast tx",

idx,

);

continue;

}

ldk-claude-review-bot · 2026-06-01T22:30:42Z

The current diff contains only two hunks, both of which I examined. The error string "not expecting funding signatures" matches the messages in lightning/src/ln/channel.rs:2197 and :2243, confirming the err.contains(...) guard is correct.

No new issues found in this diff beyond my prior review comment. The two changes are correct:

The FundingTransactionReadyForSigning error handling appropriately tolerates stale signing events invalidated by a later tx_abort, and the matched error substring exists in the production code.
The MAX_SETTLE_ITERATIONS constant (100→256) and the typo fix ("may iterations" → "many iterations") are straightforward.

My previously posted comment on the one-directional broadcast/event verification in assert_test_invariants still stands but is not re-posted here.

ldk-reviews-bot · 2026-06-03T22:18:31Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-06-06T00:01:18Z

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-06-08T00:02:11Z

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt · 2026-06-08T12:01:19Z

+			continue;
+		}
+
+		let pending_events = node.get_and_clear_pending_events();


Is it not a cleaner fix to just handle the events here? We could filter the events by FundingNegotiated if we want to retain previous behavior, though not sure it matters much.

TheBlueMatt · 2026-06-08T12:01:59Z

 use std::sync::{Arc, Mutex};

 const MAX_FEE: u32 = 10_000;
+const MAX_SETTLE_ITERATIONS: usize = 256;


joostjager · 2026-06-08T12:13:18Z

-	assert!(nodes[0].broadcaster.txn_broadcasted.borrow().is_empty());
-	assert!(nodes[1].broadcaster.txn_broadcasted.borrow().is_empty());
-	assert!(nodes[2].broadcaster.txn_broadcasted.borrow().is_empty());
+	// Broadcast transactions are handled explicitly. If the input ends immediately after


For context, #4657 is changing chanmon_consistency broadcast handling so broadcast transactions are routed through an explicit modeled mempool, with finish draining remaining broadcasts through relay/mining cleanup.

Should we fix this failure there, then?

Something needs to be re-aligned either way, depending on merge order. Not sure what parts of this PR can be omitted with the more realistic mempool.

Prefer to keep #4657 a pure refactor though, and not include splice bug fixes. It's big enough already.

joostjager · 2026-06-08T12:23:22Z

 		if !self.nodes[node_idx].deferred {
 			self.nodes[node_idx].checkpoint_manager_persistence();
 		}
+		let pre_reload_height = self.nodes[node_idx].height;


I think this pins the node's replay height to the manager height after reload, but can't this skip part of startup sync if an older monitor was reloaded below that height?

Which part specifically? The monitor height is never updated here (though I assume it will with your ongoing force close work).

wpaulino · 2026-06-09T18:07:50Z

I'm dropping 89bdf95 and baa5d0c as the corresponding failures are already addressed by #4657.

Now that the fuzz target supports canceling splice funding attempts, we may see failed signing attempts due to the cancellation.

LDK and the chanmon_consistency fuzz target have grown in complexity recently and thus require more iterations than previously assumed to fully settle the state of all active channels.

wpaulino requested a review from TheBlueMatt June 1, 2026 22:17

wpaulino self-assigned this Jun 1, 2026

wpaulino added this to the 0.3 milestone Jun 1, 2026

ldk-claude-review-bot reviewed Jun 1, 2026

View reviewed changes

TheBlueMatt reviewed Jun 8, 2026

View reviewed changes

joostjager reviewed Jun 8, 2026

View reviewed changes

wpaulino added 2 commits June 9, 2026 11:08

Ignore stale splice signing fuzz events

1379144

Now that the fuzz target supports canceling splice funding attempts, we may see failed signing attempts due to the cancellation.

Raise iteration capacity in chanmon consistency when settling state

099bb09

LDK and the chanmon_consistency fuzz target have grown in complexity recently and thus require more iterations than previously assumed to fully settle the state of all active channels.

wpaulino force-pushed the splice-chanmon-consistency-fixes branch from baa5d0c to 099bb09 Compare June 9, 2026 18:09

Conversation

wpaulino commented Jun 1, 2026

Uh oh!

ldk-reviews-bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-claude-review-bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 3, 2026

Uh oh!

ldk-reviews-bot commented Jun 6, 2026

Uh oh!

ldk-reviews-bot commented Jun 8, 2026

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!

wpaulino commented Jun 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ldk-reviews-bot commented Jun 1, 2026 •

edited

Loading

ldk-claude-review-bot commented Jun 1, 2026 •

edited

Loading