Validate funding contributions reserves in splice_init and splice_ack handling
#4011
Conversation
|
👋 Thanks for assigning @wpaulino as a reviewer! |
tankyleo
changed the title
splice_ack and splice_init messagessplice_init and splice_ack messages
tankyleo
force-pushed
the
splice-reserve-check
branch
from
6d6b07c to
93965c6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4011 +/- ##
==========================================
+ Coverage 88.76% 88.77% +0.01%
==========================================
Files 176 176
Lines 129345 129357 +12
Branches 129345 129357 +12
==========================================
+ Hits 114812 114836 +24
+ Misses 11925 11921 -4
+ Partials 2608 2600 -8
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:
|
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
On hold until splice-out PR gets in. |
|
🔔 3rd Reminder Hey @wpaulino! This PR has been waiting for your review. |
tankyleo
force-pushed
the
splice-reserve-check
branch
from
93965c6 to
d0023e3
Compare
tankyleo
requested review from
wpaulino
and removed request for
valentinewallace
|
👋 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. |
tankyleo
force-pushed
the
splice-reserve-check
branch
from
d0023e3 to
835b67b
Compare
|
Rebase on merge-base (diff):
|
tankyleo
changed the title
splice_init and splice_ack messagessplice_init and splice_ack handling
tankyleo
force-pushed
the
splice-reserve-check
branch
from
835b67b to
453a211
Compare
|
Amend: (diff)
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
Rebasing now to fix conflict... |
tankyleo
requested review from
TheBlueMatt
and removed request for
TheBlueMatt
tankyleo
force-pushed
the
splice-reserve-check
branch
2 times, most recently
from
8b3004e to
ce4161c
Compare
lightning/src/ln/channel.rs
Outdated
| let their_channel_balance = Amount::from_sat(self.funding.get_value_satoshis()) | ||
| - Amount::from_sat(self.funding.get_value_to_self_msat() / 1000); | ||
| let post_channel_balance = AddSigned::checked_add_signed( | ||
| let adjusted_funding_contribution = if let Some((contribution, is_initiator, feerate)) = |
There was a problem hiding this comment.
Are we sure this is right in the splice_ack case? In the splice_channel (initiate outbound splice) case, we pass the total contribution ignoring fees, which then mandates that we subtract the fee here. But then we go to test again in splice_ack and, AFAICT we pass the adjusted contribution at that point (which is what we send to our counterparty, or the their_funding_contribution used below), and then we shouldn't subtract the fee here. IMO we should pull this back out to the splice_channel method and keep this method strictly a symmetrical splice validation function that doesn't have any sender/recipient-specific logic at all. Once we make get_next_{local,remote}_commitment_stats fallible it'll just be a matter of calling that and checking the reserve, basically.
There was a problem hiding this comment.
Will look into it thank you
But then we go to test again in splice_ack and, AFAICT we pass the adjusted contribution at that point (which is what we send to our counterparty, or the their_funding_contribution used below), and then we shouldn't subtract the fee here.
When we receive splice_ack, we set this triple to None, and we don't subtract the fee here ?
I'll revisit this part in any case.
There was a problem hiding this comment.
Ah, sorry, duh, all the more confusing code tho :)
tankyleo
force-pushed
the
splice-reserve-check
branch
from
1ce710f to
b6757a5
Compare
There was a problem hiding this comment.
Okay, thanks for rewriting this four times 😅. Feel free to squash (looks like the latest fixup needs to get moved back a bit), I think this LGTM. cc @wpaulino
|
Also sadly needs rebase. |
tankyleo
force-pushed
the
splice-reserve-check
branch
from
b6757a5 to
c96dba4
Compare
|
First rebase: no diff, squash fixups, reorganize commits. |
As much as possible, we want to only mutate state once we are done with input validation. This also removes complaints when helper functions during validation take a `&self`.
As in `splice_init`, this helps clearly delineate `splice_ack` message validation from the subsequent state mutations. This is a code-move.
`NextCommitmentStats` provides the commitment transaction fee as a separate value to assist with applying a multiplier on it in `can_accept_incoming_htlc`. Nonetheless in most cases, we want the balances to include the commitment transaction fee, so here we add a helper that gives us these balances. Also make the style of `tx_builder::subtract_addl_outputs` consistent with `get_balances_including_fee`.
The reserve we should maintain on our own transaction should be greater than our own dust limit.
We check this when validating `splice_init`, `splice_ack` messages, and
also when validating user-specified contributions.
From BOLT 2:
```
- If `funding_contribution_satoshis` is negative and its absolute value
is greater than the sending node's current channel balance:
- MUST send a `warning` and close the connection or send an `error`
and fail the channel.
```
and further down:
```
If a side does not meet the reserve requirements, that's OK: but if they
take funds out of the channel, they must ensure that they do meet them.
If your peer adds a massive amount to the channel, then you only have
to add more reserve if you want to contribute to the splice (and you
can use `tx_remove_output` and/or `tx_remove_input` part-way through if
this happens).
```
Therefore, we check the v2 reserve anytime
`funding_contribution_satoshis` is not equal to zero.
We allow parties to draw from their previous reserve, as long as they
satisfy their v2 reserve.
tankyleo
force-pushed
the
splice-reserve-check
branch
from
c96dba4 to
85a02ca
Compare
| @@ -2298,12 +2298,12 @@ impl FundingScope { | |||
| .funding_pubkey = counterparty_funding_pubkey; | |||
|
|
|||
| // New reserve values are based on the new channel value and are v2-specific | |||
| let counterparty_selected_channel_reserve_satoshis = Some(get_v2_channel_reserve_satoshis( | |||
| let counterparty_selected_channel_reserve_satoshis = | |||
| Some(get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS)); | |||
There was a problem hiding this comment.
Wait, I thin this is wrong (and also maybe reserves should be higher than both dust limits?). Not sure what the spec says, but the point of the reserve is to have something that we can be punished for when broadcasting our own stale transaction. Thus, the reserve we keep should really be higher than our own dust limit (which applies to our local commitment transactions that we can broadcast) so that if we broadcast a stale state we can be punished.
There was a problem hiding this comment.
Isn't that what we have here ?
MIN_CHAN_DUST_LIMIT_SATOSHIS: our own dust limit
counterparty_selected_channel_reserve_satoshis: we must keep at least these many sats "punishable" on our broadcastable transaction.
Uh oh!
There was an error while loading. Please reload this page.