Taproot signer variant #2512

arik-so · 2023-08-22T05:42:13Z

No description provided.

codecov-commenter · 2023-08-22T08:26:41Z

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (146a291) 88.55% compared to head (88ce7d6) 88.55%.

Files	Patch %	Lines
lightning/src/util/test_utils.rs	0.00%	3 Missing ⚠️
lightning-invoice/src/utils.rs	71.42%	2 Missing ⚠️
lightning/src/ln/channel.rs	77.77%	2 Missing ⚠️
lightning/src/sign/mod.rs	60.00%	2 Missing ⚠️
lightning/src/sign/type_resolver.rs	80.00%	2 Missing ⚠️
lightning/src/ln/channelmanager.rs	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2512      +/-   ##
==========================================
- Coverage   88.55%   88.55%   -0.01%     
==========================================
  Files         114      114              
  Lines       89417    89421       +4     
  Branches    89417    89421       +4     
==========================================
+ Hits        79186    79187       +1     
+ Misses       7859     7858       -1     
- Partials     2372     2376       +4

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

TheBlueMatt

LGTM.

lightning/src/sign/taproot.rs

fuzz/src/chanmon_consistency.rs

lightning/src/sign/taproot.rs

lightning/src/events/bump_transaction.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/sign/taproot.rs

TheBlueMatt · 2023-10-10T19:14:36Z

Needs rebase.

lightning/src/sign/taproot.rs

wpaulino

Feel free to address these in a follow-up if you'd like.

wpaulino · 2023-11-22T18:00:50Z

lightning/src/sign/taproot.rs

+	/// Generate a local nonce pair, which requires committing to ahead of time
+	/// The counterparty needs the public nonce generated herein to compute a partial signature


Nit: end with periods

wpaulino · 2023-11-22T18:01:01Z

lightning/src/sign/taproot.rs

+	///
+	/// An external signer implementation should check that the commitment has not been revoked.
+	///
+	/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor


Nit: this is stale now

wpaulino · 2023-11-22T18:02:09Z

lightning/src/sign/taproot.rs

+	/// must be be computed using [`SchnorrSighashType::Default`]. Note that this should only be
+	/// used to sign HTLC transactions from channels supporting anchor outputs after all additional
+	/// inputs/outputs have been added to the transaction.


This can be removed, taproot relies on anchors.

wpaulino · 2023-11-27T17:17:53Z

error: unresolved link to `bitcoin::SchnorrSighashType::Default`
   --> lightning/src/sign/taproot.rs:109:39
    |
109 |     /// [`SchnorrSighashType::Default`]: bitcoin::SchnorrSighashType::Default
    |                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no item named `SchnorrSighashType` in module `bitcoin`

TheBlueMatt · 2023-11-27T23:43:40Z

Needs rebased.

For Taproot support, we need to define an alternative trait to EcdsaChannelSigner. This trait will be implemented by all signers that wish to support Taproot channels.

Previously, SignerProvider was not laid out to support multiple signer types. However, with the distinction between ECDSA and Taproot signers, we now need to account for SignerProviders needing to support both. This approach does mean that if ever we introduced another signer type in the future, all implementers of SignerProvider would need to add it as an associated type, and would also need to write a set of dummy implementations for any Signer trait they do not wish to support. For the time being, the TaprootSigner associated type is cfg-gated.

ChannelSignerType is an enum that contains variants of all currently supported signer types. Given that those signer types are enumerated as associated types in multiple places, it is prudent to denote one type as the authority on signer types. SignerProvider seemed like the best option. Thus, instead of ChannelSignerType declaring the associated types itself, it simply uses their definitions from SignerProvider.

To separate out the logic in the `sign` module, which will start to be convoluted with multiple signer types, we're splitting out each signer type into its own submodule, following the taproot.rs example from a previous commit.

TheBlueMatt · 2023-11-28T18:05:26Z

Why is validate_counterparty_revocation only in ECDSA? Shouldn't that be in ChannelSigner, not EcdsaChannelSigner? We can do that move in a followup if you prefer.

TheBlueMatt · 2023-11-28T18:08:21Z

lightning/src/sign/taproot.rs

+	/// An external signer implementation should check that the commitment has not been revoked.
+	///
+	// TODO: Document the things someone using this interface should enforce before signing.
+	fn finalize_holder_commitment(&self, commitment_number: u64,


commitment_number is already present in the HolderCommitmentTransaction object. We shouldn't repeat ourselves here.

TheBlueMatt

Two followups, but nothing blocking.

TheBlueMatt · 2023-11-28T23:25:18Z

Oh, also, please fix the new warnings by adding a #[cfg(taproot)] to all the _ => todo()s added here.

arik-so force-pushed the taproot/2023-08-taproot-signer-variant branch 4 times, most recently from 0da4669 to 8a4e8ed Compare August 22, 2023 19:23

TheBlueMatt mentioned this pull request Aug 22, 2023

Wrapped Channel Signer Type #2441

Merged

arik-so force-pushed the taproot/2023-08-taproot-signer-variant branch 7 times, most recently from 73bb025 to 636624b Compare August 23, 2023 20:52

arik-so marked this pull request as ready for review August 23, 2023 22:10

TheBlueMatt reviewed Aug 23, 2023

View reviewed changes

arik-so force-pushed the taproot/2023-08-taproot-signer-variant branch from cc1b9df to 44f46e7 Compare August 26, 2023 09:27

wpaulino reviewed Aug 28, 2023

View reviewed changes

arik-so force-pushed the taproot/2023-08-taproot-signer-variant branch 5 times, most recently from fc492b3 to 9f2f4de Compare August 29, 2023 16:54

wpaulino reviewed Aug 29, 2023

View reviewed changes

lightning/src/sign/taproot.rs Outdated Show resolved Hide resolved

lightning/src/sign/taproot.rs Outdated Show resolved Hide resolved

lightning/src/sign/taproot.rs Outdated Show resolved Hide resolved

arik-so force-pushed the taproot/2023-08-taproot-signer-variant branch 3 times, most recently from 7057185 to f61ce0d Compare September 6, 2023 03:35

wpaulino reviewed Sep 6, 2023

View reviewed changes

lightning/src/sign/taproot.rs Outdated Show resolved Hide resolved

lightning/src/sign/taproot.rs Outdated Show resolved Hide resolved

wpaulino reviewed Sep 6, 2023

View reviewed changes

lightning/src/sign/taproot.rs Outdated Show resolved Hide resolved

arik-so force-pushed the taproot/2023-08-taproot-signer-variant branch from f61ce0d to 8e222d4 Compare September 6, 2023 19:35

wpaulino previously approved these changes Sep 6, 2023

View reviewed changes

lightning/src/sign/taproot.rs Outdated Show resolved Hide resolved

arik-so force-pushed the taproot/2023-08-taproot-signer-variant branch 2 times, most recently from c026313 to 917eaee Compare September 12, 2023 22:05

arik-so mentioned this pull request Sep 25, 2023

Taproot Pending Requirement Tracking #2295

Open

54 tasks

arik-so force-pushed the taproot/2023-08-taproot-signer-variant branch 2 times, most recently from 8cc2e03 to cf66eb8 Compare November 6, 2023 21:09

TheBlueMatt reviewed Nov 12, 2023

View reviewed changes

lightning/src/sign/taproot.rs Outdated Show resolved Hide resolved

arik-so force-pushed the taproot/2023-08-taproot-signer-variant branch from cf66eb8 to d773914 Compare November 20, 2023 19:54

wpaulino previously approved these changes Nov 22, 2023

View reviewed changes

arik-so dismissed wpaulino’s stale review via f202a6a November 23, 2023 09:07

arik-so force-pushed the taproot/2023-08-taproot-signer-variant branch 3 times, most recently from d065fd5 to f04dc3f Compare November 23, 2023 10:11

arik-so force-pushed the taproot/2023-08-taproot-signer-variant branch from f04dc3f to cc8fa8c Compare November 27, 2023 17:57

wpaulino previously approved these changes Nov 27, 2023

View reviewed changes

arik-so added 5 commits November 27, 2023 16:27

Rename SignerProvider's Signer to EcdsaSigner.

27b9794

Introduce TaprootSigner trait.

f862aa9

For Taproot support, we need to define an alternative trait to EcdsaChannelSigner. This trait will be implemented by all signers that wish to support Taproot channels.

Move ECDSA-specific signers into ecdsa.rs

88ce7d6

To separate out the logic in the `sign` module, which will start to be convoluted with multiple signer types, we're splitting out each signer type into its own submodule, following the taproot.rs example from a previous commit.

arik-so dismissed wpaulino’s stale review via 88ce7d6 November 28, 2023 00:29

arik-so force-pushed the taproot/2023-08-taproot-signer-variant branch from cc8fa8c to 88ce7d6 Compare November 28, 2023 00:29

wpaulino approved these changes Nov 28, 2023

View reviewed changes

TheBlueMatt reviewed Nov 28, 2023

View reviewed changes

TheBlueMatt approved these changes Nov 28, 2023

View reviewed changes

TheBlueMatt merged commit 2659a23 into lightningdevkit:main Nov 28, 2023
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Taproot signer variant #2512

Taproot signer variant #2512

arik-so commented Aug 22, 2023

codecov-commenter commented Aug 22, 2023 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt commented Oct 10, 2023

wpaulino left a comment

wpaulino Nov 22, 2023

wpaulino Nov 22, 2023

wpaulino Nov 22, 2023

wpaulino commented Nov 27, 2023

TheBlueMatt commented Nov 27, 2023

TheBlueMatt commented Nov 28, 2023 •

edited

Loading

TheBlueMatt Nov 28, 2023

TheBlueMatt left a comment

TheBlueMatt commented Nov 28, 2023

		/// Generate a local nonce pair, which requires committing to ahead of time
		/// The counterparty needs the public nonce generated herein to compute a partial signature

Taproot signer variant #2512

Taproot signer variant #2512

Conversation

arik-so commented Aug 22, 2023

codecov-commenter commented Aug 22, 2023 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Oct 10, 2023

wpaulino left a comment

Choose a reason for hiding this comment

wpaulino Nov 22, 2023

Choose a reason for hiding this comment

wpaulino Nov 22, 2023

Choose a reason for hiding this comment

wpaulino Nov 22, 2023

Choose a reason for hiding this comment

wpaulino commented Nov 27, 2023

TheBlueMatt commented Nov 27, 2023

TheBlueMatt commented Nov 28, 2023 • edited Loading

TheBlueMatt Nov 28, 2023

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Nov 28, 2023

codecov-commenter commented Aug 22, 2023 •

edited

Loading

TheBlueMatt commented Nov 28, 2023 •

edited

Loading