-
Notifications
You must be signed in to change notification settings - Fork 411
Async BumpTransactionEventHandler #3752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Async BumpTransactionEventHandler #3752
Conversation
👋 I see @tnull was un-assigned. |
d9b1c75
to
873516b
Compare
Codecov ReportAttention: Patch coverage is
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. 🚀 New features to boost your workflow:
|
4b4b898
to
3816c66
Compare
1a86521
to
e1509f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a first / quickish pass. Generally looks good so far.
lightning/Cargo.toml
Outdated
@@ -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 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please set default-features = false
here and below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. Why is this? I looked at the tokio crate and it seems there are no default features?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
*commitment_tx_fee_satoshis, | ||
anchor_descriptor, | ||
) { | ||
if let Err(_) = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() { .. }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also tried with map_err
, but that doesn't work all that well with the parent fn without a return result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this is nicer. Wasn't aware of the unwrap_or_else on result. Updated.
lightning/src/ln/functional_tests.rs
Outdated
#[allow(clippy::expect_used, clippy::diverging_sub_expression)] | ||
{ | ||
return tokio::runtime::Builder::new_current_thread() | ||
.enable_all() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed then. If we get more of these xtests, we might need to duplicate the macro or something like that.
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 | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Indent off here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the indent off. The closing parenthesis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the parenthesis and all arguments of log_debug
too (they need to be indented by one tab).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, happened in the next commit. Fixed now.
👋 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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically LGTM, some nits/comments.
Also double-checked this still works with LDK Node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This preparatory commit converts do_coin_selection from a closure into a standard function.
93e5d7e
to
2fdcf05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to squash fixups from my side.
2fdcf05
to
94bbb5c
Compare
Squashed |
94bbb5c
to
0ca7739
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -11,6 +11,8 @@ | |||
//! | |||
//! [`Event`]: crate::events::Event | |||
|
|||
pub mod sync; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Links added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am getting to see more of how rustdoc works with this PR. It's quite nice.
where | ||
T::Target: WalletSourceSync, | ||
{ | ||
/// Returns all UTXOs, with at least 1 confirmation each, that are available to spend. Wraps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
} | ||
} | ||
|
||
/// A synchronous helper trait that can be used to implement [`CoinSelectionSource`] in a synchronous |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
/// 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, no longer necessary. Removed.
where | ||
T::Target: CoinSelectionSourceSync, | ||
{ | ||
#[allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, seems this came back after it had already been dropped? #3752 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't remove all of it apparently. And also found a new
for WalletSourceSyncWrapper
that doesn't add much. Removed as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly meant the dead_code cause its used now, but removing the new method is also nice.
@@ -0,0 +1,241 @@ | |||
// This file is Copyright its original authors, visible in version control |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It seems more sensible to move bump_transaction.rs
to bump_transaction/mod.rs
to keep it and sync.rs
together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
0ca7739
to
8b0fd27
Compare
Comments addressed, commit message expanded. Ready for another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixups look good to me. Could be squashed from my side, once Matt saw them, too.
Feel free to squash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
8b0fd27
to
0820322
Compare
Squashed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -1874,36 +1873,36 @@ impl Drop for TestScorer { | |||
|
|||
pub struct TestWalletSource { | |||
secret_key: SecretKey, | |||
utxos: RefCell<Vec<Utxo>>, | |||
utxos: Mutex<Vec<Utxo>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :/
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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely prefer to leave Arc::clone
s over .clone()
s. They're way more readable :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel it. Maybe that feeling needs time to develop 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
Converts
BumpTransactionEventHandler
to async.Changes the
CoinSelectionSource
andWalletSource
traits to be async and providesWalletSourceSyncWrapper
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