Split commitment_signed handling by check-accept
#3633
Conversation
jkczyz
force-pushed
the
2025-02-split-commitment-signed
branch
from
d8bdc8f to
3a52856
Compare
jkczyz
force-pushed
the
2025-02-split-commitment-signed
branch
from
9c29928 to
5d9e8f1
Compare
|
Latest pushes were just to address |
jkczyz
force-pushed
the
2025-02-split-commitment-signed
branch
from
5d9e8f1 to
08cef17
Compare
Yeah, luckily compiling after removing the |
jkczyz
force-pushed
the
2025-02-split-commitment-signed
branch
from
08cef17 to
baa9e76
Compare
|
Feel free to squash |
jkczyz
force-pushed
the
2025-02-split-commitment-signed
branch
from
baa9e76 to
119b64a
Compare
|
Squashed |
Codecov ReportAttention: Patch coverage is
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. |
| @@ -529,6 +529,14 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent, | |||
|
|
|||
| ); | |||
|
|
|||
| /// Partial data from ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo used to simplify the | |||
| /// return type of `FundedChannel::validate_commitment_signed`. | |||
| pub(crate) struct LatestHolderCommitmentTXInfo { | |||
There was a problem hiding this comment.
Why is this here when its only used in channel.rs?
There was a problem hiding this comment.
Ah, it's copied partly from ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo , so made sense to keep it close. Happy to move it if you prefer.
There was a problem hiding this comment.
I mean we could (should?) use it in ChannelMonitorUpdateStep::LatestHolderCommitmentTxInfo, but if we're not gonna use it it feels really weird to not just define it in the file we're using it in.
There was a problem hiding this comment.
Right, I didn't use it here because it seems it would require introducing custom serialization code, which we'd want to avoid. Having it here was more so because if the enum is updated, this probably should be, too. I suppose a comment would suffice, though.
There was a problem hiding this comment.
Moved to channel.rs and added a comment in channelmonitor.rs.
There was a problem hiding this comment.
I guess I'm still confused why rustc wont enforce that for us by just making things not compile, but okay.
There was a problem hiding this comment.
Someone can mistakenly compute the new field once for all funding scopes when it should be computed individually for each. This is all hypothetical since the right way may be entirely obvious depending on the particulars.
jkczyz
force-pushed
the
2025-02-split-commitment-signed
branch
2 times, most recently
from
4c758a8 to
7d1097a
Compare
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.
Now that validate_commitment_signed encapsulates all funding-specific checks, move the holder_commitment_point advancement immediately following the call to it. While there should be any early returns at that point, it's good to have move it earlier in case of future changes.
jkczyz
force-pushed
the
2025-02-split-commitment-signed
branch
from
7d1097a to
2be03ce
Compare
|
Sorry, ended up rebasing on |
When handling
commitment_signed messages, a number of checks are performed before aChannelMonitorUpdateis created and returned. Once splicing is added, these checks need to be performed on the primaryFundingScopeand any pending scopes that resulted from splicing or RBF.This PR splits the handling into a check and accept methods, taking
&selfand&mut self, respectively. This ensures that theChannelContextis not modified between checks. Once all funding scopes have been checked successfully, the accept portion of the code can then execute.