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

Split commitment_signed handling by check-accept #3633

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Feb 28, 2025

When handling commitment_signed messages, a number of checks are performed before a ChannelMonitorUpdate is created and returned. Once splicing is added, these checks need to be performed on the primary FundingScope and any pending scopes that resulted from splicing or RBF.

This PR splits the handling into a check and accept methods, taking &self and &mut self, respectively. This ensures that the ChannelContext is not modified between checks. Once all funding scopes have been checked successfully, the accept portion of the code can then execute.

@jkczyz jkczyz force-pushed the 2025-02-split-commitment-signed branch from d8bdc8f to 3a52856 Compare February 28, 2025 21:26
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was much simpler than we expected :)

@jkczyz jkczyz force-pushed the 2025-02-split-commitment-signed branch from 9c29928 to 5d9e8f1 Compare February 28, 2025 21:38
@jkczyz
Copy link
Contributor Author

jkczyz commented Feb 28, 2025

Latest pushes were just to address cargo doc failures in CI. Will address the comments shortly.

@jkczyz jkczyz force-pushed the 2025-02-split-commitment-signed branch from 5d9e8f1 to 08cef17 Compare February 28, 2025 22:07
@jkczyz jkczyz added the weekly goal Someone wants to land this this week label Feb 28, 2025
@jkczyz
Copy link
Contributor Author

jkczyz commented Feb 28, 2025

This was much simpler than we expected :)

Yeah, luckily compiling after removing the mut from the &self mut parameter showed the mutability wasn't needed until all the checks were done. So it turned out to be a relatively straightforward refactor.

@jkczyz jkczyz requested a review from wpaulino February 28, 2025 22:10
@jkczyz jkczyz force-pushed the 2025-02-split-commitment-signed branch from 08cef17 to baa9e76 Compare February 28, 2025 22:12
@wpaulino
Copy link
Contributor

wpaulino commented Mar 1, 2025

Feel free to squash

When handling commitment_signed messages, a number of checks are
performed before a ChannelMonitorUpdate is created and returned. Once
splicing is added, these checks need to be performed on the primary
FundingScope and any pending scopes that resulted from splicing or RBF.

This commit splits the handling into a check and accept methods, taking
&self and &mut self, respectively. This ensures that the ChannelContext
is not modified between checks. Once all funding scopes have been
checked successfully, the accept portion of the code can then execute.
Now that commitment_signed is split into check and accept methods, move
the check portion from FundedChannel to ChannelContext. This allows
calling it with a different FundingScope when there are pending splices
and RBF attempts.
@jkczyz jkczyz force-pushed the 2025-02-split-commitment-signed branch from baa9e76 to 119b64a Compare March 1, 2025 00:12
@jkczyz
Copy link
Contributor Author

jkczyz commented Mar 1, 2025

Squashed

Copy link

codecov bot commented Mar 1, 2025

Codecov Report

Attention: Patch coverage is 93.38843% with 8 lines in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (36ba27a) to head (119b64a).

Files with missing lines Patch % Lines
lightning/src/ln/channel.rs 93.38% 6 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3633      +/-   ##
==========================================
- Coverage   89.19%   89.19%   -0.01%     
==========================================
  Files         152      152              
  Lines      118681   118696      +15     
  Branches   118681   118696      +15     
==========================================
+ Hits       105860   105869       +9     
- Misses      10236    10242       +6     
  Partials     2585     2585              

☔ 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
weekly goal Someone wants to land this this week
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants