Make fuzz targets deterministic by joostjager · Pull Request #4525 · lightningdevkit/rust-lightning

joostjager · 2026-03-31T06:27:09Z

Use deterministic time and hash table ordering when building with cfg(fuzzing) to ensure fuzz test cases reproduce consistently.

ldk-reviews-bot · 2026-03-31T06:27:12Z

👋 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.

codecov · 2026-03-31T07:27:19Z

Codecov Report

❌ Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.09%. Comparing base (450c03a) to head (69edc09).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	80.00%	1 Missing ⚠️
lightning/src/onion_message/dns_resolution.rs	50.00%	0 Missing and 1 partial ⚠️
lightning/src/util/hash_tables.rs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4525      +/-   ##
==========================================
- Coverage   87.10%   87.09%   -0.01%     
==========================================
  Files         163      163              
  Lines      108740   108774      +34     
  Branches   108740   108774      +34     
==========================================
+ Hits        94723    94742      +19     
- Misses      11531    11544      +13     
- Partials     2486     2488       +2

Flag	Coverage Δ
fuzzing	`40.21% <63.63%> (+0.01%)`	⬆️
tests	`86.20% <77.77%> (+<0.01%)`	⬆️

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.

lightning/src/ln/channelmanager.rs

ldk-claude-review-bot · 2026-03-31T09:18:59Z

The serialization for Retry is gated on #[cfg(feature = "std")], not not(fuzzing). This means under std + fuzzing, Retry::Timeout can still be deserialized. If a Retry::Timeout value is used, is_retryable_now will return true always. This is internally consistent with the PR's approach - they just made Retry::Timeout a no-op during fuzzing.

I've completed my thorough review. My prior review comments cover all the significant issues. I don't see any new bugs or issues beyond what was already flagged.

Review Summary

All issues from the prior review pass remain applicable. No new issues found beyond what was already covered.

Prior inline comments (still applicable, not re-posted)

lightning/src/ln/outbound_payment.rs:452-453 — Retry::Timeout always returns true during fuzzing, creating a potential infinite-retry footgun for future fuzz targets using default retry strategies.
lightning/src/routing/gossip.rs:2323 — Split #[cfg] attributes 17 lines apart; should be merged for readability.
fuzz/Cargo.toml:21 — default-features = false is a no-op due to Cargo feature unification through transitive deps.
lightning/src/ln/channelmanager.rs:~15006 — 3-branch pattern inconsistency (may now be resolved based on current code showing 2-branch pattern).

Cross-cutting concerns (unchanged from prior review)

Incomplete SystemTime::now() coverage: Public API methods respond_with and respond_using_derived_keys in offers/invoice_request.rs (lines 768, 1000) and offers/refund.rs (lines 571, 626) remain gated on just #[cfg(feature = "std")] and still call SystemTime::now(). While offers/flow.rs correctly redirects to _no_std variants during fuzzing, these public methods would produce non-deterministic results if invoked directly from a fuzz target.

Verification of this review pass

All files and hunks in the diff were examined. The cfg gating pattern (#[cfg(all(feature = "std", not(fuzzing)))] / #[cfg(any(not(feature = "std"), fuzzing))]) is applied consistently across all modified locations. The cfg!() macro usage in dns_resolution.rs:516 is correct. No new issues found.

joostjager · 2026-03-31T09:39:43Z

Claude comments addressed

TheBlueMatt · 2026-03-31T13:11:35Z

lightning/src/offers/flow.rs

+		#[cfg(any(not(feature = "std"), fuzzing))]
 		let now = Duration::from_secs(self.highest_seen_timestamp.load(Ordering::Acquire) as u64);
-		#[cfg(feature = "std")]
+		#[cfg(all(feature = "std", not(fuzzing)))]


Shouldn't we just fuzz with no-std, then?

We'd redefine no-std as being deterministic, seems like an undesirable shortcut.

I meant just for the time logic - no-std is already defined as not supporting fetching the current time, seems reasonable to use that.

~~It appears that deterministic hashes were already in the code for fuzzing, so the only change needed is then to switch to no std.~~

Claude review pointed out that no-std wasn't actually enabled. And enabling it runs into the issue that test_utils also needs to be made no-std compatible. Then it becomes a larger change.

I am still in favor of the original change that adds the fuzzing flag to the time code.

ldk-claude-review-bot · 2026-04-01T07:25:19Z

fuzz/Cargo.toml


 [dependencies]
-lightning = { path = "../lightning", features = ["regex", "_test_utils"] }
+lightning = { path = "../lightning", default-features = false, features = ["regex", "_test_utils"] }


Bug: This change is a no-op due to Cargo feature unification.

lightning-persister on line 25 is included without default-features = false:

lightning-persister = { path = "../lightning-persister", features = ["tokio"]}

And lightning-persister/Cargo.toml depends on lightning with default features:

lightning = { version = "0.3.0", path = "../lightning" }

Lightning's defaults are ["std", "grind_signatures"]. Through Cargo feature unification, lightning-persister's dependency re-enables both std and grind_signatures on the unified lightning crate, making your default-features = false here have no effect.

Similarly, lightning-liquidity (line 23) uses default features which include std = ["lightning/std"], also re-enabling lightning/std.

To actually disable grind_signatures, you'd need to ensure no transitive dependency pulls in lightning with default features. At minimum, lightning-persister/Cargo.toml itself would need to be updated to use default-features = false for its lightning dependency (and explicitly enable only the features it needs, likely std).

Good point...

lightning/src/routing/gossip.rs

Gate all SystemTime::now() and Instant::now() calls in production code with #[cfg(all(feature = "std", not(fuzzing)))] so that fuzz targets produce consistent results regardless of wall-clock time. For each location, the existing no-std fallback (highest_seen_timestamp, None, or a constant) is reused under fuzzing. Also force deterministic hashing when the fuzzing cfg is active, rather than requiring the LDK_TEST_DETERMINISTIC_HASHES env var. AI tools were used in preparing this commit.

ldk-claude-review-bot · 2026-04-01T09:31:52Z

lightning/src/ln/outbound_payment.rs

+			#[cfg(all(feature = "std", fuzzing))]
+			(Retry::Timeout(_), _) => true,


Bug (latent): Under std + fuzzing, Retry::Timeout is always retryable. But OptionalBolt11PaymentParams::default() and OptionalOfferPaymentParams::default() (channelmanager.rs lines ~709 and ~747) still use Retry::Timeout(2s) when std is enabled — they aren't gated on not(fuzzing).

This means any code path that uses the default retry strategy during fuzzing will retry payments indefinitely (never hitting RetriesExhausted). Current fuzz targets explicitly use Retry::Attempts(2) so they're unaffected, but this is a footgun for future fuzz targets.

Consider updating those defaults to also use Retry::Attempts(3) under fuzzing:

#[cfg(all(feature = "std", not(fuzzing)))] retry_strategy: Retry::Timeout(core::time::Duration::from_secs(2)), #[cfg(any(not(feature = "std"), fuzzing))] retry_strategy: Retry::Attempts(3),

joostjager mentioned this pull request Mar 31, 2026

Fuzz failure backlog #4504

Open

joostjager marked this pull request as ready for review March 31, 2026 09:13

joostjager requested a review from TheBlueMatt March 31, 2026 09:13

ldk-claude-review-bot reviewed Mar 31, 2026

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

joostjager force-pushed the fuzz-deterministic branch from bb4d148 to 1a81a3a Compare March 31, 2026 09:39

TheBlueMatt reviewed Mar 31, 2026

View reviewed changes

joostjager force-pushed the fuzz-deterministic branch from 1a81a3a to 9ffd8b3 Compare April 1, 2026 07:20

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

View reviewed changes

joostjager force-pushed the fuzz-deterministic branch 2 times, most recently from 1a81a3a to 9f4e929 Compare April 1, 2026 08:29

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

View reviewed changes

lightning/src/routing/gossip.rs Outdated Show resolved Hide resolved

joostjager force-pushed the fuzz-deterministic branch from 9f4e929 to 35fb66b Compare April 1, 2026 09:24

joostjager force-pushed the fuzz-deterministic branch from 35fb66b to 69edc09 Compare April 1, 2026 09:27

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

View reviewed changes

		#[cfg(all(feature = "std", fuzzing))]
		(Retry::Timeout(_), _) => true,

Conversation

joostjager commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

codecov bot commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

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

Uh oh!

Review Summary

Prior inline comments (still applicable, not re-posted)

Cross-cutting concerns (unchanged from prior review)

Verification of this review pass

Uh oh!

joostjager commented Mar 31, 2026

Uh oh!

TheBlueMatt Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

ldk-claude-review-bot Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

joostjager Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldk-claude-review-bot Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joostjager commented Mar 31, 2026 •

edited

Loading

ldk-reviews-bot commented Mar 31, 2026 •

edited

Loading

codecov bot commented Mar 31, 2026 •

edited

Loading

ldk-claude-review-bot commented Mar 31, 2026 •

edited

Loading

joostjager Apr 1, 2026 •

edited

Loading