Skip to content

Conversation

@Fedeparma74
Copy link
Contributor

Summary

This PR addresses #4176

Details

  • Removed unnecessary Send and Sync bounds in the lightning-background-processor crate and the async KVStore trait, which were causing compilation issues when building without the std feature.
  • The changes ensure better compatibility for no_std environments while preserving async functionality.

Rationale

These bounds were preventing the code from compiling in async, no-std configurations. By relaxing them, the async background processor and async persistence can now be used in embedded or WASM environments without requiring Send/Sync.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Oct 29, 2025

👋 Thanks for assigning @tnull 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
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

❌ Patch coverage is 69.23077% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.86%. Comparing base (afb80e5) to head (8e4b8e4).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
lightning-background-processor/src/lib.rs 44.44% 5 Missing ⚠️
lightning/src/util/test_utils.rs 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4184      +/-   ##
==========================================
- Coverage   88.86%   88.86%   -0.01%     
==========================================
  Files         180      180              
  Lines      137869   137863       -6     
  Branches   137869   137863       -6     
==========================================
- Hits       122520   122511       -9     
- Misses      12539    12546       +7     
+ Partials     2810     2806       -4     
Flag Coverage Δ
fuzzing 20.85% <5.88%> (+<0.01%) ⬆️
tests 88.70% <69.23%> (-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.

@tnull tnull self-requested a review October 29, 2025 12:56
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

CI is broken, but I just confirmed that the current state works with LDK Node.

&'a (dyn UtxoLookup + Send + Sync),
L,
> where
>
Copy link
Contributor

Choose a reason for hiding this comment

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

This change made rustfmt unhappy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird, as I'm using rustfmt as well. Probably two different versions. Pushed a fix :)

Copy link
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.

Looks like CI is still sad :/

@TheBlueMatt
Copy link
Collaborator

Sadly needs rebase now, but should be pretty trivial, also indeed the test kvstore utils need updating to the new API. I'd like to include this in 0.2, but we're rapidly running out of time for that.

@TheBlueMatt
Copy link
Collaborator

Would you mind rebasing on current git, rather than merging? We require each PR to be a freestanding list of commits, with no merges in them.

@TheBlueMatt
Copy link
Collaborator

Feel free to squash the last two fixup commits into the second commit - we try to require that each commit individually builds and passes tests to ensure git bisect is usable later.

@Fedeparma74
Copy link
Contributor Author

I hope everything looks good now! Sorry for the extra work earlier, and thanks for your guidance! :)

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Thanks! In the future it would be nice to get more context in the commit message but this time we'll let it slide :). Gonna go ahead and land this since @tnull said it doesn't break ldk-node and we want to get this in.

@TheBlueMatt TheBlueMatt merged commit 02a9af9 into lightningdevkit:main Oct 29, 2025
23 of 25 checks passed
@TheBlueMatt TheBlueMatt mentioned this pull request Oct 29, 2025
@TheBlueMatt
Copy link
Collaborator

Backported in #4185

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.

5 participants