Support async fetching of commitment point during channel reestablish #4197

wpaulino · 2025-10-30T23:02:46Z

HolderCommitmentPoint currently tracks the current and next point used on counterparty commitments, which are unrevoked. When we reestablish a channel, the counterparty sends us the commitment height, along with the corresponding secret, for the state they believe to be the latest. We compare said secret to the derived point we fetch from the signer to know if the peer is being honest.

Since the protocol does not allow peers (assuming no data loss) to be behind the current state by more than one update, we can cache the two latest revoked commitment points alongside HolderCommitmentPoint, such that we no longer need to reach the signer asynchronously when handling channel_reestablish messages throughout the happy path. By doing so, we avoid complexity in needing to pause the state machine (which may also result in needing to stash any update messages from the counterparty) while the signer response is pending.

The only remaining case left to handle is when the counterparty presents a channel_reestablish with a state later than what we know. This can only result in two terminal cases: either they provided a valid commitment secret proving we are behind and we need to panic, or they lied and we force close the channel. This is the only case we choose to handle asynchronously as it's relatively trivial to handle.

ldk-reviews-bot · 2025-10-30T23:02:49Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-10-30T23:15:47Z

Codecov Report

❌ Patch coverage is 86.39053% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.34%. Comparing base (6749bc6) to head (1f7b249).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	80.61%	19 Missing ⚠️
lightning/src/ln/channelmanager.rs	75.00%	1 Missing and 2 partials ⚠️
lightning/src/ln/async_signer_tests.rs	98.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4197      +/-   ##
==========================================
- Coverage   89.34%   89.34%   -0.01%     
==========================================
  Files         180      180              
  Lines      138480   138620     +140     
  Branches   138480   138620     +140     
==========================================
+ Hits       123730   123846     +116     
- Misses      12129    12149      +20     
- Partials     2621     2625       +4

