Follow-ups for #4014 #4023
Follow-ups for #4014 #4023
Conversation
When the current holder commitment point is unavailable for a channel, we can't splice the channel. Make sure to disconnect so that the channel is no longer quiescent. Otherwise, it cannot be used for payments.
When introducing HolderCommitmentPoint::current_point, the value was mistakenly not set when read except in the legacy case where the next point needed to be fetched. But in that case, it would have been read as None given it is a new field.
When reading HolderCommitmentPoint, attempt to fetch the current point if it wasn't serialized. This allows channels to be spliced without first needing to have the HolderCommitmentPoint advanced. Don't fail if it can't be fetch synchronously as the channel can still be spliced once it is advanced.
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
@TheBlueMatt Should we rename |
jkczyz
force-pushed
the
splice-initial-commitment-signed-follow-ups
branch
from
6e1774a to
d94baae
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4023 +/- ##
==========================================
+ Coverage 88.61% 88.77% +0.15%
==========================================
Files 174 175 +1
Lines 127640 127895 +255
Branches 127640 127895 +255
==========================================
+ Hits 113113 113539 +426
+ Misses 12046 11796 -250
- Partials 2481 2560 +79
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lightning/src/ln/channel.rs
Outdated
| @@ -6987,7 +6987,7 @@ where | |||
| let commitment_point = self | |||
| .holder_commitment_point | |||
| .current_point() | |||
| .expect("current should be set after receiving the initial commitment_signed"); | |||
| .expect("current_point should be set for channels initiating splicing"); | |||
There was a problem hiding this comment.
Given we have an error option here should we error instead of panicing?
There was a problem hiding this comment.
Sure, I don't mind either way. But is the criteria for doing this simply if the option is available? There is an expect immediately above as well as one below plus an unwrap.
There was a problem hiding this comment.
I mean, yea, basically, though in this specific case the only reason this expect is not reachable is because we have a check in the splice-initialization logic and in the splice_init-receive logic. Relying on a single test in another method to avoid a panic kinda sucks, cause who knows what someone is gonna move around in the future. At least for some of the other expects/unwraps around here they're duplicated in many places so hopefully a more robust assumption.
There was a problem hiding this comment.
Wonder if we should include debug_assert in any of these cases. Otherwise, when reading the code it isn't always easy to determine what is a protocol error vs what is an expectation. Or maybe we make a dedicated ChannelError variant for this, which we can debug_assert elsewhere to keep the code succinct.
There was a problem hiding this comment.
We can definitely debug_assert. I mean I wasn't really trying to over-complicate this line.
|
👋 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. |
jkczyz
force-pushed
the
splice-initial-commitment-signed-follow-ups
branch
from
d94baae to
b0166dd
Compare
@TheBlueMatt Bumping this top-level comment in case you missed it. |
|
Oh sorry I did miss that. I don't have a strong opinion. |
jkczyz
force-pushed
the
splice-initial-commitment-signed-follow-ups
branch
from
b0166dd to
1f8a7d7
Compare
|
Pushed to fix CI and added the |
Addresses outstanding comments from #4014.