Skip to content
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

Remove the async_signing cfg flag #3486

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

TheBlueMatt
Copy link
Collaborator

Now that the core features required for async_signing are in
place, we can go ahead and expose it publicly (rather than behind a
a cfg-flag). We still don't have full async support for
get_per_commitment_point, but only one case in channel
reconnection remains. The overall logic may still have some
hiccups, but its been in use in production at a major LDK user for
some time now. Thus, it doesn't really make sense to hide behind a
cfg-flag, even if the feature is only 99% complete. Further, the
new paths exposed are very restricted to signing operations that
run async, so the risk for existing users should be incredibly low.

Now that the core features required for `async_signing` are in
place, we can go ahead and expose it publicly (rather than behind a
a `cfg`-flag). We still don't have full async support for
`get_per_commitment_point`, but only one case in channel
reconnection remains. The overall logic may still have some
hiccups, but its been in use in production at a major LDK user for
some time now. Thus, it doesn't really make sense to hide behind a
`cfg`-flag, even if the feature is only 99% complete. Further, the
new paths exposed are very restricted to signing operations that
run async, so the risk for existing users should be incredibly low.
@TheBlueMatt TheBlueMatt added this to the 0.1 milestone Dec 16, 2024
lightning/src/sign/ecdsa.rs Outdated Show resolved Hide resolved
lightning/src/sign/ecdsa.rs Show resolved Hide resolved
lightning/src/sign/ecdsa.rs Outdated Show resolved Hide resolved
lightning/src/sign/ecdsa.rs Outdated Show resolved Hide resolved
We should update the return types on the signing methods here as
well, but we should at least start by documenting which methods are
async and which are not.

Once we complete async support for `get_per_commitment_point`, we
can change the return types as most things in the channel signing
traits will be finalized.
@TheBlueMatt
Copy link
Collaborator Author

Squash-pushed to address the comments:

$ git diff-tree -U2 6a8bf7694 d2172e389
diff --git a/lightning/src/sign/ecdsa.rs b/lightning/src/sign/ecdsa.rs
index b1bec057d..2f42d3329 100644
--- a/lightning/src/sign/ecdsa.rs
+++ b/lightning/src/sign/ecdsa.rs
@@ -37,6 +37,4 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	/// Create a signature for a counterparty's commitment transaction and associated HTLC transactions.
 	///
-	/// Note that if signing fails or is rejected, the channel will be force-closed.
-	///
 	/// Policy checks should be implemented in this function, including checking the amount
 	/// sent to us and checking the HTLCs.
@@ -227,5 +225,5 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	/// signature is computed through [`NodeSigner::sign_gossip_message`].
 	///
-	/// This method is *not* asynchronous, if this fails or is rejected, the channel will not be
+	/// This method is *not* asynchronous. If an `Err` is returned, the channel will not be
 	/// publicly announced and our counterparty may (though likely will not) close the channel on
 	/// us for violating the protocol.
@@ -246,5 +244,5 @@ pub trait EcdsaChannelSigner: ChannelSigner {
 	/// `input_value`: The value of the previous funding transaction output.
 	///
-	/// THis method is *not* asynchronous. If an `Err` is returned, the channel will be immediately
+	/// This method is *not* asynchronous. If an `Err` is returned, the channel will be immediately
 	/// closed.
 	fn sign_splicing_funding_input(

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.43%. Comparing base (d1e94bd) to head (d2172e3).
Report is 42 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3486      +/-   ##
==========================================
- Coverage   89.70%   89.43%   -0.28%     
==========================================
  Files         130      150      +20     
  Lines      107882   117600    +9718     
  Branches   107882   117600    +9718     
==========================================
+ Hits        96778   105174    +8396     
- Misses       8695     9966    +1271     
- Partials     2409     2460      +51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +30 to +32
/// [`ChannelManager::signer_unblocked`] or [`ChainMonitor::signer_unblocked`] (see individual
/// method documentation for which method should be called) once the result is ready, at which
/// point the channel operation will resume.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that ChannelManager::signer_unblocked docs sound like it needs to be called whenever any of these methods errors. It's clear in these docs which signer_unblocked method needs to be called, but there it could be clearer and {Chain,Channel}Monitor::signer_unblocked could also benefit from referencing these docs IMO. Doesn't need to happen in this PR though.

@TheBlueMatt TheBlueMatt merged commit 42cc4e7 into lightningdevkit:main Dec 17, 2024
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants