Skip to content

fuzz: fix signature weight assumptions#4620

Merged
wpaulino merged 2 commits into
lightningdevkit:mainfrom
joostjager:fuzz-sigs
May 19, 2026
Merged

fuzz: fix signature weight assumptions#4620
wpaulino merged 2 commits into
lightningdevkit:mainfrom
joostjager:fuzz-sigs

Conversation

@joostjager
Copy link
Copy Markdown
Contributor

This aligns fuzz-build signature-size assumptions with what secp256k1_fuzz can produce, avoiding debug-only weight assertion failures. This is a preparation for #4381

@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented May 19, 2026

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

Comment on lines +550 to +554
// The secp256k1 fuzz signer emits dummy signatures without low-S normalization, so
// DER+sighash can be one byte larger for each of the two HTLC signatures.
#[cfg(secp256k1_fuzz)]
let (htlc_success_witness_weight, htlc_timeout_witness_weight) =
(htlc_success_witness_weight + 2, htlc_timeout_witness_weight + 2);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: The +2 correctly adjusts satisfaction_weight in the Input structs (used for the debug assertion at line 740), but the aggregated_htlc_success_input_output_pair_weight() / aggregated_htlc_timeout_input_output_pair_weight() functions called on lines 595/597 still use the unadjusted constants for batch sizing. Under secp256k1_fuzz, the per-HTLC weight is underestimated by 2 WU in those calls.

In practice this is safe — worst case is 483 HTLCs × 2 WU = 966 WU undercounting, which is absorbed by the 1000 WU USER_COINS_WEIGHT_BUDGET margin. But it's a conceptual inconsistency worth noting if this area evolves.

@ldk-claude-review-bot
Copy link
Copy Markdown
Collaborator

ldk-claude-review-bot commented May 19, 2026

Good — lightning-persister never references weight constants or signature grinding, confirming the default-features = false change is safe for it.

I have reviewed the complete diff across all files:

  1. lightning/src/lib.rscompile_error! guard for fuzzing + grind_signatures
  2. lightning/src/events/bump_transaction/mod.rs — anchor +1 and HTLC +2 fuzz adjustments, all three HTLC pair-weight call sites
  3. fuzz/Cargo.tomldefault-features = false with explicit std
  4. lightning-persister/Cargo.tomldefault-features = false with explicit std
  5. fuzz/src/full_stack.rs — txid 0x31→0x32, output value 115538→115537, all message references and log assertions

Everything is internally consistent and correct. My prior inline comment about the batch-sizing inconsistency (unadjusted aggregated_htlc_*_input_output_pair_weight() calls) has been addressed in this version by the new fuzz-adjusted local variables at lines 561-567.

No issues found.

Disable default lightning features in the fuzz crate and persister
so fuzz builds do not inherit grind_signatures.

Add a compile-time guard for fuzzing plus grind_signatures.
Refresh the splice fuzz seed because the no-low-R weight model
changes the signed funding transaction amount and fake-hash txid.
When secp256k1_fuzz is active, dummy ECDSA signatures may
serialize one byte larger per signature. Use fuzz-aware witness
estimates for keyed-anchor bumping and HTLC resolution so debug
weight assertions and aggregation limits use the fuzz signer bound.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

❌ Patch coverage is 62.50000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.54%. Comparing base (2af4096) to head (241ac47).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/events/bump_transaction/mod.rs 62.50% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4620      +/-   ##
==========================================
- Coverage   86.56%   86.54%   -0.02%     
==========================================
  Files         159      159              
  Lines      109860   109877      +17     
  Branches   109860   109877      +17     
==========================================
- Hits        95100    95096       -4     
- Misses      12242    12262      +20     
- Partials     2518     2519       +1     
Flag Coverage Δ
fuzzing-fake-hashes 6.58% <0.00%> (+<0.01%) ⬆️
fuzzing-real-hashes 23.23% <0.00%> (+<0.01%) ⬆️
tests 86.18% <100.00%> (-0.02%) ⬇️

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.

@wpaulino wpaulino merged commit 4d6528c into lightningdevkit:main May 19, 2026
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants