Create Taproot feature and introduce signer type enum #2289

arik-so · 2023-05-11T17:00:49Z

No description provided.

TheBlueMatt · 2023-05-11T19:50:55Z

lightning/src/sign/taproot.rs

+use crate::ln::PaymentPreimage;
+use crate::sign::ChannelSigner;
+
+pub trait TaprootSigner: ChannelSigner {


We should add an implementation in the same pr, no? There's a bunch of TODOs here related to the new code - even if it's behind a feature flag let's commit complete code.

TheBlueMatt · 2023-05-11T19:51:33Z

lightning/src/sign/taproot.rs

+use crate::sign::ChannelSigner;
+
+pub trait TaprootSigner: ChannelSigner {
+	fn generate_local_nonce_pair(&self, secp_ctx: &Secp256k1<secp256k1::All>) -> PublicNonce;


Should we have some kind of id here so that later references to the Dane nonce pair can use the same id for derivation? Also this needs docs.

TheBlueMatt · 2023-05-11T19:52:00Z

lightning/src/sign/mod.rs

@@ -53,6 +53,9 @@ use crate::util::atomic_counter::AtomicCounter;
 use crate::util::chacha20::ChaCha20;
 use crate::util::invoice::construct_invoice_preimage;

+#[cfg(all(anchors, taproot))]


We should add a CI test of the new flag.

TheBlueMatt · 2023-05-11T19:59:55Z

lightning/src/sign/taproot.rs

+	/// revoked the state which they eventually broadcast. It's not a _holder_ secret key and does
+	/// not allow the spending of any funds by itself (you need our holder `revocation_secret` to do
+	/// so).
+	fn sign_justice_revoked_output(&self, justice_tx: &Transaction, input: usize, amount: u64,


I know the existing signer doesn't but can we specify in these docs exactly when we call all these methods?

ariard · 2023-05-12T00:58:39Z

lightning/src/ln/features.rs

@@ -384,6 +384,9 @@ mod sealed {
 	define_feature!(27, ShutdownAnySegwit, [InitContext, NodeContext],
 		"Feature flags for `opt_shutdown_anysegwit`.", set_shutdown_any_segwit_optional,
 		set_shutdown_any_segwit_required, supports_shutdown_anysegwit, requires_shutdown_anysegwit);
+	define_feature!(31, Taproot, [InitContext, NodeContext, ChannelTypeContext],


I think the extension-bolt should be updated to mark INC: https://github.com/lightning/bolts/pull/995/files#diff-5005c17d0f8887bea09bff54878cb0bdb2bb3d632b1813a2622560e842a0d322R362

ariard · 2023-05-12T01:10:14Z

lightning/src/sign/taproot.rs

+	/// Note that all the relevant preimages will be provided, but there may also be additional
+	/// irrelevant or duplicate preimages.
+	//
+	// TODO: Document the things someone using this interface should enforce before signing.


The signer should verify we’re spending a P2TR.

ariard · 2023-05-12T01:13:32Z

lightning/src/sign/taproot.rs

+	// TODO: Document the things someone using this interface should enforce before signing.
+	fn partially_sign_counterparty_commitment(&self, commitment_tx: &CommitmentTransaction,
+		preimages: Vec<PaymentPreimage>, secp_ctx: &Secp256k1<secp256k1::All>,
+	) -> Result<(PartialSignature, Vec<Signature>), ()>;


Do we expect this method to return Signature for each, if yes also the inclusion_proof for the script path as we expect this method to be use internally by LDK for on-chain claims?

ariard · 2023-05-12T01:17:03Z

lightning/src/sign/taproot.rs

+	/// It may be called multiple times for same output(s) if a fee-bump is needed with regards
+	/// to an upcoming timelock expiration.
+	///
+	/// Amount is value of the output spent by this input, committed to in the BIP 143 signature.


BIP341 signature if we’re talking about PT2R outputs no ?

TheBlueMatt · 2023-05-12T04:16:37Z

CC @devrandom any idea what y'all need on the signing API side for VLS?

TheBlueMatt · 2023-05-12T04:17:00Z

Or @ksedgwic

arik-so · 2023-05-12T04:34:53Z

FYI, everyone, slow down a bit. This PR is a draft for a reason. I'm gonna have tons of revisions to the signer trait. In fact, I think I'll move it to a separate PR.

codecov-commenter · 2023-05-17T16:49:03Z

Codecov Report

Patch coverage: 97.56% and project coverage change: +0.46 🎉

Comparison is base (818dbdf) 91.50% compared to head (b8ee6bd) 91.96%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2289      +/-   ##
==========================================
+ Coverage   91.50%   91.96%   +0.46%     
==========================================
  Files         104      105       +1     
  Lines       52087    69041   +16954     
  Branches    52087    69041   +16954     
==========================================
+ Hits        47660    63491   +15831     
- Misses       4427     5550    +1123

Impacted Files	Coverage Δ
lightning/src/ln/features.rs	`97.40% <ø> (-1.06%)`	⬇️
lightning/src/sign/mod.rs	`91.53% <ø> (+2.71%)`	⬆️
lightning/src/ln/channel.rs	`92.72% <96.77%> (+2.91%)`	⬆️
lightning/src/sign/type_resolver.rs	`100.00% <100.00%> (ø)`

... and 58 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

arik-so · 2023-05-17T17:22:58Z

Ok, I decided to move the Taproot trait definition into a separate PR, which will also add a variant to the signer type enum introduced here.

lightning/src/sign/type_resolver.rs

wpaulino · 2023-05-18T14:28:48Z

lightning/src/sign/type_resolver.rs

+}
+
+/// Helper trait for accessing common channel signer methods between different implementations
+pub trait AsChannelSigner {


I don't think this needs to be public, right?

Oh I guess it doesn't matter since we make the module pub(crate).

It actually does, unfortunately. Rust is complaining that ChannelSigner (public) inherits from a trait that would otherwise have a lower visibility, which is a no-no. Thankfully, the compiler doesn't yet detect that that's meaningless. So 🤫

I think this means a downstream crate is unable to implement ChannelSigner (you've sealed it). Instead, you can impl<C: ChannelSigner> AsChannelSigner for C and not inherit.

Not inheriting will break the functionality, unfortunately. It's a crucial aspect. But I have a different idea.

Why is the inheritance critical here? Maybe we don't land this PR until we get a few more commits so that reviewers can better understand where its going?

Because without inheritance, this won't work: https://stackoverflow.com/questions/28632968/why-doesnt-rust-support-trait-object-upcasting/28664881#28664881

Huh, TIL, but you can take that sample and simply remove the extension and it still compiles?

trait Base { fn a(&self) -> (); } trait AsBase { fn as_base(&self) -> &dyn Base; } impl<T: Base> AsBase for T { fn as_base(&self) -> &dyn Base { self } }

Here's a more self-contained example of what we're trying to achieve: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4d9d295a47d6da11e2fd58c27ffc3fda

The issue is the Bar(Box<dyn Bar>), which seems like it should be fixed instead of adding a new public trait. ISTM we should just include additional commits here so that the whole context can be reviewed, rather than reviewing something in part that doesn't make sense on its own.

lightning/src/ln/features.rs

lightning/src/sign/type_resolver.rs

TheBlueMatt · 2023-05-21T17:12:14Z

lightning/src/sign/type_resolver.rs

+}
+
+/// Helper trait for accessing common channel signer methods between different implementations
+pub trait AsChannelSigner {


I think this means a downstream crate is unable to implement ChannelSigner (you've sealed it). Instead, you can impl<C: ChannelSigner> AsChannelSigner for C and not inherit.

TheBlueMatt · 2023-05-21T17:12:48Z

lightning/src/ln/channel.rs

@@ -4196,7 +4197,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {

 		let channel_ready = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.cur_holder_commitment_transaction_number == 1 {
 			// We should never have to worry about MonitorUpdateInProgress resending ChannelReady
-			let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx);
+			let next_per_commitment_point = self.holder_signer.as_ref().get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx);


While you're at it can we clean up a lot of the long lines in this file (that are now worse).

dunxen · 2023-06-20T09:00:32Z

lightning/src/ln/features.rs

@@ -384,6 +384,9 @@ mod sealed {
 	define_feature!(27, ShutdownAnySegwit, [InitContext, NodeContext],
 		"Feature flags for `opt_shutdown_anysegwit`.", set_shutdown_any_segwit_optional,
 		set_shutdown_any_segwit_required, supports_shutdown_anysegwit, requires_shutdown_anysegwit);
+	define_feature!(31, Taproot, [InitContext, NodeContext, ChannelTypeContext],


Should we add a TODO(taproot) to the module docs above to remember to document support for simple taproot channels when the time comes?

arik-so · 2024-01-10T00:19:07Z

Implemented in another PR.

arik-so changed the title ~~Create Taproot signer and feature~~ Create Taproot feature and define signer trait May 11, 2023

TheBlueMatt reviewed May 11, 2023

View reviewed changes

ariard reviewed May 12, 2023

View reviewed changes

arik-so force-pushed the 2023-05-taproot-signer branch from 28e4d38 to 1c80b04 Compare May 15, 2023 23:02

wpaulino self-requested a review May 16, 2023 17:46

arik-so force-pushed the 2023-05-taproot-signer branch 3 times, most recently from 5abe515 to fcd0322 Compare May 17, 2023 07:31

arik-so changed the title ~~Create Taproot feature and define signer trait~~ Create Taproot feature and introduce signer type enum May 17, 2023

arik-so marked this pull request as ready for review May 17, 2023 17:22

wpaulino reviewed May 18, 2023

View reviewed changes

Add Taproot feature support.

65bf87b

arik-so force-pushed the 2023-05-taproot-signer branch from fcd0322 to cc6add5 Compare May 18, 2023 16:19

wpaulino previously approved these changes May 18, 2023

View reviewed changes

22388o mentioned this pull request May 20, 2023

Release Q3 2023 AreaLayer/javascript-dlc#1

Closed

13 tasks

TheBlueMatt reviewed May 21, 2023

View reviewed changes

dunxen self-requested a review May 23, 2023 09:54

arik-so dismissed wpaulino’s stale review via 8135406 May 24, 2023 19:53

arik-so mentioned this pull request May 25, 2023

Taproot Pending Requirement Tracking #2295

Open

54 tasks

arik-so added 2 commits June 1, 2023 17:00

Create ChannelSignerType enum.

e369bbf

Make as_ecdsa return types optional.

b8ee6bd

arik-so force-pushed the 2023-05-taproot-signer branch from 8135406 to b8ee6bd Compare June 2, 2023 00:02

dunxen reviewed Jun 20, 2023

View reviewed changes

arik-so mentioned this pull request Jul 21, 2023

Wrapped Channel Signer Type #2441

Merged

arik-so closed this Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Taproot feature and introduce signer type enum #2289

Create Taproot feature and introduce signer type enum #2289

arik-so commented May 11, 2023

TheBlueMatt May 11, 2023

TheBlueMatt May 11, 2023

TheBlueMatt May 11, 2023

TheBlueMatt May 11, 2023

ariard May 12, 2023

ariard May 12, 2023

ariard May 12, 2023

ariard May 12, 2023

TheBlueMatt commented May 12, 2023

TheBlueMatt commented May 12, 2023

arik-so commented May 12, 2023

codecov-commenter commented May 17, 2023 •

edited

arik-so commented May 17, 2023

wpaulino May 18, 2023

wpaulino May 18, 2023

arik-so May 18, 2023

TheBlueMatt May 21, 2023

arik-so May 21, 2023

TheBlueMatt May 21, 2023

arik-so May 24, 2023

TheBlueMatt Jun 1, 2023

wpaulino Jun 2, 2023

TheBlueMatt Jun 3, 2023

TheBlueMatt May 21, 2023

TheBlueMatt May 21, 2023

dunxen Jun 20, 2023

arik-so commented Jan 10, 2024

Create Taproot feature and introduce signer type enum #2289

Create Taproot feature and introduce signer type enum #2289

Conversation

arik-so commented May 11, 2023

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

TheBlueMatt commented May 12, 2023

TheBlueMatt commented May 12, 2023

arik-so commented May 12, 2023

codecov-commenter commented May 17, 2023 • edited

Codecov Report

arik-so commented May 17, 2023

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

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

arik-so commented Jan 10, 2024

codecov-commenter commented May 17, 2023 •

edited