Async BumpTransactionEventHandler #3752

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.00000% with 23 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (35748f6) to head (c34c042).

Files with missing lines	Patch %	Lines
lightning/src/events/bump_transaction.rs	90.00%	13 Missing and 9 partials ⚠️
lightning/src/util/test_utils.rs	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3752      +/-   ##
==========================================
- Coverage   89.92%   89.90%   -0.03%     
==========================================
  Files         160      160              
  Lines      129322   129428     +106     
  Branches   129322   129428     +106     
==========================================
+ Hits       116290   116358      +68     
- Misses      10345    10375      +30     
- Partials     2687     2695       +8

☔ 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

@@ -48,12 +48,14 @@ backtrace = { version = "0.3", optional = true }

 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

+			let configs = [(false, false), (false, true), (true, false), (true, true)];
+			let mut last_err = None;
+			for (force_conflicting_utxo_spend, tolerate_high_network_feerates) in configs {
+				log_debug!(
 				self.logger,
 				"Attempting coin selection targeting {} sat/kW (force_conflicting_utxo_spend = {}, tolerate_high_network_feerates = {})",
 				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.

ldk-reviews-bot · 2025-05-24T00:00:41Z

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-05-26T00:00:53Z

🔔 4th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-05-26T00:01:00Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-05-28T00:01:31Z

🔔 5th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-05-28T00:01:36Z

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-05-31T00:00:19Z

🔔 6th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-05-31T00:00:28Z

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-06-02T00:01:29Z

🔔 7th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-06-02T00:01:40Z

🔔 4th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull

Excuse the delay here, finally got to testing this out with LDK Node.

One request for a minor change, otherwise it seems to work as expected with minmal changes (see https://github.com/tnull/ldk-node/tree/2025-02-upgrade-to-ldk-0.2-with-async-bumper)

tnull · 2025-06-03T12:51:41Z

lightning/src/events/bump_transaction.rs

+	fn list_confirmed_utxos<'a>(&'a self) -> AsyncResult<'a, Vec<Utxo>>;
+	/// Returns a script to use for change above dust resulting from a successful coin selection
+	/// attempt.
+	fn get_change_script<'a>(&self) -> AsyncResult<'a, ScriptBuf>;


Is there a particular reason why we bound &'a self for list_confirmed_utxos but not for get_change_script / sign_psbt? I should be able to work around this, but this simple diff would make my life considerably easier:

diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs index 8a3d3cb9b..98bd3525a 100644 --- a/lightning/src/events/bump_transaction.rs +++ b/lightning/src/events/bump_transaction.rs @@ -366,14 +366,14 @@ pub trait WalletSource { fn list_confirmed_utxos<'a>(&'a self) -> AsyncResult<'a, Vec<Utxo>>; /// Returns a script to use for change above dust resulting from a successful coin selection /// attempt. - fn get_change_script<'a>(&self) -> AsyncResult<'a, ScriptBuf>; + fn get_change_script<'a>(&'a self) -> AsyncResult<'a, ScriptBuf>; /// Signs and provides the full [`TxIn::script_sig`] and [`TxIn::witness`] for all inputs within /// the transaction known to the wallet (i.e., any provided via /// [`WalletSource::list_confirmed_utxos`]). /// /// If your wallet does not support signing PSBTs you can call `psbt.extract_tx()` to get the /// unsigned transaction and then sign it with your wallet. - fn sign_psbt<'a>(&self, psbt: Psbt) -> AsyncResult<'a, Transaction>; + fn sign_psbt<'a>(&'a self, psbt: Psbt) -> AsyncResult<'a, Transaction>; } /// A synchronous version of the [`WalletSource`] trait. Implementations of this trait should be wrapped in @@ -420,7 +420,7 @@ where /// Returns a script to use for change above dust resulting from a successful coin selection attempt. Wraps /// [`WalletSourceSync::get_change_script`]. - fn get_change_script<'a>(&self) -> AsyncResult<'a, ScriptBuf> { + fn get_change_script<'a>(&'a self) -> AsyncResult<'a, ScriptBuf> { let script = self.0.get_change_script(); Box::pin(async move { script }) } @@ -428,7 +428,7 @@ where /// Signs and provides the full [`TxIn::script_sig`] and [`TxIn::witness`] for all inputs within the transaction /// known to the wallet (i.e., any provided via [`WalletSource::list_confirmed_utxos`]). Wraps /// [`WalletSourceSync::sign_psbt`]. - fn sign_psbt<'a>(&self, psbt: Psbt) -> AsyncResult<'a, Transaction> { + fn sign_psbt<'a>(&'a self, psbt: Psbt) -> AsyncResult<'a, Transaction> { let signed_psbt = self.0.sign_psbt(psbt); Box::pin(async move { signed_psbt }) }

I think I removed them because they weren't necessary. But seems better to stick with the same pattern everywhere. If we would have used async traits, they would also always expand to the same thing, I think.

I think I removed them because they weren't necessary. But seems better to stick with the same pattern everywhere. If we would have used async traits, they would also always expand to the same thing, I think.

Well, and just because they are not necessary in for the TestWalletSource, it doesn't mean they aren't needed/helpful elsewhere.

ldk-reviews-bot · 2025-06-04T00:02:37Z

🔔 5th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

joostjager · 2025-06-04T09:44:43Z

Excuse the delay here, finally got to testing this out with LDK Node.

One request for a minor change, otherwise it seems to work as expected with minmal changes (see https://github.com/tnull/ldk-node/tree/2025-02-upgrade-to-ldk-0.2-with-async-bumper)

Looks good that code. I also found myself doing the _inner thing to wrap it easily into async without too much indent.

tnull

LGTM

TheBlueMatt

Two doc nits and one actual comment.

lightning/src/events/bump_transaction.rs

TheBlueMatt · 2025-06-06T12:48:26Z

lightning/src/events/bump_transaction.rs

+	}
+}
+
+impl<W: Deref + MaybeSync + MaybeSend, L: Deref + MaybeSync + MaybeSend> CoinSelectionSource


I believe we should instead implement CoinSelectionSourceSync so that we can use BumpTransactionEventHandlerSync, no?

Oh man, you are totally right. Now I can revert all the asyncification of the tests 😅

I will be back

It seems that the compiler doesn't recognize the drop and complains that the mutex crosses an await (introduced in later commit), even though it doesn't.

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

joostjager · 2025-06-06T15:11:39Z

lightning/src/util/test_utils.rs

@@ -1874,36 +1873,36 @@ impl Drop for TestScorer {

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


Still need this in the sync code it seems, unfortunately.

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

joostjager requested a review from TheBlueMatt May 23, 2025 11:31

tnull mentioned this pull request Jun 3, 2025

Expose AsyncResult type alias #3824

Open

tnull reviewed Jun 3, 2025

View reviewed changes

joostjager force-pushed the async-wallet-bump branch from 534ee90 to 4107b83 Compare June 4, 2025 09:41

joostjager requested a review from tnull June 4, 2025 09:48

tnull previously approved these changes Jun 4, 2025

View reviewed changes

TheBlueMatt reviewed Jun 5, 2025

View reviewed changes

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

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

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

joostjager dismissed tnull’s stale review via ead3a1d June 5, 2025 15:16

joostjager force-pushed the async-wallet-bump branch 2 times, most recently from d8f4c67 to 462afa0 Compare June 6, 2025 08:29

joostjager requested review from TheBlueMatt and tnull June 6, 2025 12:15

TheBlueMatt reviewed Jun 6, 2025

View reviewed changes

joostjager added 3 commits June 6, 2025 17:09

Add scope instead of drop

decddcb

It seems that the compiler doesn't recognize the drop and complains that the mutex crosses an await (introduced in later commit), even though it doesn't.

Rewrite closure do_coin_selection

35ccef6

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

Convert BumpTransactionEventHandler to async

c34c042

joostjager force-pushed the async-wallet-bump branch from 462afa0 to c34c042 Compare June 6, 2025 15:10

joostjager commented Jun 6, 2025

View reviewed changes

joostjager removed the request for review from tnull June 6, 2025 15:33

Async BumpTransactionEventHandler #3752

Are you sure you want to change the base?

Async BumpTransactionEventHandler #3752

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!

ldk-reviews-bot commented May 24, 2025

Uh oh!

ldk-reviews-bot commented May 26, 2025

Uh oh!

ldk-reviews-bot commented May 26, 2025

Uh oh!

ldk-reviews-bot commented May 28, 2025

Uh oh!

ldk-reviews-bot commented May 28, 2025

Uh oh!

ldk-reviews-bot commented May 31, 2025

Uh oh!

ldk-reviews-bot commented May 31, 2025

Uh oh!

ldk-reviews-bot commented Jun 2, 2025

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 commented Jun 4, 2025 •

edited

Loading