Open and accept zero reserve channels#4428
Open and accept zero reserve channels#4428tankyleo wants to merge 9 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @carlaKC as a reviewer! |
ffa1657 to
5fa3a7c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4428 +/- ##
==========================================
- Coverage 85.94% 85.91% -0.03%
==========================================
Files 159 159
Lines 104644 105103 +459
Branches 104644 105103 +459
==========================================
+ Hits 89934 90298 +364
- Misses 12204 12300 +96
+ Partials 2506 2505 -1
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
There was a problem hiding this comment.
chanmon_consistency needs to be updated to have a 0-reserve channel or two (I believe we now have three channels between each pair of peers, so we can just do it on a subset of them, in fact we could have three separate channel types for better coverage).
| /// Creates a new outbound channel to the given remote node and with the given value. | ||
| /// | ||
| /// The only difference between this method and [`ChannelManager::create_channel`] is that this method sets | ||
| /// the reserve the counterparty must keep at all times in the channel to zero. This allows the counterparty to |
There was a problem hiding this comment.
nit: If that's the only difference let's say create_channel_to_trusted_peer_0_reserve? Nice to be explicit, imo.
|
|
||
| let channel_value_satoshis = | ||
| our_funding_contribution_sats.saturating_add(msg.common_fields.funding_satoshis); | ||
| // TODO(zero_reserve): support reading and writing the `disable_channel_reserve` field |
There was a problem hiding this comment.
Two questions. Shouldn't we check that if a channel has the 0-reserve feature bit and if it is fail if the user isn't accepting 0-reserve? Also why shouldn't we just set it now? I'm not sure we need to bother with a staging bit, really, honestly...
|
Needs rebase now :/ |
471ba8f to
253db4d
Compare
Let me know if you prefer I rebase first |
|
Feel free to go ahead and rebase and squash, yea. |
5fa3a7c to
43be438
Compare
|
Squash diff (do not click compare just above, I pushed the wrong branch, and later corrected it): |
The goal is to prevent any commitments with no outputs, since these are not broadcastable.
This new flag sets 0-reserve for the channel opener.
This new method sets 0-reserve for the channel accepter.
`ChannelContext::do_accept_channel_checks`, `ChannelContext::new_for_outbound_channel`, `ChannelContext::new_for_inbound_channel`, `InboundV1Channel::new`, `OutboundV1Channel::new`.
Co-Authored-By: HAL 9000
43be438 to
7fde002
Compare
|
|
|
✅ Added second reviewer: @joostjager |
|
🔔 1st Reminder Hey @TheBlueMatt @joostjager! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @joostjager! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt @joostjager! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @TheBlueMatt @carlaKC! This PR has been waiting for your review. |
carlaKC
left a comment
There was a problem hiding this comment.
First pass - haven't gone through the tests yet.
lightning/src/sign/tx_builder.rs
Outdated
| let dust_limit_msat = dust_limit_satoshis.saturating_mul(1000); | ||
| if holder_balance_msat.saturating_sub(max_dust_htlc_msat) < dust_limit_msat | ||
| && counterparty_balance_msat < dust_limit_msat | ||
| && nondust_htlc_count == 0 |
There was a problem hiding this comment.
Can we re-use check_no_outputs here? Could also return early if we don't need any action.
We'd need to change it to a saturating_sub, but I think that's okay because we have a checked_sub check of our balances on 321 after calling check_no_outputs anyway?
There was a problem hiding this comment.
done below thank you !
| // If we are allowed to send non-dust HTLCs, set the min HTLC to the smallest non-dust HTLC... | ||
| if available_capacity_msat >= min_nondust_htlc_sat.saturating_mul(1000) { |
There was a problem hiding this comment.
nit: to the largest non-dust htlc?
There was a problem hiding this comment.
hmm min_nondust_htlc_sat is right on the dust limit, so that's the smallest non-dust HTLC we can send ? The largest non-dust HTLC would be the available_capacity_msat, which we leave untouched.
lightning/src/sign/tx_builder.rs
Outdated
| // 1) The dust_limit_satoshis plus the fee of the exisiting commitment at the spiked feerate. | ||
| // 2) The fee of the commitment with an additional non-dust HTLC, aka the fee spike buffer HTLC. | ||
| // In this case we don't mind the holder balance output dropping below the dust limit, as | ||
| // this additional non-dust HTLC will create the single remaining output on the commitment. | ||
| let min_balance_msat = | ||
| cmp::max(dust_limit_satoshis + tx_fee_sat, fee_spike_buffer_sat) * 1000; |
There was a problem hiding this comment.
Comment for (1) says "at the spiked feerate" but isn't tx_fee_sat is just our current feerate?
A more descriptive name would be helpful here (if you can thing of something less gross than max_output_preserving_fee?).
There was a problem hiding this comment.
Comment for (1) says "at the spiked feerate" but isn't
tx_fee_satis just our current feerate?
See the callsites of the function; we always use the spiked_feerate as the feerate_per_kw parameter. I admit this is confusing, should I rename the function parameter to spiked_feerate ?
A more descriptive name would be helpful here (if you can thing of something less gross than max_output_preserving_fee?).
How is current_spiked_tx_fee_sat ? It would be consistent with the spiked_feerate style. With max_output_preserving_fee you want to highlight that we want to maintain an output on the commitment transaction even with a 2x increase in the feerate ?
There was a problem hiding this comment.
went ahead with both variable renames below
| /// If it does not confirm before we decide to close the channel, or if the funding transaction | ||
| /// does not pay to the correct script the correct amount, *you will lose funds*. | ||
| /// | ||
| /// # Zero-reserve |
There was a problem hiding this comment.
Shouldn't we be setting the option_zero_reserve feature in lightning/bolts#1140?
There was a problem hiding this comment.
Discussed offline, I'll hold off on signaling for now pending further spec discussions
Also take the opportunity to remove some test case runs that don't do anything novel compared to other runs.
Fixes #1801