Support async signing for V2 channel establishment #3411

jkczyz · 2024-11-15T23:02:38Z

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. The user calls ChannelManager::signer_unblocked to indicate that signing is complete, which will attempt to finish handling the tx_complete message again.

Based on #3137.
Fixes #3404.

dunxen

Seems good to me, but I'm not 100% familiar with the async signing stuff. Seems mostly straightforward though.

dunxen · 2024-11-18T17:23:16Z

lightning/src/ln/channel.rs

@@ -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>,


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.

dunxen · 2024-11-18T17:30:15Z

lightning/src/ln/channelmanager.rs

+		// 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.


This can be done with the ChannelPhase into Channel refactor, right?

Yup, that's what the comment is referring to.

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.

The errs Vec in ChannelManager::claim_payment_internal hasn't been populated since commit fea6393. Drop it along with the code that consumed it.

jkczyz · 2024-11-21T20:31:23Z

Fixed a couple rustfmt issues in dual_funding_tests.rs. Also, removed a ton of unnecessary drains from channelmanager.rs for good measure since I was looking at related code and noticed a Vec wasn't being used, which I removed in another commit, too.

codecov · 2024-11-21T20:34:00Z

Codecov Report

Attention: Patch coverage is 66.91176% with 45 lines in your changes missing coverage. Please review.

Project coverage is 89.23%. Comparing base (0c31021) to head (6db89d7).

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	64.61%	16 Missing and 7 partials ⚠️
lightning/src/ln/channel.rs	62.50%	20 Missing and 1 partial ⚠️
lightning/src/ln/interactivetxs.rs	50.00%	1 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

lightning/src/ln/channel.rs

optout21 · 2024-11-25T10:33:40Z

lightning/src/ln/channel.rs

-							"Failed to get signatures for new commitment_signed".to_owned(),
-							ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) },
-						)))
+					.ok()


What's the reasoning for discarding error info here? The signer may return an error.

Signer returning an error indicates async signing. At least that's how the other async signing operates. Looks like the EcdsaChannelSigner::sign_counterparty_commitment docs weren't updated like ChannelSigner::get_per_commitment_point were. Let me do that in this PR.

@TheBlueMatt Wonder if we should change the interface to use an Option instead of a Resultonce async_signing configuration parameter is dropped?

We should probably clean up the API, yea, but lets not conflict with #3109 if at all possible.

optout21 · 2024-11-25T10:37:24Z

lightning/src/ln/channelmanager.rs

 						ChannelPhase::UnfundedOutboundV2(chan) => {
-							chan.funding_tx_constructed(&mut signing_session, &self.logger)
+							*chan.interactive_tx_signing_session_mut() = Some(signing_session);


Maybe a setter method would be more idiomatic in this case.

TheBlueMatt · 2024-11-25T16:57:54Z

lightning/src/ln/channelmanager.rs


+macro_rules! finish_tx_complete {


Would this be more straightforward if we do the state transition immediately and only send the message later?

Hmm... possibly. Maybe we should wait on #3423 before moving forward with this PR?

I'm fine with that, might also reduce the risk of #3109 being conflicted.

TheBlueMatt · 2024-11-25T16:58:49Z

lightning/src/ln/channelmanager.rs

+			None => {
+				let per_peer_state = self.per_peer_state.read().unwrap();
+				let mut channels = Vec::with_capacity(per_peer_state.len());
+				for (counterparty_node_id, peer_state_mutex) in per_peer_state.iter() {


Not sure I understand why we need to do a two-pass here, we don't drop the locks during the loop in the second pass so we should be able to just do a straight iteration?

finish_tx_complete! removes and re-inserts the entry, which you can't do while iterating. See TODO above that loop.

jkczyz added the blocked on dependent pr label Nov 15, 2024

jkczyz requested review from TheBlueMatt and dunxen November 15, 2024 23:05

dunxen reviewed Nov 18, 2024

View reviewed changes

jkczyz mentioned this pull request Nov 18, 2024

Implement accepting dual-funded channels without contributing #3137

Merged

jkczyz added 4 commits November 20, 2024 11:59

Support async signing for V2 channel establishment

846faf6

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.

Remove Result from into_channel methods

53fc264

These methods always return Ok, so there is no need to use a Result.

Finish handling tx_complete in signer_unblocked

92018f6

Expand ChannelManager::signer_unblocked to finish handling tx_complete messages. This completes support for async signing in V2 channel establishment.

jkczyz force-pushed the 2024-11-async-signing-v2 branch from c3f5063 to 8dfa6d2 Compare November 20, 2024 18:10

jkczyz marked this pull request as ready for review November 20, 2024 18:10

jkczyz removed the blocked on dependent pr label Nov 21, 2024

jkczyz added 4 commits November 21, 2024 14:25

Test async signing in V2 channel establishment

405f3ad

Remove unnecessary clone and collect

c8bbcff

Remove unused errs Vec when claiming payments

770e161

The errs Vec in ChannelManager::claim_payment_internal hasn't been populated since commit fea6393. Drop it along with the code that consumed it.

Remove unnecessary drains from channelmanager.rs

6db89d7

jkczyz force-pushed the 2024-11-async-signing-v2 branch from 8dfa6d2 to 6db89d7 Compare November 21, 2024 20:25

optout21 reviewed Nov 25, 2024

View reviewed changes

lightning/src/ln/channel.rs Show resolved Hide resolved

optout21 reviewed Nov 25, 2024

View reviewed changes

TheBlueMatt reviewed Nov 25, 2024

View reviewed changes

This was referenced Dec 2, 2024

Handle fallible per commitment point in channel establishment #3109

Open

Follow-ups to PR 3137 #3423

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support async signing for V2 channel establishment #3411

Support async signing for V2 channel establishment #3411

jkczyz commented Nov 15, 2024 •

edited

Loading

dunxen left a comment

dunxen Nov 18, 2024

dunxen Nov 18, 2024

jkczyz Nov 18, 2024

dunxen Nov 18, 2024

jkczyz commented Nov 21, 2024

codecov bot commented Nov 21, 2024

optout21 Nov 25, 2024

jkczyz Nov 25, 2024

TheBlueMatt Dec 2, 2024

optout21 Nov 25, 2024

TheBlueMatt Nov 25, 2024

jkczyz Nov 25, 2024

TheBlueMatt Dec 2, 2024

TheBlueMatt Nov 25, 2024

jkczyz Nov 25, 2024


		macro_rules! finish_tx_complete {

Support async signing for V2 channel establishment #3411

Are you sure you want to change the base?

Support async signing for V2 channel establishment #3411

Conversation

jkczyz commented Nov 15, 2024 • edited Loading

dunxen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Nov 21, 2024

codecov bot commented Nov 21, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Nov 15, 2024 •

edited

Loading