Skip to content

fuzz: add chanmon holder signer fuzz ops#4660

Open
joostjager wants to merge 1 commit into
lightningdevkit:mainfrom
joostjager:fuzz-chanmon-holder-signer-ops
Open

fuzz: add chanmon holder signer fuzz ops#4660
joostjager wants to merge 1 commit into
lightningdevkit:mainfrom
joostjager:fuzz-chanmon-holder-signer-ops

Conversation

@joostjager
Copy link
Copy Markdown
Contributor

Allow chanmon consistency inputs to block and later unblock holder-side signing operations. This lets focused force-close fuzzing reuse the signer-op machinery without carrying the larger mining and settlement model.

I split this out ahead of the broader force-close fuzzing work so we can review the signer-op modeling independently. The main question is whether these holder-side unblock actions are worth spending dedicated chanmon consistency opcodes on, or whether we should keep the opcode surface smaller and rely on final cleanup to re-enable them.

@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented Jun 4, 2026

I've assigned @valentinewallace 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.

Allow chanmon consistency inputs to block and later unblock
holder-side signing operations. This lets focused force-close
fuzzing reuse the signer-op machinery without carrying the larger
mining and settlement model.
@joostjager joostjager force-pushed the fuzz-chanmon-holder-signer-ops branch from 994d779 to ba0e4c5 Compare June 4, 2026 11:58
@joostjager joostjager marked this pull request as ready for review June 4, 2026 14:44
@joostjager joostjager self-assigned this Jun 4, 2026
@ldk-claude-review-bot
Copy link
Copy Markdown
Collaborator

I've thoroughly reviewed the entire PR diff, examining:

  1. The expanded SUPPORTED_SIGNER_OPS array (4 -> 6 elements)
  2. The new enable_holder_signer_ops() helper method
  3. The new opcodes 0xe4-0xe6 in the fuzz dispatch
  4. The settle_all finalization changes adding monitor.signer_unblocked(None) calls

I verified that:

  • SignHolderCommitment is only used in chain monitor paths (chain/package.rs:668, chain/channelmonitor.rs:5393), NOT in ChannelManager live-channel paths, so calling only monitor.signer_unblocked() (without node.signer_unblocked()) in enable_holder_signer_ops() is correct.
  • disable_supported_ops_for_all_signers iterates over the same SUPPORTED_SIGNER_OPS array, so the disable/enable lifecycle is consistent.
  • The finalization in settle_all correctly re-enables all ops and notifies both manager and monitor.
  • The fuzz target binary needs no changes.

No issues found.

Copy link
Copy Markdown
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any heuristics on when to save vs spend fuzz opcodes? I'm not sure how to make that call, probably @TheBlueMatt should take a look. We can always update post-merge though, I suppose.

Comment on lines +3406 to +3407
// Keep holder signer unblocks adjacent to the existing signer op
// bytes. The helper re-enables both holder-side operations for
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Adjacent" makes me think these bytes should be 0xd3..d5, so I find that a bit confusing.

self.node.timer_tick_occurred();
}

// Re-enables holder-side signer operations and asks the chain monitor to
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the commit message could be improved. It references a larger refactor, which I'm not sure is helpful as someone without much context, and I don't think it explains the "why" of the diff as it is stand-alone.

// bytes. The helper re-enables both holder-side operations for
// every signer owned by the selected node, matching the existing
// key-manager-wide blocking model.
0xe4 => harness.nodes[0].enable_holder_signer_ops(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other bytes of this fuzzer seem much more granular and enable/unblock on a per-op and per-channel basis. Maybe we could document why we're taking a more sweeping approach here? Also "the existing key-manager-wide blocking model" -- what does that mean?

@ldk-reviews-bot
Copy link
Copy Markdown

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants