Wrapped Channel Signer Type #2441

arik-so · 2023-07-21T18:58:43Z

Supersedes #2289.

TheBlueMatt

Mostly good

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2023-08-14T22:39:42Z

lightning/src/ln/functional_tests.rs

@@ -738,7 +738,7 @@ fn test_update_fee_that_funder_cannot_afford() {
 			&mut htlcs,
 			&local_chan.context.channel_transaction_parameters.as_counterparty_broadcastable()
 		);
-		local_chan_signer.sign_counterparty_commitment(&commitment_tx, Vec::new(), &secp_ctx).unwrap()
+		local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment(&commitment_tx, Vec::new(), &secp_ctx).unwrap()


Should we be unwraping here? Shouldn't we do a get and then Err if its wrong (presumably taproot will use different messages?

I believe it's a panic-worthy exception because as_ecdsa() should never even be called within a Taproot impl context. But happy to adjust, or to replace it with an expect().

Well, its obviously unreachable today, but looking forward ISTM it will be reachable, so worth handling.

Now that I'm about to address this, I'm noticing that this particular comment was left in a test. Are you saying we should be matching Errs in unit tests for this, too? I would think that an expect() should be enough, no?

lightning/src/sign/type_resolver.rs

codecov-commenter · 2023-08-16T16:45:56Z

Codecov Report

Patch coverage: 95.20% and no project coverage change.

Comparison is base (2b898a3) 90.58% compared to head (805e825) 90.58%.
Report is 30 commits behind head on main.

❗ Current head 805e825 differs from pull request most recent head 6a2f43d. Consider uploading reports for the commit 6a2f43d to get more accurate results

❗ 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    #2441   +/-   ##
=======================================
  Coverage   90.58%   90.58%           
=======================================
  Files         106      107    +1     
  Lines       56467    56479   +12     
  Branches    56467    56479   +12     
=======================================
+ Hits        51149    51161   +12     
  Misses       5318     5318

Files Changed	Coverage Δ
lightning-background-processor/src/lib.rs	`82.67% <ø> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`98.72% <ø> (ø)`
lightning/src/ln/features.rs	`98.05% <ø> (ø)`
lightning/src/ln/monitor_tests.rs	`98.52% <ø> (ø)`
lightning/src/ln/priv_short_conf_tests.rs	`97.71% <ø> (ø)`
lightning/src/ln/reorg_tests.rs	`100.00% <ø> (ø)`
lightning/src/sign/mod.rs	`83.89% <ø> (ø)`
lightning/src/ln/channel.rs	`89.77% <93.60%> (-0.02%)`	⬇️
lightning/src/ln/channelmanager.rs	`87.11% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`98.15% <100.00%> (-0.02%)`	⬇️
... and 2 more

... and 3 files with indirect coverage changes

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

TheBlueMatt · 2023-08-21T17:58:13Z

Can you squash fixup commits and mark this not-draft if you're finished with it?

arik-so · 2023-08-22T01:51:17Z

It would be much easier to undraft if every rebase on main didn't immediately break the lifetimes.

arik-so · 2023-08-22T03:08:17Z

Only true fixup commits I'm seeing are the match arm one and the EcdsaChannelSigner type. The other fixes are independent of, but necessary for, the changes in this PR.

lightning-background-processor/src/lib.rs

lightning/src/ln/channel.rs

wpaulino · 2023-08-22T19:23:47Z

lightning/src/ln/channel.rs

@@ -6631,7 +6665,8 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for Channel<Signer> {
 		self.context.latest_monitor_update_id.write(writer)?;

 		let mut key_data = VecWriter(Vec::new());
-		self.context.holder_signer.write(&mut key_data)?;
+		// TODO: Introduce serialization distinction for non-ECDSA signers.
+		self.context.holder_signer.as_ecdsa().expect("Only ECDSA signers may be serialized").write(&mut key_data)?;


We should be checking the type here with if let rather than asserting. The TODO seems irrelevant since we won't be serializing other signer types? We'd just want to make sure we fail to read new signer types on old versions, which should already be handled with ChannelTypeFeatures.

We will need to specify for other types though that the serialization data is to be skipped, which is slightly more involved. For that reason I think unwrap is correct here for the time being.

Can you mark the new TODOs that we need to address in #2512 in some way so that we can easily grep to ensure we've fixed all of them?

wpaulino · 2023-08-22T19:28:15Z

lightning/src/sign/type_resolver.rs

+		}
+	}
+
+	pub(crate) fn as_mut_ecdsa(&mut self) -> Option<&mut ECS> {


Nit: this is unused at the moment, maybe remove it?

I think this method should stay just such that external contributors can have access to it.

TheBlueMatt

LGTM, but please write actual commit messages, minimum two paragraphs and a title each.

TheBlueMatt · 2023-08-22T19:34:24Z

lightning/src/ln/functional_tests.rs

 	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
-	let (persister, chain_monitor, nodes_0_deserialized);
+	let nodes_0_deserialized;


Can you move this commit up to the top?

The persister and chain_monitor variables must be declared before the node channel manager is initialized to avoid out of order deallocation.

Introduce a Taproot feature on bits 30/31 for initialization, node, and channel type contexts.

Benchmarks were failing because node config and channel monitor configs were tied to the same lifetime. Introducing a separate lifetime allows to avoid out-of-order deallocation errors.

Rather than using a holder_signer of a specific signer type in Channel and ChannelContext, this allows us to hold an enum such that depending on the type of channel, the appropriate signer could be held in its respective variant. Doing so required the reparametrization of Channel from using a Signer to using the SignerProvider trait. This percolated down to the ChannelManager and multiple tests. Now, when accessign various signer methods, there is a distinction between accessing methods defined for all signers on ChannelSigner, and accessing type-specific methods using accessors such as `as_ecdsa`.

Remove a bunch of unnecessary ChannelManager imports.

devrandom · 2023-08-22T22:10:04Z

lightning/src/ln/channel.rs

-		).map_err(|_| ChannelError::Close("Failed to validate revocation from peer".to_owned()))?;
+		match &self.context.holder_signer {
+			ChannelSignerType::Ecdsa(ecdsa) => {
+				ecdsa.validate_counterparty_revocation(


isn't this fn the same for ecdsa and taproot?

unfortunately not, because of the nonce that needs to be processed in the Taproot variant

TheBlueMatt · 2023-08-22T23:40:18Z

lightning/src/sign/type_resolver.rs

@@ -0,0 +1,32 @@
+use crate::sign::{ChannelSigner, EcdsaChannelSigner};
+
+pub(crate) enum ChannelSignerType<ECS: EcdsaChannelSigner> {


I guess the anticipated next step is to move this bound to just be a SignerProvider to simplify the bounds when we add the taproot variant?

oooh, that's an interesting idea! I'll give that a try.

arik-so force-pushed the 2023-07-taproot-signer-wrapped branch 3 times, most recently from 5a0b475 to 4c05087 Compare July 21, 2023 23:23

TheBlueMatt reviewed Aug 14, 2023

View reviewed changes

arik-so force-pushed the 2023-07-taproot-signer-wrapped branch from b2b0239 to a6f5df6 Compare August 15, 2023 06:35

TheBlueMatt mentioned this pull request Aug 15, 2023

Correct test lifetimes #2500

Merged

arik-so force-pushed the 2023-07-taproot-signer-wrapped branch from a6f5df6 to 9549888 Compare August 16, 2023 06:28

arik-so force-pushed the 2023-07-taproot-signer-wrapped branch 2 times, most recently from d011755 to c1faff0 Compare August 18, 2023 04:54

arik-so force-pushed the 2023-07-taproot-signer-wrapped branch from 8dc3262 to 5588f53 Compare August 22, 2023 01:40

arik-so force-pushed the 2023-07-taproot-signer-wrapped branch from f14f568 to 805e825 Compare August 22, 2023 03:53

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

wpaulino reviewed Aug 22, 2023

View reviewed changes

TheBlueMatt reviewed Aug 22, 2023

View reviewed changes

arik-so added 5 commits August 22, 2023 14:23

Fix persister/chain_monitor lifetimes.

43a9215

The persister and chain_monitor variables must be declared before the node channel manager is initialized to avoid out of order deallocation.

Add Taproot feature support.

5e6bc76

Introduce a Taproot feature on bits 30/31 for initialization, node, and channel type contexts.

Fix bench lifetimes.

d256a2b

Benchmarks were failing because node config and channel monitor configs were tied to the same lifetime. Introducing a separate lifetime allows to avoid out-of-order deallocation errors.

Remove unused imports.

6a2f43d

Remove a bunch of unnecessary ChannelManager imports.

arik-so force-pushed the 2023-07-taproot-signer-wrapped branch from 805e825 to 6a2f43d Compare August 22, 2023 21:28

wpaulino approved these changes Aug 22, 2023

View reviewed changes

devrandom reviewed Aug 22, 2023

View reviewed changes

TheBlueMatt approved these changes Aug 22, 2023

View reviewed changes

arik-so merged commit 8866ed3 into lightningdevkit:main Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrapped Channel Signer Type #2441

Wrapped Channel Signer Type #2441

arik-so commented Jul 21, 2023

TheBlueMatt left a comment

TheBlueMatt Aug 14, 2023

arik-so Aug 15, 2023

TheBlueMatt Aug 17, 2023

arik-so Aug 18, 2023

codecov-commenter commented Aug 16, 2023 •

edited

Loading

TheBlueMatt commented Aug 21, 2023

arik-so commented Aug 22, 2023

arik-so commented Aug 22, 2023

wpaulino Aug 22, 2023

arik-so Aug 22, 2023

TheBlueMatt Aug 22, 2023

arik-so Aug 22, 2023

wpaulino Aug 22, 2023

arik-so Aug 22, 2023

TheBlueMatt left a comment

TheBlueMatt Aug 22, 2023

devrandom Aug 22, 2023

arik-so Aug 22, 2023

TheBlueMatt Aug 22, 2023

arik-so Aug 22, 2023

		@@ -0,0 +1,32 @@
		use crate::sign::{ChannelSigner, EcdsaChannelSigner};

		pub(crate) enum ChannelSignerType<ECS: EcdsaChannelSigner> {

Wrapped Channel Signer Type #2441

Wrapped Channel Signer Type #2441

Conversation

arik-so commented Jul 21, 2023

TheBlueMatt 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

codecov-commenter commented Aug 16, 2023 • edited Loading

Codecov Report

TheBlueMatt commented Aug 21, 2023

arik-so commented Aug 22, 2023

arik-so commented Aug 22, 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

TheBlueMatt 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

Choose a reason for hiding this comment

codecov-commenter commented Aug 16, 2023 •

edited

Loading