Add fuzz coverage for splicing + minor bug fixes #4219

wpaulino · 2025-11-11T18:10:23Z

No description provided.

ldk-reviews-bot · 2025-11-11T18:10:27Z

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

codecov · 2025-11-11T19:32:57Z

Codecov Report

❌ Patch coverage is 44.44444% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.32%. Comparing base (e42e74e) to head (1e78628).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	44.44%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4219      +/-   ##
==========================================
- Coverage   89.33%   89.32%   -0.01%     
==========================================
  Files         180      180              
  Lines      138055   138166     +111     
  Branches   138055   138166     +111     
==========================================
+ Hits       123326   123420      +94     
- Misses      12122    12140      +18     
+ Partials     2607     2606       -1

Flag	Coverage Δ
fuzzing	`35.82% <18.51%> (+2.24%)`	⬆️
tests	`88.70% <44.44%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jkczyz · 2025-11-11T20:37:10Z

lightning/src/ln/channel.rs

+			match self.send_stfu(logger) {
+				Ok(stfu) => Ok(Some(stfu)),
+				Err(e) => {
+					log_debug!(logger, "{e}");
+					Ok(None)
+				},
+			}


Could we have send_stfu return an Option instead?

It's nice to have the error context come from the method itself IMO

lightning/src/ln/channel.rs

jkczyz · 2025-11-11T20:44:51Z

lightning/src/ln/channel.rs

-		if !is_splice {
-			self.assert_no_commitment_advancement(holder_commitment_transaction_number, "initial commitment_signed");
-			self.channel_state = ChannelState::FundingNegotiated(FundingNegotiatedFlags::new());
-		}


This method might still be useful for DRYing up async signing. Alternatively, could keep it but move these lines to Channel::funding_tx_constructed.

It should just be a matter of setting a signer_pending_* var to true and maybe a log statement

TheBlueMatt

All LGTM, thanks! I know its annoying, but one thing worth doing would be to have a splice in the full_stack_target seeds. That will let us catch things that require only small modifications to existing messages (eg overfows in splicing fees/amount logic).

TheBlueMatt · 2025-11-11T20:54:31Z

lightning/src/ln/channel.rs

+			match self.send_stfu(logger) {
+				Ok(stfu) => Ok(Some(stfu)),
+				Err(e) => {
+					log_debug!(logger, "{e}");


Would context be helpful?

Suggested change

log_debug!(logger, "{e}");

log_debug!(logger, "Quiescence pending, waiting to send stfu: {e}");

There's a Attempting to initiate quiescence log at the top of the method that should provide enough context

There may well be other things going on in the node that cause the logs to appear separately, I don't think two logs in a row is a good excuse to avoid context that won't be distracting.

When we propose quiescence due to a user initiated action, such as a splice, it's possible that the `stfu` message is not ready to be sent out yet due to the state machine pending a change. This would result in an error communicated back to the caller of `ChannelManager::splice_channel`, but this is unnecessary and confusing, the splice is still pending and will be carried out once quiescence is eventually negotiated.

We incorrectly assumed that both commitments must be at the same height, but this is certainly possible whenever updates are proposed in both directions at the same time.

This method was originally intended to DRY the logic between `FundedChannel` and `PendingChannelV2` when handling `funding_tx_constructed`, though it's not very useful anymore.

Splicing should also have fuzz coverage, and it depends on the quiescence protocol.

wpaulino · 2025-11-11T22:28:41Z

I know its annoying, but one thing worth doing would be to have a splice in the full_stack_target seeds.

I'll have a follow-up for this, will need to familiarize myself with the setup here.

I just pushed coverage for splice-out as well.

TheBlueMatt · 2025-11-12T12:48:48Z

fuzz/src/chanmon_consistency.rs

+					.map(|chan| chan.outbound_capacity_msat)
+					.unwrap();
+				if outbound_capacity_msat >= 20_000_000 {
+					let contribution = SpliceContribution::SpliceOut {


Would be nice to DRY these up somewhat.

TheBlueMatt · 2025-11-12T12:49:52Z

lightning/src/ln/channel.rs

+			match self.send_stfu(logger) {
+				Ok(stfu) => Ok(Some(stfu)),
+				Err(e) => {
+					log_debug!(logger, "{e}");


There may well be other things going on in the node that cause the logs to appear separately, I don't think two logs in a row is a good excuse to avoid context that won't be distracting.

TheBlueMatt · 2025-11-12T13:08:31Z

Backported in #4221

wpaulino added this to the 0.2 milestone Nov 11, 2025

wpaulino requested review from TheBlueMatt and jkczyz November 11, 2025 18:10

wpaulino added the backport 0.2 label Nov 11, 2025

jkczyz reviewed Nov 11, 2025

View reviewed changes

TheBlueMatt previously approved these changes Nov 11, 2025

View reviewed changes

wpaulino added 4 commits November 11, 2025 14:27

Remove invalid splice debug assertion in funding_tx_constructed

a6ed667

We incorrectly assumed that both commitments must be at the same height, but this is certainly possible whenever updates are proposed in both directions at the same time.

Break up ChannelContext::funding_tx_constructed

25264d7

This method was originally intended to DRY the logic between `FundedChannel` and `PendingChannelV2` when handling `funding_tx_constructed`, though it's not very useful anymore.

Replace quiescence fuzz coverage with splicing

1e78628

Splicing should also have fuzz coverage, and it depends on the quiescence protocol.

wpaulino dismissed TheBlueMatt’s stale review via 1e78628 November 11, 2025 22:27

wpaulino force-pushed the fuzz-splicing branch from 9911a29 to 1e78628 Compare November 11, 2025 22:27

jkczyz approved these changes Nov 11, 2025

View reviewed changes

TheBlueMatt approved these changes Nov 12, 2025

View reviewed changes

TheBlueMatt merged commit ec0b969 into lightningdevkit:main Nov 12, 2025
26 checks passed

TheBlueMatt mentioned this pull request Nov 12, 2025

0.2 backports #4221

Merged

TheBlueMatt removed the backport 0.2 label Nov 12, 2025

wpaulino deleted the fuzz-splicing branch November 12, 2025 19:22

	log_debug!(logger, "{e}");
	log_debug!(logger, "Quiescence pending, waiting to send stfu: {e}");

Add fuzz coverage for splicing + minor bug fixes #4219

Add fuzz coverage for splicing + minor bug fixes #4219

Uh oh!

Conversation

wpaulino commented Nov 11, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

wpaulino commented Nov 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ldk-reviews-bot commented Nov 11, 2025 •

edited

Loading

codecov bot commented Nov 11, 2025 •

edited

Loading