[Splicing] Add reserve check to splicing #3641
Conversation
|
👋 Thanks for assigning @wpaulino as a reviewer! |
|
Relevant comments from #3407:
Relevant method: |
optout21
force-pushed
the
splice-reserve-check
branch
2 times, most recently
from
e34d33c to
6ec1a19
Compare
optout21
force-pushed
the
splice-reserve-check
branch
from
6ec1a19 to
8ac1467
Compare
|
Rebased, post #3407 . |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3641 +/- ##
==========================================
- Coverage 89.23% 89.22% -0.01%
==========================================
Files 155 155
Lines 119327 119327
Branches 119327 119327
==========================================
- Hits 106482 106473 -9
- Misses 10242 10249 +7
- Partials 2603 2605 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
optout21
force-pushed
the
splice-reserve-check
branch
from
8ac1467 to
b073d8c
Compare
optout21
changed the title
|
👋 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. |
optout21
force-pushed
the
splice-reserve-check
branch
from
b073d8c to
f406c53
Compare
|
Fixes done:
|
| return Err(post_channel_reserve_sats); | ||
| } | ||
| } else { | ||
| if pre_balance >= self.funding.holder_selected_channel_reserve_satoshis * 1000 { |
There was a problem hiding this comment.
We also need to check if the channel has never been spliced before to use the v1 reserve
| /// Pending HTLCs are not taken into account, this method should be used when there is no such, | ||
| /// e.g. in quiscence state | ||
| #[cfg(splicing)] | ||
| fn compute_balances_less_fees(&self, channel_value_sats: u64, value_to_self_msat: u64, is_local: bool) -> (u64, u64) { |
There was a problem hiding this comment.
@tankyleo is going to split up the existing build_commitment_transaction method into two, so that we can get the balances without building the commitment transaction. Once that lands, we can use it here in favor of this to avoid the code duplication.
| /// Check that post-splicing balance meets reserve requirements, but only if it met it pre-splice as well. | ||
| /// Returns the minimum channel reserve (sats) | ||
| #[cfg(splicing)] | ||
| pub fn check_splice_balance_meets_v2_reserve_requirement_noerr(&self, pre_balance: u64, post_balance: u64, pre_channel_value: u64, post_channel_value: u64, dust_limit: u64) -> Result<(), u64> { |
There was a problem hiding this comment.
Could you move this below check_splice_balances_meet_v2_reserve_requirements so that the code could be read top-down?
| /// Check that post-splicing balance meets reserve requirements, but only if it met it pre-splice as well. | ||
| /// Returns the minimum channel reserve (sats) | ||
| #[cfg(splicing)] | ||
| pub fn check_splice_balance_meets_v2_reserve_requirement_noerr(&self, pre_balance: u64, post_balance: u64, pre_channel_value: u64, post_channel_value: u64, dust_limit: u64) -> Result<(), u64> { |
There was a problem hiding this comment.
Why "noerr" if it can error?
| } | ||
|
|
||
| /// Check that post-splicing balance meets reserve requirements, but only if it met it pre-splice as well. | ||
| /// Returns the minimum channel reserve (sats) |
| #[cfg(splicing)] | ||
| pub fn check_splice_balance_meets_v2_reserve_requirement_noerr(&self, pre_balance: u64, post_balance: u64, pre_channel_value: u64, post_channel_value: u64, dust_limit: u64) -> Result<(), u64> { | ||
| let post_channel_reserve_sats = get_v2_channel_reserve_satoshis(post_channel_value, dust_limit); | ||
| if post_balance >= post_channel_reserve_sats * 1000 { |
There was a problem hiding this comment.
Could we name the function parameters with _msat?
| let pre_channel_value = self.funding.get_value_satoshis(); | ||
| let post_channel_value = PendingSplice::compute_post_value(pre_channel_value, our_funding_contribution, their_funding_contribution_satoshis); | ||
| let pre_balance_self = self.funding.value_to_self_msat; | ||
| let post_balance_self = PendingSplice::add_checked(pre_balance_self, our_funding_contribution); | ||
| let (pre_balance_self_less_fees, pre_balance_counterparty_less_fees) = self.compute_balances_less_fees(pre_channel_value, pre_balance_self, true); | ||
| let (post_balance_self_less_fees, post_balance_counterparty_less_fees) = self.compute_balances_less_fees(post_channel_value, post_balance_self, true); |
This is a continuation of #3407, adds proper channel balance/reserve check, when handling
splice_ack(on initiator side).