Add `HolderCommitmentPoint` struct to track commitment points #3086

alecchendev · 2024-05-30T22:44:46Z

This is the first of several upcoming PRs to complete async signing. This adds the HolderCommitmentPoint struct to consolidate our logic getting commitment points, which will make things easier when this operation returns a result type in an upcoming PR. This refactor helps prepare for async signing, but still assumes the signer is synchronous.

I left a bunch of TODOs that should get removed in upcoming PRs, let me know if they're too much.

codecov-commenter · 2024-05-30T22:53:51Z

Codecov Report

Attention: Patch coverage is 92.40506% with 12 lines in your changes missing coverage. Please review.

Project coverage is 91.83%. Comparing base (df01208) to head (cf545b4).
Report is 99 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channel.rs	92.40%	6 Missing and 6 partials ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3086      +/-   ##
==========================================
+ Coverage   89.90%   91.83%   +1.92%     
==========================================
  Files         117      119       +2     
  Lines       97105   113897   +16792     
  Branches    97105   113897   +16792     
==========================================
+ Hits        87303   104596   +17293     
+ Misses       7243     6976     -267     
+ Partials     2559     2325     -234

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

lightning/src/ln/channel.rs

TheBlueMatt · 2024-05-31T14:46:47Z

lightning/src/ln/channel.rs

@@ -1119,6 +1119,81 @@ pub(crate) struct ShutdownResult {
 pub(crate) channel_funding_txo: Option<OutPoint>,
 }

