-
Notifications
You must be signed in to change notification settings - Fork 367
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
Support async signing for V2 channel establishment #3411
base: main
Are you sure you want to change the base?
Conversation
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.
Seems good to me, but I'm not 100% familiar with the async signing stuff. Seems mostly straightforward though.
@@ -8862,6 +8891,7 @@ pub(super) struct InboundV2Channel<SP: Deref> where SP::Target: SignerProvider { | |||
pub dual_funding_context: DualFundingChannelContext, | |||
/// The current interactive transaction construction session under negotiation. | |||
interactive_tx_constructor: Option<InteractiveTxConstructor>, | |||
signing_session: Option<InteractiveTxSigningSession>, |
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, honestly I'm not sure how to avoid the Option
without introducing a new state variant, which is something we'd prefer to avoid, as discussed.
// Finish any tx_complete handling waiting on async signing. | ||
// | ||
// TODO: Move this into the earlier channel iteration to avoid duplication and the Vec | ||
// allocation once ChannelPhase is refactored into Channel. This can't be avoided with the | ||
// current data model because tx_complete handling requires removing the entry from the | ||
// channel_by_id map and re-inserting it, which can't be done while iterating over the map. |
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.
This can be done with the ChannelPhase into Channel
refactor, right?
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.
Yup, that's what the comment is referring to.
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.
Oh i missed a whole line when reading. Stated the exact same thing 🤦♂️
To support async signing, the InteractiveTxSigningSession returned when handling tx_complete needs to be saved in order to retry the funding_tx_constructed method when the signer is unblocked. This unfortunately means an Option as needed since the unfunded V2 channel phase variant is left unfunded until the signer completes.
When handling a tx_complete message, allow signers to return an error indicating that the signer has not yet complete. This will leave the ChannelPhase in an unfunded variant until the signer becomes unblocked.
These methods always return Ok, so there is no need to use a Result.
Expand ChannelManager::signer_unblocked to finish handling tx_complete messages. This completes support for async signing in V2 channel establishment.
c3f5063
to
8dfa6d2
Compare
The errs Vec in ChannelManager::claim_payment_internal hasn't been populated since commit fea6393. Drop it along with the code that consumed it.
8dfa6d2
to
6db89d7
Compare
Fixed a couple |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3411 +/- ##
==========================================
+ Coverage 89.22% 89.23% +0.01%
==========================================
Files 130 130
Lines 106965 106958 -7
Branches 106965 106958 -7
==========================================
+ Hits 95438 95446 +8
+ Misses 8734 8725 -9
+ Partials 2793 2787 -6 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
When handling a
tx_complete
message, allow signers to return an error indicating that the signer has not yet complete. This will leave theChannelPhase
in an unfunded variant until the signer becomes unblocked. The user callsChannelManager::signer_unblocked
to indicate that signing is complete, which will attempt to finish handling thetx_complete
message again.Based on #3137.
Fixes #3404.