Async BumpTransactionEventHandler by joostjager · Pull Request #3752 · lightningdevkit/rust-lightning

joostjager · 2025-04-28T19:09:23Z

Converts BumpTransactionEventHandler to async.

Changes the CoinSelectionSource and WalletSource traits to be async and provides WalletSourceSyncWrapper as a helper for users that want to implement a sync wallet source.

TestWalletSource is kept sync, to prevent a cascade of async conversions in tests.

Fixes #3540

ldk-reviews-bot · 2025-04-28T19:09:26Z

👋 I see @tnull was un-assigned.
If you'd like another reviewer assignemnt, please click here.

codecov · 2025-05-01T11:16:59Z

Codecov Report

Attention: Patch coverage is 90.08621% with 23 lines in your changes missing coverage. Please review.

Project coverage is 90.78%. Comparing base (35748f6) to head (e7d43db).
Report is 16 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/events/bump_transaction.rs	89.92%	5 Missing and 9 partials ⚠️
lightning/src/events/bump_transaction_sync.rs	90.00%	8 Missing ⚠️
lightning/src/util/test_utils.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3752      +/-   ##
==========================================
+ Coverage   89.92%   90.78%   +0.86%     
==========================================
  Files         160      161       +1     
  Lines      129322   137021    +7699     
  Branches   129322   137021    +7699     
==========================================
+ Hits       116290   124393    +8103     
+ Misses      10345     9956     -389     
+ Partials     2687     2672      -15

☔ 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

Did a first / quickish pass. Generally looks good so far.

tnull · 2025-05-15T13:17:48Z

lightning/Cargo.toml


 libm = { version = "0.2", default-features = false }
 inventory = { version = "0.3", optional = true  }
+tokio = { version = "1.35", features = [ "macros", "rt" ], optional = true }


Please set default-features = false here and below.

Added. Why is this? I looked at the tokio crate and it seems there are no default features?

It's 'best practice' and will also make sure we'd not add arbitrary dependencies if tokio decided to add default features in some release.

tnull · 2025-05-15T13:21:04Z

lightning/src/events/bump_transaction.rs

-					*commitment_tx_fee_satoshis,
-					anchor_descriptor,
-				) {
+				if let Err(_) = self


IMO these would be a bit more readable if we did the the whole call including .await on a separate line and then simply did if res.is_err() { .. }

Tried it, but I don't think it is an improvement with that temp var.

let res = self .handle_channel_close( *claim_id, *package_target_feerate_sat_per_1000_weight, commitment_tx, *commitment_tx_fee_satoshis, anchor_descriptor, ) .await; if res.is_err() { log_error!( self.logger, "Failed bumping commitment transaction fee for {}", commitment_tx.compute_txid() ); }

Also tried with map_err, but that doesn't work all that well with the parent fn without a return result.

Also tried with map_err, but that doesn't work all that well with the parent fn without a return result.

How about:

self.handle_channel_close( *claim_id, *package_target_feerate_sat_per_1000_weight, commitment_tx, *commitment_tx_fee_satoshis, anchor_descriptor, ) .await .unwrap_or_else(|_| { log_error!( self.logger, "Failed bumping commitment transaction fee for {}", commitment_tx.compute_txid() ); });

Ah this is nicer. Wasn't aware of the unwrap_or_else on result. Updated.

lightning/src/events/bump_transaction.rs

tnull · 2025-05-15T13:27:08Z

lightning/src/ln/functional_tests.rs

+	#[allow(clippy::expect_used, clippy::diverging_sub_expression)]
+	{
+		return tokio::runtime::Builder::new_current_thread()
+			.enable_all()


FWIW, I don't think we need to enable_all here, could just actually enable features we need. Same goes for all the tests that use [tokio::test], although not sure if it makes a big difference in practice.

I just expanded the macro. Can make customizations, but maybe it is better to keep to the original expansion? Also if it doesn't matter in practice...

Yeah, no strong opinion. If we increase the number of tests using tokio, we might want to some benchmarks though, as the runtime of cargo test accumulates over time, so every second we can squeeze out of it would be appreciated.

Removed then. If we get more of these xtests, we might need to duplicate the macro or something like that.

lightning/src/events/bump_transaction.rs

tnull · 2025-05-15T13:52:19Z

lightning/src/events/bump_transaction.rs

 				target_feerate_sat_per_1000_weight,
 				force_conflicting_utxo_spend,
 				tolerate_high_network_feerates
 			);


nit: Indent off here.

I don't see the indent off. The closing parenthesis?

Actually the parenthesis and all arguments of log_debug too (they need to be indented by one tab).

I don't see the problem anymore in the diff now, but github still shows it on the old code fragment? In my IDE it all looks good with tabs, so I think it is solved.

Nope, still there on HEAD:

Ah, happened in the next commit. Fixed now.

ldk-reviews-bot · 2025-05-15T13:52:43Z

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

joostjager · 2025-06-09T14:18:51Z

Reviewers, didn't do fix up commits as the commit structure changed substantially. Thanks for your patience with this PR. Should have realized that the sync wrappers could avoid all the test changes that have now been reverted.

tnull

Basically LGTM, some nits/comments.

Also double-checked this still works with LDK Node.

lightning/src/events/bump_transaction.rs

tnull · 2025-06-10T10:39:21Z

lightning/src/events/bump_transaction_sync.rs

Not sure having this module is parallel is the way to go. If you really prefer to move the wrappers to their own file, should they be in events/bump_transaction/blocking.rs (or sync.rs, but the latter is usually associated with synchronization primitives in Rust), for example?

Also, this module is missing a licensing header.

I put them in a separate file to avoid large files. Quite a few devs indicated that they did not like large files.

What is wrong with bump_transaction_sync.rs? It aligns with the types inside. Something like blocking.rs might grow big again.

What is wrong with bump_transaction_sync.rs?

In terms of crate organization, it's just a file in-parallel even though it hold utilities for the bump_transaction logic. So would just make sense to have it part of the same sub-module rather an in-parallel module, IMO, but no blocker.

Something like blocking.rs might grow big again.

Not sure I'm following how the name relates to what/how much we put in a file?

Ah, I missed that you suggested a bump_tx subdir. Made the change in a fix up commit. Generated docs look better now, with sync residing below bump_transaction. Let me know if this is what you had in mind.

lightning/src/events/bump_transaction_sync.rs

This preparatory commit converts do_coin_selection from a closure into a standard function.

tnull

Feel free to squash fixups from my side.

joostjager · 2025-06-10T13:25:56Z

Squashed

lightning/src/events/bump_transaction/sync.rs

TheBlueMatt

Please write a commit message for the last commit. Its +393 -105 and very far from trivial. It needs a textual explanation of the approach, why we're doing wrappers, etc.

TheBlueMatt · 2025-06-10T18:55:06Z

lightning/src/events/bump_transaction.rs

 //!
 //! [`Event`]: crate::events::Event

+pub mod sync;


If we're moving all the wrappers into a separate module let's definitely link to the wrapper types in the documentation for the traits/structs. When all the types are the in the same module things are pretty visible in docs, when they're not the sync wrappers become invisible and people will get confused and think they have to use the async version.

Links added

I am getting to see more of how rustdoc works with this PR. It's quite nice.

TheBlueMatt · 2025-06-10T19:00:16Z

lightning/src/events/bump_transaction/sync.rs

+where
+	T::Target: WalletSourceSync,
+{
+	/// Returns all UTXOs, with at least 1 confirmation each, that are available to spend. Wraps


Usually we don't bother to write documentation for impls of traits as rustdoc doesn't do a good job of surfacing it, but also in this case its just repeating the trait docs?

TheBlueMatt · 2025-06-10T19:02:43Z

lightning/src/events/bump_transaction/sync.rs

+	}
+}
+
+/// A synchronous helper trait that can be used to implement [`CoinSelectionSource`] in a synchronous


These docs seem to imply to me that I'm not supposed to use it directly (its a "helper trait") but that's not accurate.