+#[derive(Debug, Copy, Clone)]
+enum HolderCommitmentPoint {
+ Uninitialized { transaction_number: u64 },


Why even have this variant given we always immediately call request_next? ISTM we could drop the panics below if we just elided this and requested in new?

#3109 should hopefully provide more context for this now

Right...related question, how are y'all handling the ChannelSigner::pubkeys call? Are you just blocking for the initial signer setup there or are you doing something more clever to preload the pubkeys (that could also apply to the initial commitment point)?

Oh woops thought i responded to this - we basically preload pubkeys upon the creation of a node for all channels, so what we use for that probably can't directly help with the first commitment point. We could make it so that for async signing you need to pass the initial per commitment point into ChannelManager::create_channel or accept_inbound_channel to avoid doing the Uninitialized variant - and on our side we'd just have to wait for the response there which isn't a ton different from how we're waiting already

alecchendev · 2024-06-06T23:30:32Z

squashed because this is still in it's early phases. the major changes were:

merged HolderCommitmentPoint::request_next into advance
now return an option from current_point which gets rid of the panics, but in several cases I don't want to handle this case in this PR, so i've left some .expect("TODO")s for upcoming PRs. going to open the first PR resolving some of these within the hour, which should provide more context to this

Since this is a pretty big change, my goal is to make this easy to review as possible by splitting it up into separate PRs - some of this is a bit hard (-> .expect("TODO") is my best solution at the moment...), let me know if there are better/preferred ways of doing these things

TheBlueMatt · 2024-06-07T19:33:18Z

I almost wonder if we shouldn't introduce the machinery here with nothing fallible so that we don't any any extra expects and then add those in the next PR where we have to actually handle them anyway? We could even do dances like let next_point = Some(advance()); match next_point { None => future code, .. } to land things progressively.

alecchendev · 2024-06-10T01:00:48Z

Dropped the Uninitialized variant, made current_point infallible

TheBlueMatt

LGTM, please squash the fixups and lets land this!

alecchendev · 2024-06-10T15:21:00Z

squashed!

valentinewallace

Nothing blocking!

valentinewallace · 2024-06-10T16:14:10Z

lightning/src/ln/channel.rs

+ if self.context.signer_pending_funding {
+ // TODO: set signer_pending_channel_ready
+ log_debug!(logger, "Can't produce channel_ready: the signer is pending funding.");
+ return None;
+ }


Looks like this check exists already about 60 lines up?

oops just moved the log + TODO up there

sike, just removed the one above, i realized in the next PR we should only set signer_pending_channel_ready after we've passed all the other checks

sike x2, causes a bug, presumably because it causes us to do this check after modifying state

valentinewallace · 2024-06-10T16:47:31Z

lightning/src/ln/channel.rs

+ PendingNext { transaction_number: u64, current: PublicKey },
+ Available { transaction_number: u64, current: PublicKey, next: PublicKey },


IMO these could use some docs at some point, but feel free to hold off if it would make more sense to add them in following PRs.

oh true, i'll add some interim docs and probably expand them more in an upcoming PR

TheBlueMatt · 2024-06-10T19:44:05Z

Looks like tests are failing: thread 'ln::async_signer_tests::test_async_commitment_signature_for_funding_signed_0conf' panicked at lightning/src/ln/async_signer_tests.rs:238:9:

This includes when building TxCreationKeys, as well as for open_channel and accept_channel messages. Note: this is only for places where we are retrieving the current per commitment point, which excludes channel_reestablish.

alecchendev · 2024-06-10T20:07:37Z

looks to be this, should be fixed now

valentinewallace · 2024-06-10T19:27:34Z

lightning/src/ln/channel.rs

+ /// Our current commitment point is ready, we've cached our next point,
+ /// and we are not pending a new one.


nit: are "we've cached our next point" and "we are not pending a new one" the same thing? If so, I think one of the clauses could be removed since it sounds like separate things atm

valentinewallace · 2024-06-10T22:18:41Z

lightning/src/ln/channel.rs

@@ -5417,7 +5417,10 @@ impl<SP: Deref> Channel<SP> where
 }

 fn get_last_revoke_and_ack(&self) -> msgs::RevokeAndACK {
- let next_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(self.context.holder_commitment_point.transaction_number(), &self.context.secp_ctx);
+ debug_assert!(self.context.holder_commitment_point.transaction_number() <= INITIAL_COMMITMENT_NUMBER + 2);


Trying to understand the + 2. The bolts seem to read like our transaction number will never be > INITIAL_COMMITMENT_NUMBER, let me know what I'm missing here!

ah oops, this should actually be - 2, to assert we have always advanced our commitment point twice before we ever call here

Ah that makes sense lol

valentinewallace · 2024-06-10T22:21:15Z

lightning/src/ln/channel.rs

@@ -5417,7 +5417,10 @@ impl<SP: Deref> Channel<SP> where
 }

 fn get_last_revoke_and_ack(&self) -> msgs::RevokeAndACK {
- let next_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(self.context.holder_commitment_point.transaction_number(), &self.context.secp_ctx);
+ debug_assert!(self.context.holder_commitment_point.transaction_number() <= INITIAL_COMMITMENT_NUMBER + 2);
+ // TODO: handle non-available case when get_per_commitment_point becomes async


For my understanding, is this referring to when we implement this TODO to add the new HolderCommitmentPoint variant (which I think means that current_point() will become Optional)?

yep! technically it should never be unavailable by the time we get here, but figured i'd leave a note to think about it again when things change

It seems like PendingNext would also work here, so the assertion below still doesn't totally make sense to me, but it probably gets clearer in the follow-ups.

alecchendev force-pushed the 2024-05-holder-commitment branch from 95894e4 to 83c1fa9 Compare May 30, 2024 23:36

TheBlueMatt reviewed May 31, 2024

View reviewed changes

alecchendev force-pushed the 2024-05-holder-commitment branch from 83c1fa9 to 65b3898 Compare June 6, 2024 23:25

alecchendev mentioned this pull request Jun 6, 2024

Handle fallible per commitment point in channel establishment #3109

Open

3 tasks

alecchendev force-pushed the 2024-05-holder-commitment branch 3 times, most recently from e22f940 to 9bc514e Compare June 10, 2024 00:59

TheBlueMatt reviewed Jun 10, 2024

View reviewed changes

alecchendev force-pushed the 2024-05-holder-commitment branch from 9bc514e to e947e84 Compare June 10, 2024 15:20

valentinewallace self-requested a review June 10, 2024 16:52

valentinewallace previously approved these changes Jun 10, 2024

View reviewed changes

alecchendev dismissed valentinewallace’s stale review via adb038a June 10, 2024 17:29

alecchendev force-pushed the 2024-05-holder-commitment branch from e947e84 to adb038a Compare June 10, 2024 17:29

alecchendev added 2 commits June 10, 2024 10:39

Add HolderCommitmentPoint struct to ChannelContext

c08aa34

Use HolderCommitmentPoint for commitment number

7a115d7

alecchendev force-pushed the 2024-05-holder-commitment branch 2 times, most recently from 317426a to 74297f4 Compare June 10, 2024 18:42

alecchendev added 2 commits June 10, 2024 13:06

Get per commitment point for channel ready using HolderCommitmentPoint

d189cf0

alecchendev force-pushed the 2024-05-holder-commitment branch from 74297f4 to cf545b4 Compare June 10, 2024 20:07

TheBlueMatt approved these changes Jun 10, 2024

View reviewed changes

valentinewallace approved these changes Jun 10, 2024

View reviewed changes

valentinewallace merged commit f2237a7 into lightningdevkit:main Jun 10, 2024
14 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `HolderCommitmentPoint` struct to track commitment points #3086

Add `HolderCommitmentPoint` struct to track commitment points #3086

alecchendev commented May 30, 2024

codecov-commenter commented May 30, 2024 •

edited

Loading

TheBlueMatt May 31, 2024

alecchendev Jun 6, 2024

TheBlueMatt Jun 7, 2024

alecchendev Jun 10, 2024

alecchendev commented Jun 6, 2024 •

edited

Loading

TheBlueMatt commented Jun 7, 2024

alecchendev commented Jun 10, 2024

TheBlueMatt left a comment

alecchendev commented Jun 10, 2024

valentinewallace left a comment

valentinewallace Jun 10, 2024

alecchendev Jun 10, 2024

alecchendev Jun 10, 2024

alecchendev Jun 10, 2024

valentinewallace Jun 10, 2024

alecchendev Jun 10, 2024

alecchendev Jun 10, 2024

TheBlueMatt commented Jun 10, 2024

alecchendev commented Jun 10, 2024

valentinewallace Jun 10, 2024

valentinewallace Jun 10, 2024

alecchendev Jun 10, 2024

valentinewallace Jun 10, 2024

valentinewallace Jun 10, 2024

alecchendev Jun 10, 2024

valentinewallace Jun 10, 2024

		PendingNext { transaction_number: u64, current: PublicKey },
		Available { transaction_number: u64, current: PublicKey, next: PublicKey },

		/// Our current commitment point is ready, we've cached our next point,
		/// and we are not pending a new one.

Add HolderCommitmentPoint struct to track commitment points #3086

Add HolderCommitmentPoint struct to track commitment points #3086

Conversation

alecchendev commented May 30, 2024

codecov-commenter commented May 30, 2024 • edited Loading

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

alecchendev commented Jun 6, 2024 • edited Loading

TheBlueMatt commented Jun 7, 2024

alecchendev commented Jun 10, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

alecchendev commented Jun 10, 2024

valentinewallace 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jun 10, 2024

alecchendev commented Jun 10, 2024

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

Add `HolderCommitmentPoint` struct to track commitment points #3086

Add `HolderCommitmentPoint` struct to track commitment points #3086

codecov-commenter commented May 30, 2024 •

edited

Loading

alecchendev commented Jun 6, 2024 •

edited

Loading