Validate next_splice_out_maximum_sat and reserved fees on both commitments#4625
Validate next_splice_out_maximum_sat and reserved fees on both commitments#4625tankyleo wants to merge 3 commits into
next_splice_out_maximum_sat and reserved fees on both commitments#4625Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
| // reserve violation even though it's a dust HTLC and therefore shouldn't count towards the | ||
| // commitment transaction fee. | ||
| route_payment(&nodes[1], &[&nodes[0]], dust_amt); | ||
| // This will be dust on the remote commitment, but non-dust on the local commitment |
There was a problem hiding this comment.
The comment is incorrect. dust_amt_on_both_commitments = dust_limit + htlc_timeout_fee - 1, which is below the dust threshold on both commitments (timeout applies on node 1's local commitment, success applies on node 0's local commitment, and success_fee > timeout_fee). The variable name dust_amt_on_both_commitments is correct — this amount is dust on both commitments, not "non-dust on the local commitment" as the comment states.
| // This will be dust on the remote commitment, but non-dust on the local commitment | |
| // This will be dust on both commitments |
|
|
||
| let _node_id_0 = nodes[0].node.get_our_node_id(); |
There was a problem hiding this comment.
Nit: _node_id_0 and _node_id_1 are computed but never used in this test. Consider removing these lines.
| let _node_id_0 = nodes[0].node.get_our_node_id(); |
| let nodes = create_network(2, &node_cfgs, &node_chanmgrs); | ||
|
|
There was a problem hiding this comment.
Same here — _node_id_0 and _node_id_1 are unused dead code.
| let nodes = create_network(2, &node_cfgs, &node_chanmgrs); |
| let available_capacity_on_remote_commitment = read_available_capacity( | ||
| remote_nondust_htlc_count, | ||
| channel_constraints.counterparty_dust_limit_satoshis + real_htlc_success_tx_fee_sat, | ||
| ); | ||
| cmp::min(available_capacity_on_local_commitment, available_capacity_on_remote_commitment) |
There was a problem hiding this comment.
Behavioral change: in the original code, the remote commitment was only checked in the !is_outbound_from_holder path using feerate_per_kw (not spiked). Now, when the holder is the funder, the remote commitment fee is also checked at spiked_feerate.
For anchor channels (where spiked_feerate == feerate_per_kw), this is a no-op. For legacy non-anchor channels (where spiked_feerate == feerate_per_kw * 2), this is more conservative — the holder must be able to afford the spiked fee on the remote commitment too. That seems correct (the holder pays fees on both commitments), but the new tests only cover anchor channels, so this behavioral change for legacy channels is untested.
|
After a thorough line-by-line review of the entire diff, including tracing through the logic of every new and refactored function, I found no new issues beyond what was flagged in the prior review pass. Review SummaryNo new issues found beyond those already posted in the prior review. Prior comments still applicable:
Verification of key changes:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4625 +/- ##
==========================================
+ Coverage 86.58% 86.67% +0.08%
==========================================
Files 159 159
Lines 110419 110654 +235
Branches 110419 110654 +235
==========================================
+ Hits 95610 95904 +294
+ Misses 12276 12222 -54
+ Partials 2533 2528 -5
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:
|
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
All these commits make sense to me (boy --color-moved --color-moved-ws=ignore-space-change is great), but they could all do with some actual comentary in the commit message.
Wilmer's fuzzing runs caught a case where an advertised splice out maximum hit the debug assertions in `get_next_splice_out_maximum`. These debug assertions ensure that any adverstised splice out maximum passes the validation of splice contributions. The core issue is that we only read the local commitment when calculating the splice out maximum, but our splice validation requires that any splice out maximum is covered by the minimum of the holder's balances on the local and the remote commitments. Therefore, if a HTLC is dust on the local commitment, but non-dust on the remote commitment, and the holder is the funder of the channel, we advertise a splice out maximum that is not covered by the holder's balance on the remote commitment, and fails our validation of splice contributions. We now read both commitments when calculating the next splice out maximum, which fixes this issue.
Most diffs here are code moves.
The local and remote commitments may have different dust limits, which can cause each commitment to have a different transaction fee. Therefore when we reserve commitment transaction fees in `get_available_balances`, we must ensure that we read the maximum of the transaction fees on the local and the remote commitments. Otherwise, we may have not reserved enough fees to ensure that our next proposed channel state update is onside.
tankyleo
force-pushed
the
2026-05-check-both-commitments
branch
from
7d0c2e1 to
9fe1a36
Compare
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Didn't really review the tests, but the code changes look good
4 participants
No description provided.