TheBlueMatt · 2025-06-10T19:03:40Z

lightning/src/events/bump_transaction/sync.rs

+/// This wrapper isn't intended to be used directly, because that would risk blocking an async context. Instead, it is
+/// built for you in [`WalletSync::new`].
+#[doc(hidden)]
+pub(crate) struct WalletSourceSyncWrapper<T: Deref>(T)


Ah, I didn't realize we could make this non-pub. We don't really have to worry about verbose docs in that case, and probably shouldn't #[doc(hidden)] since it should show up in crate-internal documentation (cargo doc --document-private-items)

Indeed, no longer necessary. Removed.

TheBlueMatt · 2025-06-10T19:04:13Z

lightning/src/events/bump_transaction/sync.rs

+where
+	T::Target: CoinSelectionSourceSync,
+{
+	#[allow(dead_code)]


Hmm, seems this came back after it had already been dropped? #3752 (comment)

Didn't remove all of it apparently. And also found a new for WalletSourceSyncWrapper that doesn't add much. Removed as well.

I mostly meant the dead_code cause its used now, but removing the new method is also nice.

TheBlueMatt · 2025-06-10T19:07:34Z

lightning/src/events/bump_transaction/sync.rs

@@ -0,0 +1,241 @@
+// This file is Copyright its original authors, visible in version control


nit: It seems more sensible to move bump_transaction.rs to bump_transaction/mod.rs to keep it and sync.rs together.

Done. Although so many mod.rs files with actual code make it a bit harder to navigate. Not sure if rust has a way of keeping files in the same sub dir and still have a descriptive file name, without making it a sub module. And which is also in line with project conventions.

The only way to do that would be to have a separate module and re-export from the top-level one. I'm generally not a fan of that unless we have a good reason (eg the module is way too large), which this doesn't feel like.

joostjager · 2025-06-11T09:53:47Z

Comments addressed, commit message expanded. Ready for another look.

tnull

Fixups look good to me. Could be squashed from my side, once Matt saw them, too.

TheBlueMatt · 2025-06-11T14:00:30Z

Feel free to squash

TheBlueMatt

Needs squash

This commit converts the `CoinSelectionSource` and `WalletSource` traits to `async` and bubbles up the `async` keyword where needed. To preserve usability in a synchronous context, sync versions of these traits are still provided and can be used via the `WalletSync` and `BumpTransactionEventHandlerSync` wrappers. Existing synchronous tests remain unchanged and also use these wrappers. Additionally, the `MaybeSync` and `MaybeSend` marker traits are introduced to support `no_std` compilation. Due to limitations in Rust 1.63's type inference, these marker traits are used more frequently than might be necessary in later versions.

joostjager · 2025-06-11T15:02:28Z

Squashed

TheBlueMatt

Gonna go ahead and land this since @tnull ACK'd it previously and LGTM'd the fixups. Will probably open a followup to remove a few allocations but no need to delay this.

TheBlueMatt · 2025-06-11T19:06:53Z

lightning/src/util/test_utils.rs

 pub struct TestWalletSource {
 	secret_key: SecretKey,
-	utxos: RefCell<Vec<Utxo>>,
+	utxos: Mutex<Vec<Utxo>>,


I think its okay. Its certainly not ideal but it only kicks on with std, and the alternative is macro-ing the whole code and duplicating it all, which I'm not really sure is a better answer. Of course once Rust has proper impl Trait returns from trait methods we can drop this stuff, but it sounds like thats still a bit off :/

TheBlueMatt · 2025-06-11T20:41:12Z

lightning/src/ln/functional_test_utils.rs

-			wallet_source: Arc::clone(&wallet_source),
-			bump_tx_handler: BumpTransactionEventHandler::new(
-				cfgs[i].tx_broadcaster, Arc::new(Wallet::new(Arc::clone(&wallet_source), cfgs[i].logger)),
+			wallet_source: wallet_source.clone(),


Definitely prefer to leave Arc::clones over .clone()s. They're way more readable :)

I don't feel it. Maybe that feeling needs time to develop 😅

I don't feel it. Maybe that feeling needs time to develop 😅

Maybe. FWIW, I share the preference for Arc::clone, especially as it highlights that it's not a deep (i.e. costly) clone, but just cloning a reference..

Trivial #3752 followups

joostjager force-pushed the async-wallet-bump branch 3 times, most recently from d9b1c75 to 873516b Compare May 1, 2025 11:08

joostjager force-pushed the async-wallet-bump branch 6 times, most recently from 4b4b898 to 3816c66 Compare May 5, 2025 11:37

joostjager force-pushed the async-wallet-bump branch 13 times, most recently from 1a86521 to e1509f1 Compare May 14, 2025 09:55

joostjager changed the title ~~Async wallet bump~~ Async BumpTransactionEventHandler May 14, 2025

joostjager marked this pull request as ready for review May 14, 2025 11:06

joostjager requested a review from tnull May 14, 2025 11:06

joostjager added the weekly goal Someone wants to land this this week label May 14, 2025

tnull reviewed May 15, 2025

View reviewed changes

tnull reviewed Jun 10, 2025

View reviewed changes

Rewrite closure do_coin_selection in Wallet impl

048230a

This preparatory commit converts do_coin_selection from a closure into a standard function.

joostjager force-pushed the async-wallet-bump branch 2 times, most recently from 93e5d7e to 2fdcf05 Compare June 10, 2025 12:54

tnull reviewed Jun 10, 2025

View reviewed changes

joostjager force-pushed the async-wallet-bump branch from 2fdcf05 to 94bbb5c Compare June 10, 2025 13:25

tnull reviewed Jun 10, 2025

View reviewed changes

lightning/src/events/bump_transaction/sync.rs Show resolved Hide resolved

joostjager force-pushed the async-wallet-bump branch from 94bbb5c to 0ca7739 Compare June 10, 2025 13:35

joostjager requested a review from tnull June 10, 2025 13:37

tnull previously approved these changes Jun 10, 2025

View reviewed changes

TheBlueMatt reviewed Jun 10, 2025

View reviewed changes

joostjager dismissed tnull’s stale review via 8b0fd27 June 11, 2025 09:52

joostjager force-pushed the async-wallet-bump branch from 0ca7739 to 8b0fd27 Compare June 11, 2025 09:52

tnull reviewed Jun 11, 2025

View reviewed changes

joostjager requested review from TheBlueMatt and tnull and removed request for tnull June 11, 2025 12:28

TheBlueMatt reviewed Jun 11, 2025

View reviewed changes

joostjager force-pushed the async-wallet-bump branch from 8b0fd27 to 0820322 Compare June 11, 2025 15:02

joostjager requested a review from TheBlueMatt June 11, 2025 18:29

TheBlueMatt approved these changes Jun 11, 2025

View reviewed changes

joostjager merged commit 7e6ca67 into lightningdevkit:main Jun 12, 2025
26 of 27 checks passed

joostjager mentioned this pull request Jun 12, 2025

Trivial #3752 followups #3848

Merged

tnull added a commit that referenced this pull request Jun 16, 2025

Merge pull request #3848 from TheBlueMatt/2025-06-3752-clones

e9c9d12

Trivial #3752 followups

		@@ -0,0 +1,241 @@
		// This file is Copyright its original authors, visible in version control

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

tnull left a comment • 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!

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!

joostjager May 15, 2025 • 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager May 15, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager May 16, 2025 • 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!

ldk-reviews-bot commented May 15, 2025

Uh oh!

joostjager commented Jun 9, 2025

Uh oh!

tnull left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

joostjager commented Apr 28, 2025 •

edited

Loading

ldk-reviews-bot commented Apr 28, 2025 •

edited

Loading

codecov bot commented May 1, 2025 •

edited

Loading

tnull left a comment •

edited

Loading

joostjager May 15, 2025 •

edited

Loading

joostjager May 15, 2025 •

edited

Loading

joostjager May 16, 2025 •

edited

Loading

joostjager Jun 11, 2025 •

edited

Loading