Flag	Coverage Δ
fuzzing	`35.96% <31.81%> (-0.01%)`	⬇️
tests	`88.70% <86.39%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ldk-reviews-bot · 2025-11-03T00:00:16Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt · 2025-11-03T14:30:31Z

lightning/src/ln/channel.rs

+					.ok();
+				if expected_point.is_none() {
+					self.context.signer_pending_stale_state_verification = Some((commitment_number, given_secret));
+					return Err(ChannelError::Ignore("Waiting on async signer to verify stale state proof".to_owned()));


In practice I think this means we'll often never panic - the peer will reconnect, we'll ignore the message, then they'll send some other message which will cause us to, for example, ChannelError::close("Got commitment signed message when channel was not in an operational state"). We'll either have to have logic in ~every message handler to ignore the message if signer_pending_stale_state_verification is set or we can just disconnect them here and let them be in a reconnect loop until the signer resolves (which I think is fine?).

Good point, ended up disconnecting. Is there any reason for us to close in those cases though? We could just make those ChannelError::close a WarnAndDisconnect instead.

No, those cases could definitely move to a warn-and-disconnect. Historically we've been pretty happy to just close if the peer does something dumb, and in 95% of the cases we've never seen peers do anything so dumb, so we've never really had a motivation to change it. Not crazy to do though.

ldk-reviews-bot · 2025-11-03T14:31:16Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

TheBlueMatt · 2025-11-03T14:32:01Z

fwiw clippy is unhappy.

TheBlueMatt · 2025-11-04T02:10:32Z

ln::async_signer_tests::test_async_force_close_on_invalid_secret_for_stale_state is failing in CI.

ldk-reviews-bot · 2025-11-06T17:51:03Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-11-10T00:00:37Z

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-11-12T00:01:12Z

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-11-13T19:15:48Z

✅ Added second reviewer: @valentinewallace

ldk-reviews-bot · 2025-11-17T00:00:10Z

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

lightning/src/ln/channel.rs

TheBlueMatt · 2025-11-17T19:28:01Z

CI is quite sad

wpaulino · 2025-11-18T18:27:18Z

Had to rebase to account for the changes to check_channel_closed

valentinewallace

Nothing blocking!

lightning/src/ln/channel.rs

valentinewallace · 2025-11-18T18:59:26Z

lightning/src/ln/channel.rs

+				if expected_point != Some(PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret)) {
+					return Err(ChannelError::close("Peer sent a channel_reestablish indicating we're stale with an invalid commitment secret".to_owned()));
+				}
+				Self::panic_on_stale_state(logger);


We don't have test coverage for hitting this, may be pre-existing though

valentinewallace · 2025-11-18T19:03:58Z

lightning/src/ln/channel.rs

+				if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) {
+					return Err(ChannelError::close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned()));
+				}
+			} else if msg.next_remote_commitment_number + 1 == our_commitment_transaction {


It would be nice to rename our_commitment_transaction to our_current_commit_tx_number or something like that, but it tis preexisting

valentinewallace · 2025-11-18T19:16:17Z

lightning/src/ln/channel.rs

+							holder_commitment_next_transaction_number + 3,
+							&secp_ctx,
+						)
+						.expect("Must be able to derive the previous revoked commitment point upon channel restoration"))


Just wondering -- is there plans to get rid of this and go fully async with the method? I guess in a release or three?

This is only for the upgrade case, we assume liveness prior to switching over to an async signer.

valentinewallace · 2025-11-18T19:27:37Z

lightning/src/ln/channel.rs

+					return Err(ChannelError::close("Peer sent a channel_reestablish indicating we're stale with an invalid commitment secret".to_owned()));
+				}
+				Self::panic_on_stale_state(logger);
+			} else if msg.next_remote_commitment_number == our_commitment_transaction {


Probably a dumb question -- in the spec the next_remote_commitment_number is described as the next commitment number they expect to receive, but above we seem to be setting current_transaction_number to the current commitment number, which is a bit confusing. Just want to double check there's no off-by-one there

It's actually the next_revocation_number in the spec. If you look at get_channel_reestablish, you'll find this comment where we set the field:

// We have to set next_remote_commitment_number to the next revoke_and_ack we expect to // receive, however we track it by the next commitment number for a remote transaction // (which is one further, as they always revoke previous commitment transaction, not // the one we send) so we have to decrement by 1. Note that if

Hm, that's confusing. Comment is a bit buried.

valentinewallace

One comment but I'm otherwise good to ack after CI is fixed

lightning/src/ln/channel.rs

`HolderCommitmentPoint` currently tracks the current and next point used on counterparty commitments, which are unrevoked. When we reestablish a channel, the counterparty sends us the commitment height, along with the corresponding secret, for the state they believe to be the latest. We compare said secret to the derived point we fetch from the signer to know if the peer is being honest. Since the protocol does not allow peers (assuming no data loss) to be behind the current state by more than one update, we can cache the two latest revoked commitment points alongside `HolderCommitmentPoint`, such that we no longer need to reach the signer asynchronously when handling `channel_reestablish` messages throughout the happy path. By doing so, we avoid complexity in needing to pause the state machine (which may also result in needing to stash any update messages from the counterparty) while the signer response is pending. The only remaining case left to handle is when the counterparty presents a `channel_reestablish` with a state later than what we know. This can only result in two terminal cases: either they provided a valid commitment secret proving we are behind and we need to panic, or they lied and we force close the channel. This is the only case we choose to handle asynchronously as it's relatively trivial to handle.

TheBlueMatt

Landing, given @valentinewallace indicated she was happy with it.

TheBlueMatt · 2025-11-19T13:15:40Z

lightning/src/ln/channel.rs

 	/// Similar to [`Self::signer_pending_commitment_update`] but we're waiting to send a
 	/// [`msgs::ChannelReady`].
 	signer_pending_channel_ready: bool,
+	// Upon receiving a [`msgs::ChannelReestablish`] message with a `next_remote_commitment_number`


nit: even for internal stuff its nice to make it a doc comment cause then cargo doc --include-private-items will generate docs for them and presumably some people's RLS will see it. Not sure if it actually impacts anyone on the team currently but I imagine in the future LLMs might care or maybe better IDEs might.

wpaulino added this to the 0.3 milestone Oct 30, 2025

wpaulino requested a review from TheBlueMatt October 30, 2025 23:02

wpaulino self-assigned this Oct 30, 2025

TheBlueMatt reviewed Nov 3, 2025

View reviewed changes

wpaulino force-pushed the async-get-per-commitment-point-channel-reestablish branch from 7c25f35 to 6b8123d Compare November 3, 2025 22:04

wpaulino force-pushed the async-get-per-commitment-point-channel-reestablish branch from 6b8123d to fa13381 Compare November 4, 2025 17:50

wpaulino requested a review from TheBlueMatt November 4, 2025 17:50

TheBlueMatt previously approved these changes Nov 13, 2025

View reviewed changes

ldk-reviews-bot requested a review from valentinewallace November 13, 2025 19:15

valentinewallace reviewed Nov 17, 2025

View reviewed changes

lightning/src/ln/channel.rs Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

wpaulino dismissed TheBlueMatt’s stale review via 201478e November 17, 2025 18:10

wpaulino force-pushed the async-get-per-commitment-point-channel-reestablish branch from fa13381 to 201478e Compare November 17, 2025 18:10

wpaulino requested a review from valentinewallace November 17, 2025 18:11

wpaulino force-pushed the async-get-per-commitment-point-channel-reestablish branch from 201478e to 82e2371 Compare November 18, 2025 18:26

valentinewallace previously approved these changes Nov 18, 2025

View reviewed changes

wpaulino dismissed valentinewallace’s stale review via 905f990 November 18, 2025 20:08

wpaulino force-pushed the async-get-per-commitment-point-channel-reestablish branch from 82e2371 to 905f990 Compare November 18, 2025 20:08

valentinewallace reviewed Nov 18, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the async-get-per-commitment-point-channel-reestablish branch from 905f990 to 1f7b249 Compare November 18, 2025 22:09

wpaulino requested review from TheBlueMatt and valentinewallace November 18, 2025 22:09

TheBlueMatt approved these changes Nov 19, 2025

View reviewed changes

TheBlueMatt merged commit 97204d6 into lightningdevkit:main Nov 19, 2025
26 checks passed

wpaulino deleted the async-get-per-commitment-point-channel-reestablish branch November 19, 2025 17:16

Support async fetching of commitment point during channel reestablish #4197

Support async fetching of commitment point during channel reestablish #4197

Conversation

wpaulino commented Oct 30, 2025

Uh oh!

ldk-reviews-bot commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ldk-reviews-bot commented Nov 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Nov 3, 2025

Uh oh!

TheBlueMatt commented Nov 3, 2025

Uh oh!

TheBlueMatt commented Nov 4, 2025

Uh oh!

ldk-reviews-bot commented Nov 6, 2025

Uh oh!

ldk-reviews-bot commented Nov 10, 2025

Uh oh!

ldk-reviews-bot commented Nov 12, 2025

Uh oh!

ldk-reviews-bot commented Nov 13, 2025

Uh oh!

ldk-reviews-bot commented Nov 17, 2025

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Nov 17, 2025

Uh oh!

wpaulino commented Nov 18, 2025

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ldk-reviews-bot commented Oct 30, 2025 •

edited

Loading

codecov bot commented Oct 30, 2025 •

edited

Loading