Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set holder_commitment_point to Available on upgrade #3365

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TheBlueMatt
Copy link
Collaborator

When we upgrade from LDK 0.0.123 or prior, we need to intialize holder_commitment_point with commitment point(s). In 1f7f3a3 we changed the point(s) which we fetch from both the current and next per-commitment-point (setting the value to HolderCommitmentPoint::Available on upgrade) to only fetching the current per-commitment-point (setting the value to HolderCommitmentPoint::PendingNext on upgrade).

In commitment_signed handling, we expect the next per-commitment-point to be available (allowing us to advance() the holder_commitment_point), as it was included in the revoke_and_ack we most recently sent to our peer, so must've been available at that time.

Sadly, these two interact negatively with each other - on upgrade, assuming the channel is at a steady state and there are no pending updates, we'll not make the next per-commitment-point available but if we receive a commitment_signed from our peer we'll assume it is. As a result, in debug mode, we'll hit an assertion failure, and in production mode we'll force-close the channel.

Instead, here, we fix the upgrade logic to always upgrade directly to HolderCommitmentPoint::Available, making the next per-commitment-point available immediately.

We also attempt to resolve the next per-commitment-point in get_channel_reestablish, allowing any channels which were upgraded to LDK 0.0.124 and are in this broken state to avoid the force-closure, as long as they don't receive a commitment_signed in the interim.

This was included in the 0.0.125 release.

When we upgrade from LDK 0.0.123 or prior, we need to intialize
`holder_commitment_point` with commitment point(s). In
1f7f3a3 we changed the point(s)
which we fetch from both the current and next per-commitment-point
(setting the value to `HolderCommitmentPoint::Available` on
upgrade) to only fetching the current per-commitment-point (setting
the value to `HolderCommitmentPoint::PendingNext` on upgrade).

In `commitment_signed` handling, we expect the next
per-commitment-point to be available (allowing us to `advance()`
the `holder_commitment_point`), as it was included in the
`revoke_and_ack` we most recently sent to our peer, so must've been
available at that time.

Sadly, these two interact negatively with each other - on upgrade,
assuming the channel is at a steady state and there are no pending
updates, we'll not make the next per-commitment-point available but
if we receive a `commitment_signed` from our peer we'll assume it
is. As a result, in debug mode, we'll hit an assertion failure, and
in production mode we'll force-close the channel.

Instead, here, we fix the upgrade logic to always upgrade directly
to `HolderCommitmentPoint::Available`, making the next
per-commitment-point available immediately.

We also attempt to resolve the next per-commitment-point in
`get_channel_reestablish`, allowing any channels which were
upgraded to LDK 0.0.124 and are in this broken state to avoid the
force-closure, as long as they don't receive a `commitment_signed`
in the interim.
@TheBlueMatt TheBlueMatt added this to the 0.1 milestone Oct 14, 2024
Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.58%. Comparing base (46d8a0d) to head (1486b1b).

Files with missing lines Patch % Lines
lightning/src/ln/channel.rs 42.85% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3365      +/-   ##
==========================================
- Coverage   89.61%   89.58%   -0.03%     
==========================================
  Files         127      127              
  Lines      103533   103544      +11     
  Branches   103533   103544      +11     
==========================================
- Hits        92778    92758      -20     
- Misses       8056     8082      +26     
- Partials     2699     2704       +5     

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant