Open and accept zero reserve channels

[tankyleo]

Fixes#1801

[ldk-reviews-bot]

👋 Thanks for assigning @carlaKC as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

❌ Patch coverage is 80.11696% with 170 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.17%. Comparing base (d830f10) to head (32a67f8).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	71.12%	148 Missing and 3 partials ⚠️
lightning/src/ln/channelmanager.rs	84.78%	14 Missing ⚠️
lightning/src/ln/msgs.rs	94.87%	4 Missing ⚠️
lightning/src/sign/tx_builder.rs	99.37%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4428 +/- ##
==========================================
- Coverage 86.18% 86.17% -0.02% 
==========================================
Files 160 160 Lines 107536 108046 +510 Branches 107536 108046 +510 ==========================================
+ Hits 92680 93105 +425 - Misses 12231 12321 +90 + Partials 2625 2620 -5 

Flag	Coverage Δ
tests	86.17% <80.11%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

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).

[TheBlueMatt]

nit: If that's the only difference let's say create_channel_to_trusted_peer_0_reserve? Nice to be explicit, imo.

[TheBlueMatt]

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...

[TheBlueMatt]

Needs rebase now :/

[tankyleo]

Needs rebase now :/

Let me know if you prefer I rebase first

[TheBlueMatt]

Feel free to go ahead and rebase and squash, yea.

[tankyleo]

Squash diff (do not click compare just above, I pushed the wrong branch, and later corrected it):

https://github.com/lightningdevkit/rust-lightning/compare/253db4d9a05cda43f74c2835448126d3205b27b8..43be438f70ba28ad7918e407026a78763ac2b368

[tankyleo]

git range-diff main 43be438 7fde002

1: bfba6e993 ! 1: b916c3596 Add inbound and outbound checks for zero reserve channels
@@ lightning/src/ln/channel.rs: impl FundingScope {
// 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(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS));
+- get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS);
- let holder_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis(
- post_channel_value,
- context.counterparty_dust_limit_satoshis,
@@ lightning/src/ln/channel.rs: impl FundingScope {
+ == 0
+ {
+ // If we previously had a 0-value reserve, continue with the same reserve
-+ Some(0)
++ 0
+ } else {
-+ Some(get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS))
++ get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS)
+ };
+ let holder_selected_channel_reserve_satoshis =
+ if prev_funding.holder_selected_channel_reserve_satoshis == 0 {
@@ lightning/src/sign/tx_builder.rs: fn get_next_commitment_stats(
);
@@ lightning/src/sign/tx_builder.rs: fn get_available_balances(
- let fee_spike_buffer_htlc =
if channel_type.supports_anchor_zero_fee_commitments() { 0 } else { 1 };
+ // Note that the feerate is 0 in zero-fee commitment channels, so this statement is a noop
- let local_feerate = feerate_per_kw
+ let spiked_feerate = feerate_per_kw
* if is_outbound_from_holder && !channel_type.supports_anchors_zero_fee_htlc_tx() {
2: 1f8ca6c66 = 2: 53fc9a354 Add 0-reserve to `accept_inbound_channel_from_trusted_peer`
3: 1b94f7cc3 = 3: da0520818 Add `ChannelManager::create_channel_to_trusted_peer_0reserve`
4: 19b8d7943 = 4: 1905f94e7 Shakedown zero reserve channels
5: de2366da4 = 5: 4e6a7527a Format `ChannelManager::create_channel_internal` and...
6: 43be438f7 = 6: 7fde002e7 Update `chanmon_consistency` to include 0FC and 0-reserve channels

[ldk-reviews-bot]

✅ Added second reviewer: @joostjager

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 2nd Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 3rd Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[carlaKC]

First pass - haven't gone through the tests yet.

[carlaKC]

Shouldn't we be setting the option_zero_reserve feature in lightning/bolts#1140?

[tankyleo]

Discussed offline, I'll hold off on signaling for now pending further spec discussions

[tankyleo]

git diff bbe7a62 2967e6f, all significant changes in commit 5eb530d

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 8f8ab4a9b..fa394ee6e 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -15586,6 +15586,10 @@
}
let is_manual_broadcast = Some(self.context.is_manual_broadcast);
+ let has_0reserve =+ (self.funding.counterparty_selected_channel_reserve_satoshis.is_some_and(|r| r == 0)+ || self.funding.holder_selected_channel_reserve_satoshis == 0)+ .then_some(());
let holder_commitment_point_previous_revoked =
self.holder_commitment_point.previous_revoked_point();
let holder_commitment_point_last_revoked =
@@ -15655,6 +15659,7 @@
// 65 was previously used for quiescent_action
(67, pending_outbound_held_htlc_flags, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags, optional_vec), // Added in 0.2
+ (70, has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked, option), // Added in 0.3
(73, holder_commitment_point_last_revoked, option), // Added in 0.3
(75, inbound_committed_update_adds, optional_vec),
@@ -16028,6 +16033,7 @@
let mut malformed_htlcs: Option<Vec<(u64, u16, [u8; 32])>> = None;
let mut monitor_pending_update_adds: Option<Vec<msgs::UpdateAddHTLC>> = None;
+ let mut _has_0reserve: Option<()> = None;
let mut holder_commitment_point_previous_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_last_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_current_opt: Option<PublicKey> = None;
@@ -16098,6 +16104,7 @@
// 65 quiescent_action: Added in 0.2; removed in 0.3
(67, pending_outbound_held_htlc_flags_opt, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags_opt, optional_vec), // Added in 0.2
+ (70, _has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked_opt, option), // Added in 0.3
(73, holder_commitment_point_last_revoked_opt, option), // Added in 0.3
(75, inbound_committed_update_adds_opt, optional_vec),
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 432070129..184bc4052 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -3555,7 +3555,7 @@
///
/// Note that there is no guarantee that the counterparty accepts such a channel themselves.
ZeroReserve,
-	/// Sets combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]+	/// Sets the combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]
ZeroConfZeroReserve,
}

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Happy to land this to get the API changes in now, but a few small tweaks we'll want to change/address before release.

[TheBlueMatt]

I've never heard of a subtrahend 😂

[tankyleo]

haha yes I looked it up do you have a suggestion ? value_to_subtract comes to mind :)

[TheBlueMatt]

I assume this needs a bLIP update? Given this is for a bLIP and not a BOLT (IIRC?) should we not be using a +1000 or whatever range?

[tankyleo]

This is a BOLT proposal. Using the staging bit makes sense to me yes.

[TheBlueMatt]

We've always supported counterparty-selected reserve of 0, so I don't think we want to break downgrade for that?

[carlaKC]

lgtm!

[tankyleo]

Thank you for the review ! Happy to hold off on the merge and get this PR just right :)

[carlaKC]

RIP subtrahend, word of the week ✊

[tankyleo]

Fixes#1801

[ldk-reviews-bot]

👋 Thanks for assigning @carlaKC as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

❌ Patch coverage is 80.11696% with 170 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.17%. Comparing base (d830f10) to head (32a67f8).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	71.12%	148 Missing and 3 partials ⚠️
lightning/src/ln/channelmanager.rs	84.78%	14 Missing ⚠️
lightning/src/ln/msgs.rs	94.87%	4 Missing ⚠️
lightning/src/sign/tx_builder.rs	99.37%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4428 +/- ##
==========================================
- Coverage 86.18% 86.17% -0.02% 
==========================================
Files 160 160 Lines 107536 108046 +510 Branches 107536 108046 +510 ==========================================
+ Hits 92680 93105 +425 - Misses 12231 12321 +90 + Partials 2625 2620 -5 

Flag	Coverage Δ
tests	86.17% <80.11%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

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).

[TheBlueMatt]

nit: If that's the only difference let's say create_channel_to_trusted_peer_0_reserve? Nice to be explicit, imo.

[TheBlueMatt]

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...

[TheBlueMatt]

Needs rebase now :/

[tankyleo]

Needs rebase now :/

Let me know if you prefer I rebase first

[TheBlueMatt]

Feel free to go ahead and rebase and squash, yea.

[tankyleo]

Squash diff (do not click compare just above, I pushed the wrong branch, and later corrected it):

https://github.com/lightningdevkit/rust-lightning/compare/253db4d9a05cda43f74c2835448126d3205b27b8..43be438f70ba28ad7918e407026a78763ac2b368

[tankyleo]

git range-diff main 43be438 7fde002

1: bfba6e993 ! 1: b916c3596 Add inbound and outbound checks for zero reserve channels
@@ lightning/src/ln/channel.rs: impl FundingScope {
// 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(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS));
+- get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS);
- let holder_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis(
- post_channel_value,
- context.counterparty_dust_limit_satoshis,
@@ lightning/src/ln/channel.rs: impl FundingScope {
+ == 0
+ {
+ // If we previously had a 0-value reserve, continue with the same reserve
-+ Some(0)
++ 0
+ } else {
-+ Some(get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS))
++ get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS)
+ };
+ let holder_selected_channel_reserve_satoshis =
+ if prev_funding.holder_selected_channel_reserve_satoshis == 0 {
@@ lightning/src/sign/tx_builder.rs: fn get_next_commitment_stats(
);
@@ lightning/src/sign/tx_builder.rs: fn get_available_balances(
- let fee_spike_buffer_htlc =
if channel_type.supports_anchor_zero_fee_commitments() { 0 } else { 1 };
+ // Note that the feerate is 0 in zero-fee commitment channels, so this statement is a noop
- let local_feerate = feerate_per_kw
+ let spiked_feerate = feerate_per_kw
* if is_outbound_from_holder && !channel_type.supports_anchors_zero_fee_htlc_tx() {
2: 1f8ca6c66 = 2: 53fc9a354 Add 0-reserve to `accept_inbound_channel_from_trusted_peer`
3: 1b94f7cc3 = 3: da0520818 Add `ChannelManager::create_channel_to_trusted_peer_0reserve`
4: 19b8d7943 = 4: 1905f94e7 Shakedown zero reserve channels
5: de2366da4 = 5: 4e6a7527a Format `ChannelManager::create_channel_internal` and...
6: 43be438f7 = 6: 7fde002e7 Update `chanmon_consistency` to include 0FC and 0-reserve channels

[ldk-reviews-bot]

✅ Added second reviewer: @joostjager

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 2nd Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 3rd Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[carlaKC]

First pass - haven't gone through the tests yet.

[carlaKC]

Shouldn't we be setting the option_zero_reserve feature in lightning/bolts#1140?

[tankyleo]

Discussed offline, I'll hold off on signaling for now pending further spec discussions

[tankyleo]

git diff bbe7a62 2967e6f, all significant changes in commit 5eb530d

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 8f8ab4a9b..fa394ee6e 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -15586,6 +15586,10 @@
}
let is_manual_broadcast = Some(self.context.is_manual_broadcast);
+ let has_0reserve =+ (self.funding.counterparty_selected_channel_reserve_satoshis.is_some_and(|r| r == 0)+ || self.funding.holder_selected_channel_reserve_satoshis == 0)+ .then_some(());
let holder_commitment_point_previous_revoked =
self.holder_commitment_point.previous_revoked_point();
let holder_commitment_point_last_revoked =
@@ -15655,6 +15659,7 @@
// 65 was previously used for quiescent_action
(67, pending_outbound_held_htlc_flags, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags, optional_vec), // Added in 0.2
+ (70, has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked, option), // Added in 0.3
(73, holder_commitment_point_last_revoked, option), // Added in 0.3
(75, inbound_committed_update_adds, optional_vec),
@@ -16028,6 +16033,7 @@
let mut malformed_htlcs: Option<Vec<(u64, u16, [u8; 32])>> = None;
let mut monitor_pending_update_adds: Option<Vec<msgs::UpdateAddHTLC>> = None;
+ let mut _has_0reserve: Option<()> = None;
let mut holder_commitment_point_previous_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_last_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_current_opt: Option<PublicKey> = None;
@@ -16098,6 +16104,7 @@
// 65 quiescent_action: Added in 0.2; removed in 0.3
(67, pending_outbound_held_htlc_flags_opt, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags_opt, optional_vec), // Added in 0.2
+ (70, _has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked_opt, option), // Added in 0.3
(73, holder_commitment_point_last_revoked_opt, option), // Added in 0.3
(75, inbound_committed_update_adds_opt, optional_vec),
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 432070129..184bc4052 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -3555,7 +3555,7 @@
///
/// Note that there is no guarantee that the counterparty accepts such a channel themselves.
ZeroReserve,
-	/// Sets combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]+	/// Sets the combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]
ZeroConfZeroReserve,
}

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Happy to land this to get the API changes in now, but a few small tweaks we'll want to change/address before release.

[TheBlueMatt]

I've never heard of a subtrahend 😂

[tankyleo]

haha yes I looked it up do you have a suggestion ? value_to_subtract comes to mind :)

[TheBlueMatt]

I assume this needs a bLIP update? Given this is for a bLIP and not a BOLT (IIRC?) should we not be using a +1000 or whatever range?

[tankyleo]

This is a BOLT proposal. Using the staging bit makes sense to me yes.

[TheBlueMatt]

We've always supported counterparty-selected reserve of 0, so I don't think we want to break downgrade for that?

[carlaKC]

lgtm!

[tankyleo]

Thank you for the review ! Happy to hold off on the merge and get this PR just right :)

[carlaKC]

RIP subtrahend, word of the week ✊

[tankyleo]

Fixes#1801

[ldk-reviews-bot]

👋 Thanks for assigning @carlaKC as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

❌ Patch coverage is 80.11696% with 170 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.17%. Comparing base (d830f10) to head (32a67f8).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	71.12%	148 Missing and 3 partials ⚠️
lightning/src/ln/channelmanager.rs	84.78%	14 Missing ⚠️
lightning/src/ln/msgs.rs	94.87%	4 Missing ⚠️
lightning/src/sign/tx_builder.rs	99.37%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4428 +/- ##
==========================================
- Coverage 86.18% 86.17% -0.02% 
==========================================
Files 160 160 Lines 107536 108046 +510 Branches 107536 108046 +510 ==========================================
+ Hits 92680 93105 +425 - Misses 12231 12321 +90 + Partials 2625 2620 -5 

Flag	Coverage Δ
tests	86.17% <80.11%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

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).

[TheBlueMatt]

nit: If that's the only difference let's say create_channel_to_trusted_peer_0_reserve? Nice to be explicit, imo.

[TheBlueMatt]

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...

[TheBlueMatt]

Needs rebase now :/

[tankyleo]

Needs rebase now :/

Let me know if you prefer I rebase first

[TheBlueMatt]

Feel free to go ahead and rebase and squash, yea.

[tankyleo]

Squash diff (do not click compare just above, I pushed the wrong branch, and later corrected it):

https://github.com/lightningdevkit/rust-lightning/compare/253db4d9a05cda43f74c2835448126d3205b27b8..43be438f70ba28ad7918e407026a78763ac2b368

[tankyleo]

git range-diff main 43be438 7fde002

1: bfba6e993 ! 1: b916c3596 Add inbound and outbound checks for zero reserve channels
@@ lightning/src/ln/channel.rs: impl FundingScope {
// 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(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS));
+- get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS);
- let holder_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis(
- post_channel_value,
- context.counterparty_dust_limit_satoshis,
@@ lightning/src/ln/channel.rs: impl FundingScope {
+ == 0
+ {
+ // If we previously had a 0-value reserve, continue with the same reserve
-+ Some(0)
++ 0
+ } else {
-+ Some(get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS))
++ get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS)
+ };
+ let holder_selected_channel_reserve_satoshis =
+ if prev_funding.holder_selected_channel_reserve_satoshis == 0 {
@@ lightning/src/sign/tx_builder.rs: fn get_next_commitment_stats(
);
@@ lightning/src/sign/tx_builder.rs: fn get_available_balances(
- let fee_spike_buffer_htlc =
if channel_type.supports_anchor_zero_fee_commitments() { 0 } else { 1 };
+ // Note that the feerate is 0 in zero-fee commitment channels, so this statement is a noop
- let local_feerate = feerate_per_kw
+ let spiked_feerate = feerate_per_kw
* if is_outbound_from_holder && !channel_type.supports_anchors_zero_fee_htlc_tx() {
2: 1f8ca6c66 = 2: 53fc9a354 Add 0-reserve to `accept_inbound_channel_from_trusted_peer`
3: 1b94f7cc3 = 3: da0520818 Add `ChannelManager::create_channel_to_trusted_peer_0reserve`
4: 19b8d7943 = 4: 1905f94e7 Shakedown zero reserve channels
5: de2366da4 = 5: 4e6a7527a Format `ChannelManager::create_channel_internal` and...
6: 43be438f7 = 6: 7fde002e7 Update `chanmon_consistency` to include 0FC and 0-reserve channels

[ldk-reviews-bot]

✅ Added second reviewer: @joostjager

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 2nd Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 3rd Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[carlaKC]

First pass - haven't gone through the tests yet.

[carlaKC]

Shouldn't we be setting the option_zero_reserve feature in lightning/bolts#1140?

[tankyleo]

Discussed offline, I'll hold off on signaling for now pending further spec discussions

[tankyleo]

git diff bbe7a62 2967e6f, all significant changes in commit 5eb530d

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 8f8ab4a9b..fa394ee6e 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -15586,6 +15586,10 @@
}
let is_manual_broadcast = Some(self.context.is_manual_broadcast);
+ let has_0reserve =+ (self.funding.counterparty_selected_channel_reserve_satoshis.is_some_and(|r| r == 0)+ || self.funding.holder_selected_channel_reserve_satoshis == 0)+ .then_some(());
let holder_commitment_point_previous_revoked =
self.holder_commitment_point.previous_revoked_point();
let holder_commitment_point_last_revoked =
@@ -15655,6 +15659,7 @@
// 65 was previously used for quiescent_action
(67, pending_outbound_held_htlc_flags, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags, optional_vec), // Added in 0.2
+ (70, has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked, option), // Added in 0.3
(73, holder_commitment_point_last_revoked, option), // Added in 0.3
(75, inbound_committed_update_adds, optional_vec),
@@ -16028,6 +16033,7 @@
let mut malformed_htlcs: Option<Vec<(u64, u16, [u8; 32])>> = None;
let mut monitor_pending_update_adds: Option<Vec<msgs::UpdateAddHTLC>> = None;
+ let mut _has_0reserve: Option<()> = None;
let mut holder_commitment_point_previous_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_last_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_current_opt: Option<PublicKey> = None;
@@ -16098,6 +16104,7 @@
// 65 quiescent_action: Added in 0.2; removed in 0.3
(67, pending_outbound_held_htlc_flags_opt, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags_opt, optional_vec), // Added in 0.2
+ (70, _has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked_opt, option), // Added in 0.3
(73, holder_commitment_point_last_revoked_opt, option), // Added in 0.3
(75, inbound_committed_update_adds_opt, optional_vec),
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 432070129..184bc4052 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -3555,7 +3555,7 @@
///
/// Note that there is no guarantee that the counterparty accepts such a channel themselves.
ZeroReserve,
-	/// Sets combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]+	/// Sets the combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]
ZeroConfZeroReserve,
}

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Happy to land this to get the API changes in now, but a few small tweaks we'll want to change/address before release.

[TheBlueMatt]

I've never heard of a subtrahend 😂

[tankyleo]

haha yes I looked it up do you have a suggestion ? value_to_subtract comes to mind :)

[TheBlueMatt]

I assume this needs a bLIP update? Given this is for a bLIP and not a BOLT (IIRC?) should we not be using a +1000 or whatever range?

[tankyleo]

This is a BOLT proposal. Using the staging bit makes sense to me yes.

[TheBlueMatt]

We've always supported counterparty-selected reserve of 0, so I don't think we want to break downgrade for that?

[carlaKC]

lgtm!

[tankyleo]

Thank you for the review ! Happy to hold off on the merge and get this PR just right :)

[carlaKC]

RIP subtrahend, word of the week ✊

[tankyleo]

Fixes#1801

[ldk-reviews-bot]

👋 Thanks for assigning @carlaKC as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

❌ Patch coverage is 80.11696% with 170 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.17%. Comparing base (d830f10) to head (32a67f8).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	71.12%	148 Missing and 3 partials ⚠️
lightning/src/ln/channelmanager.rs	84.78%	14 Missing ⚠️
lightning/src/ln/msgs.rs	94.87%	4 Missing ⚠️
lightning/src/sign/tx_builder.rs	99.37%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4428 +/- ##
==========================================
- Coverage 86.18% 86.17% -0.02% 
==========================================
Files 160 160 Lines 107536 108046 +510 Branches 107536 108046 +510 ==========================================
+ Hits 92680 93105 +425 - Misses 12231 12321 +90 + Partials 2625 2620 -5 

Flag	Coverage Δ
tests	86.17% <80.11%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

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).

[TheBlueMatt]

nit: If that's the only difference let's say create_channel_to_trusted_peer_0_reserve? Nice to be explicit, imo.

[TheBlueMatt]

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...

[TheBlueMatt]

Needs rebase now :/

[tankyleo]

Needs rebase now :/

Let me know if you prefer I rebase first

[TheBlueMatt]

Feel free to go ahead and rebase and squash, yea.

[tankyleo]

Squash diff (do not click compare just above, I pushed the wrong branch, and later corrected it):

https://github.com/lightningdevkit/rust-lightning/compare/253db4d9a05cda43f74c2835448126d3205b27b8..43be438f70ba28ad7918e407026a78763ac2b368

[tankyleo]

git range-diff main 43be438 7fde002

1: bfba6e993 ! 1: b916c3596 Add inbound and outbound checks for zero reserve channels
@@ lightning/src/ln/channel.rs: impl FundingScope {
// 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(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS));
+- get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS);
- let holder_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis(
- post_channel_value,
- context.counterparty_dust_limit_satoshis,
@@ lightning/src/ln/channel.rs: impl FundingScope {
+ == 0
+ {
+ // If we previously had a 0-value reserve, continue with the same reserve
-+ Some(0)
++ 0
+ } else {
-+ Some(get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS))
++ get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS)
+ };
+ let holder_selected_channel_reserve_satoshis =
+ if prev_funding.holder_selected_channel_reserve_satoshis == 0 {
@@ lightning/src/sign/tx_builder.rs: fn get_next_commitment_stats(
);
@@ lightning/src/sign/tx_builder.rs: fn get_available_balances(
- let fee_spike_buffer_htlc =
if channel_type.supports_anchor_zero_fee_commitments() { 0 } else { 1 };
+ // Note that the feerate is 0 in zero-fee commitment channels, so this statement is a noop
- let local_feerate = feerate_per_kw
+ let spiked_feerate = feerate_per_kw
* if is_outbound_from_holder && !channel_type.supports_anchors_zero_fee_htlc_tx() {
2: 1f8ca6c66 = 2: 53fc9a354 Add 0-reserve to `accept_inbound_channel_from_trusted_peer`
3: 1b94f7cc3 = 3: da0520818 Add `ChannelManager::create_channel_to_trusted_peer_0reserve`
4: 19b8d7943 = 4: 1905f94e7 Shakedown zero reserve channels
5: de2366da4 = 5: 4e6a7527a Format `ChannelManager::create_channel_internal` and...
6: 43be438f7 = 6: 7fde002e7 Update `chanmon_consistency` to include 0FC and 0-reserve channels

[ldk-reviews-bot]

✅ Added second reviewer: @joostjager

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 2nd Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 3rd Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[carlaKC]

First pass - haven't gone through the tests yet.

[carlaKC]

Shouldn't we be setting the option_zero_reserve feature in lightning/bolts#1140?

[tankyleo]

Discussed offline, I'll hold off on signaling for now pending further spec discussions

[tankyleo]

git diff bbe7a62 2967e6f, all significant changes in commit 5eb530d

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 8f8ab4a9b..fa394ee6e 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -15586,6 +15586,10 @@
}
let is_manual_broadcast = Some(self.context.is_manual_broadcast);
+ let has_0reserve =+ (self.funding.counterparty_selected_channel_reserve_satoshis.is_some_and(|r| r == 0)+ || self.funding.holder_selected_channel_reserve_satoshis == 0)+ .then_some(());
let holder_commitment_point_previous_revoked =
self.holder_commitment_point.previous_revoked_point();
let holder_commitment_point_last_revoked =
@@ -15655,6 +15659,7 @@
// 65 was previously used for quiescent_action
(67, pending_outbound_held_htlc_flags, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags, optional_vec), // Added in 0.2
+ (70, has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked, option), // Added in 0.3
(73, holder_commitment_point_last_revoked, option), // Added in 0.3
(75, inbound_committed_update_adds, optional_vec),
@@ -16028,6 +16033,7 @@
let mut malformed_htlcs: Option<Vec<(u64, u16, [u8; 32])>> = None;
let mut monitor_pending_update_adds: Option<Vec<msgs::UpdateAddHTLC>> = None;
+ let mut _has_0reserve: Option<()> = None;
let mut holder_commitment_point_previous_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_last_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_current_opt: Option<PublicKey> = None;
@@ -16098,6 +16104,7 @@
// 65 quiescent_action: Added in 0.2; removed in 0.3
(67, pending_outbound_held_htlc_flags_opt, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags_opt, optional_vec), // Added in 0.2
+ (70, _has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked_opt, option), // Added in 0.3
(73, holder_commitment_point_last_revoked_opt, option), // Added in 0.3
(75, inbound_committed_update_adds_opt, optional_vec),
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 432070129..184bc4052 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -3555,7 +3555,7 @@
///
/// Note that there is no guarantee that the counterparty accepts such a channel themselves.
ZeroReserve,
-	/// Sets combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]+	/// Sets the combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]
ZeroConfZeroReserve,
}

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Happy to land this to get the API changes in now, but a few small tweaks we'll want to change/address before release.

[TheBlueMatt]

I've never heard of a subtrahend 😂

[tankyleo]

haha yes I looked it up do you have a suggestion ? value_to_subtract comes to mind :)

[TheBlueMatt]

I assume this needs a bLIP update? Given this is for a bLIP and not a BOLT (IIRC?) should we not be using a +1000 or whatever range?

[tankyleo]

This is a BOLT proposal. Using the staging bit makes sense to me yes.

[TheBlueMatt]

We've always supported counterparty-selected reserve of 0, so I don't think we want to break downgrade for that?

[carlaKC]

lgtm!

[tankyleo]

Thank you for the review ! Happy to hold off on the merge and get this PR just right :)

[carlaKC]

RIP subtrahend, word of the week ✊

[tankyleo]

Fixes#1801

[ldk-reviews-bot]

👋 Thanks for assigning @carlaKC as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

❌ Patch coverage is 80.11696% with 170 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.17%. Comparing base (d830f10) to head (32a67f8).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	71.12%	148 Missing and 3 partials ⚠️
lightning/src/ln/channelmanager.rs	84.78%	14 Missing ⚠️
lightning/src/ln/msgs.rs	94.87%	4 Missing ⚠️
lightning/src/sign/tx_builder.rs	99.37%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4428 +/- ##
==========================================
- Coverage 86.18% 86.17% -0.02% 
==========================================
Files 160 160 Lines 107536 108046 +510 Branches 107536 108046 +510 ==========================================
+ Hits 92680 93105 +425 - Misses 12231 12321 +90 + Partials 2625 2620 -5 

Flag	Coverage Δ
tests	86.17% <80.11%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

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).

[TheBlueMatt]

nit: If that's the only difference let's say create_channel_to_trusted_peer_0_reserve? Nice to be explicit, imo.

[TheBlueMatt]

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...

[TheBlueMatt]

Needs rebase now :/

[tankyleo]

Needs rebase now :/

Let me know if you prefer I rebase first

[TheBlueMatt]

Feel free to go ahead and rebase and squash, yea.

[tankyleo]

Squash diff (do not click compare just above, I pushed the wrong branch, and later corrected it):

https://github.com/lightningdevkit/rust-lightning/compare/253db4d9a05cda43f74c2835448126d3205b27b8..43be438f70ba28ad7918e407026a78763ac2b368

[tankyleo]

git range-diff main 43be438 7fde002

1: bfba6e993 ! 1: b916c3596 Add inbound and outbound checks for zero reserve channels
@@ lightning/src/ln/channel.rs: impl FundingScope {
// 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(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS));
+- get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS);
- let holder_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis(
- post_channel_value,
- context.counterparty_dust_limit_satoshis,
@@ lightning/src/ln/channel.rs: impl FundingScope {
+ == 0
+ {
+ // If we previously had a 0-value reserve, continue with the same reserve
-+ Some(0)
++ 0
+ } else {
-+ Some(get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS))
++ get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS)
+ };
+ let holder_selected_channel_reserve_satoshis =
+ if prev_funding.holder_selected_channel_reserve_satoshis == 0 {
@@ lightning/src/sign/tx_builder.rs: fn get_next_commitment_stats(
);
@@ lightning/src/sign/tx_builder.rs: fn get_available_balances(
- let fee_spike_buffer_htlc =
if channel_type.supports_anchor_zero_fee_commitments() { 0 } else { 1 };
+ // Note that the feerate is 0 in zero-fee commitment channels, so this statement is a noop
- let local_feerate = feerate_per_kw
+ let spiked_feerate = feerate_per_kw
* if is_outbound_from_holder && !channel_type.supports_anchors_zero_fee_htlc_tx() {
2: 1f8ca6c66 = 2: 53fc9a354 Add 0-reserve to `accept_inbound_channel_from_trusted_peer`
3: 1b94f7cc3 = 3: da0520818 Add `ChannelManager::create_channel_to_trusted_peer_0reserve`
4: 19b8d7943 = 4: 1905f94e7 Shakedown zero reserve channels
5: de2366da4 = 5: 4e6a7527a Format `ChannelManager::create_channel_internal` and...
6: 43be438f7 = 6: 7fde002e7 Update `chanmon_consistency` to include 0FC and 0-reserve channels

[ldk-reviews-bot]

✅ Added second reviewer: @joostjager

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 2nd Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 3rd Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[carlaKC]

First pass - haven't gone through the tests yet.

[carlaKC]

Shouldn't we be setting the option_zero_reserve feature in lightning/bolts#1140?

[tankyleo]

Discussed offline, I'll hold off on signaling for now pending further spec discussions

[tankyleo]

git diff bbe7a62 2967e6f, all significant changes in commit 5eb530d

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 8f8ab4a9b..fa394ee6e 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -15586,6 +15586,10 @@
}
let is_manual_broadcast = Some(self.context.is_manual_broadcast);
+ let has_0reserve =+ (self.funding.counterparty_selected_channel_reserve_satoshis.is_some_and(|r| r == 0)+ || self.funding.holder_selected_channel_reserve_satoshis == 0)+ .then_some(());
let holder_commitment_point_previous_revoked =
self.holder_commitment_point.previous_revoked_point();
let holder_commitment_point_last_revoked =
@@ -15655,6 +15659,7 @@
// 65 was previously used for quiescent_action
(67, pending_outbound_held_htlc_flags, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags, optional_vec), // Added in 0.2
+ (70, has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked, option), // Added in 0.3
(73, holder_commitment_point_last_revoked, option), // Added in 0.3
(75, inbound_committed_update_adds, optional_vec),
@@ -16028,6 +16033,7 @@
let mut malformed_htlcs: Option<Vec<(u64, u16, [u8; 32])>> = None;
let mut monitor_pending_update_adds: Option<Vec<msgs::UpdateAddHTLC>> = None;
+ let mut _has_0reserve: Option<()> = None;
let mut holder_commitment_point_previous_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_last_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_current_opt: Option<PublicKey> = None;
@@ -16098,6 +16104,7 @@
// 65 quiescent_action: Added in 0.2; removed in 0.3
(67, pending_outbound_held_htlc_flags_opt, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags_opt, optional_vec), // Added in 0.2
+ (70, _has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked_opt, option), // Added in 0.3
(73, holder_commitment_point_last_revoked_opt, option), // Added in 0.3
(75, inbound_committed_update_adds_opt, optional_vec),
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 432070129..184bc4052 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -3555,7 +3555,7 @@
///
/// Note that there is no guarantee that the counterparty accepts such a channel themselves.
ZeroReserve,
-	/// Sets combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]+	/// Sets the combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]
ZeroConfZeroReserve,
}

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Happy to land this to get the API changes in now, but a few small tweaks we'll want to change/address before release.

[TheBlueMatt]

I've never heard of a subtrahend 😂

[tankyleo]

haha yes I looked it up do you have a suggestion ? value_to_subtract comes to mind :)

[TheBlueMatt]

I assume this needs a bLIP update? Given this is for a bLIP and not a BOLT (IIRC?) should we not be using a +1000 or whatever range?

[tankyleo]

This is a BOLT proposal. Using the staging bit makes sense to me yes.

[TheBlueMatt]

We've always supported counterparty-selected reserve of 0, so I don't think we want to break downgrade for that?

[carlaKC]

lgtm!

[tankyleo]

Thank you for the review ! Happy to hold off on the merge and get this PR just right :)

[carlaKC]

RIP subtrahend, word of the week ✊

[tankyleo]

Fixes#1801

[ldk-reviews-bot]

👋 Thanks for assigning @carlaKC as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

❌ Patch coverage is 80.11696% with 170 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.17%. Comparing base (d830f10) to head (32a67f8).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	71.12%	148 Missing and 3 partials ⚠️
lightning/src/ln/channelmanager.rs	84.78%	14 Missing ⚠️
lightning/src/ln/msgs.rs	94.87%	4 Missing ⚠️
lightning/src/sign/tx_builder.rs	99.37%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4428 +/- ##
==========================================
- Coverage 86.18% 86.17% -0.02% 
==========================================
Files 160 160 Lines 107536 108046 +510 Branches 107536 108046 +510 ==========================================
+ Hits 92680 93105 +425 - Misses 12231 12321 +90 + Partials 2625 2620 -5 

Flag	Coverage Δ
tests	86.17% <80.11%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

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).

[TheBlueMatt]

nit: If that's the only difference let's say create_channel_to_trusted_peer_0_reserve? Nice to be explicit, imo.

[TheBlueMatt]

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...

[TheBlueMatt]

Needs rebase now :/

[tankyleo]

Needs rebase now :/

Let me know if you prefer I rebase first

[TheBlueMatt]

Feel free to go ahead and rebase and squash, yea.

[tankyleo]

Squash diff (do not click compare just above, I pushed the wrong branch, and later corrected it):

https://github.com/lightningdevkit/rust-lightning/compare/253db4d9a05cda43f74c2835448126d3205b27b8..43be438f70ba28ad7918e407026a78763ac2b368

[tankyleo]

git range-diff main 43be438 7fde002

1: bfba6e993 ! 1: b916c3596 Add inbound and outbound checks for zero reserve channels
@@ lightning/src/ln/channel.rs: impl FundingScope {
// 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(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS));
+- get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS);
- let holder_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis(
- post_channel_value,
- context.counterparty_dust_limit_satoshis,
@@ lightning/src/ln/channel.rs: impl FundingScope {
+ == 0
+ {
+ // If we previously had a 0-value reserve, continue with the same reserve
-+ Some(0)
++ 0
+ } else {
-+ Some(get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS))
++ get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS)
+ };
+ let holder_selected_channel_reserve_satoshis =
+ if prev_funding.holder_selected_channel_reserve_satoshis == 0 {
@@ lightning/src/sign/tx_builder.rs: fn get_next_commitment_stats(
);
@@ lightning/src/sign/tx_builder.rs: fn get_available_balances(
- let fee_spike_buffer_htlc =
if channel_type.supports_anchor_zero_fee_commitments() { 0 } else { 1 };
+ // Note that the feerate is 0 in zero-fee commitment channels, so this statement is a noop
- let local_feerate = feerate_per_kw
+ let spiked_feerate = feerate_per_kw
* if is_outbound_from_holder && !channel_type.supports_anchors_zero_fee_htlc_tx() {
2: 1f8ca6c66 = 2: 53fc9a354 Add 0-reserve to `accept_inbound_channel_from_trusted_peer`
3: 1b94f7cc3 = 3: da0520818 Add `ChannelManager::create_channel_to_trusted_peer_0reserve`
4: 19b8d7943 = 4: 1905f94e7 Shakedown zero reserve channels
5: de2366da4 = 5: 4e6a7527a Format `ChannelManager::create_channel_internal` and...
6: 43be438f7 = 6: 7fde002e7 Update `chanmon_consistency` to include 0FC and 0-reserve channels

[ldk-reviews-bot]

✅ Added second reviewer: @joostjager

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 2nd Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 3rd Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[carlaKC]

First pass - haven't gone through the tests yet.

[carlaKC]

Shouldn't we be setting the option_zero_reserve feature in lightning/bolts#1140?

[tankyleo]

Discussed offline, I'll hold off on signaling for now pending further spec discussions

[tankyleo]

git diff bbe7a62 2967e6f, all significant changes in commit 5eb530d

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 8f8ab4a9b..fa394ee6e 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -15586,6 +15586,10 @@
}
let is_manual_broadcast = Some(self.context.is_manual_broadcast);
+ let has_0reserve =+ (self.funding.counterparty_selected_channel_reserve_satoshis.is_some_and(|r| r == 0)+ || self.funding.holder_selected_channel_reserve_satoshis == 0)+ .then_some(());
let holder_commitment_point_previous_revoked =
self.holder_commitment_point.previous_revoked_point();
let holder_commitment_point_last_revoked =
@@ -15655,6 +15659,7 @@
// 65 was previously used for quiescent_action
(67, pending_outbound_held_htlc_flags, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags, optional_vec), // Added in 0.2
+ (70, has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked, option), // Added in 0.3
(73, holder_commitment_point_last_revoked, option), // Added in 0.3
(75, inbound_committed_update_adds, optional_vec),
@@ -16028,6 +16033,7 @@
let mut malformed_htlcs: Option<Vec<(u64, u16, [u8; 32])>> = None;
let mut monitor_pending_update_adds: Option<Vec<msgs::UpdateAddHTLC>> = None;
+ let mut _has_0reserve: Option<()> = None;
let mut holder_commitment_point_previous_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_last_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_current_opt: Option<PublicKey> = None;
@@ -16098,6 +16104,7 @@
// 65 quiescent_action: Added in 0.2; removed in 0.3
(67, pending_outbound_held_htlc_flags_opt, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags_opt, optional_vec), // Added in 0.2
+ (70, _has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked_opt, option), // Added in 0.3
(73, holder_commitment_point_last_revoked_opt, option), // Added in 0.3
(75, inbound_committed_update_adds_opt, optional_vec),
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 432070129..184bc4052 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -3555,7 +3555,7 @@
///
/// Note that there is no guarantee that the counterparty accepts such a channel themselves.
ZeroReserve,
-	/// Sets combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]+	/// Sets the combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]
ZeroConfZeroReserve,
}

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Happy to land this to get the API changes in now, but a few small tweaks we'll want to change/address before release.

[TheBlueMatt]

I've never heard of a subtrahend 😂

[tankyleo]

haha yes I looked it up do you have a suggestion ? value_to_subtract comes to mind :)

[TheBlueMatt]

I assume this needs a bLIP update? Given this is for a bLIP and not a BOLT (IIRC?) should we not be using a +1000 or whatever range?

[tankyleo]

This is a BOLT proposal. Using the staging bit makes sense to me yes.

[TheBlueMatt]

We've always supported counterparty-selected reserve of 0, so I don't think we want to break downgrade for that?

[carlaKC]

lgtm!

[tankyleo]

Thank you for the review ! Happy to hold off on the merge and get this PR just right :)

[carlaKC]

RIP subtrahend, word of the week ✊

[tankyleo]

Fixes#1801

[ldk-reviews-bot]

👋 Thanks for assigning @carlaKC as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

❌ Patch coverage is 80.11696% with 170 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.17%. Comparing base (d830f10) to head (32a67f8).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	71.12%	148 Missing and 3 partials ⚠️
lightning/src/ln/channelmanager.rs	84.78%	14 Missing ⚠️
lightning/src/ln/msgs.rs	94.87%	4 Missing ⚠️
lightning/src/sign/tx_builder.rs	99.37%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4428 +/- ##
==========================================
- Coverage 86.18% 86.17% -0.02% 
==========================================
Files 160 160 Lines 107536 108046 +510 Branches 107536 108046 +510 ==========================================
+ Hits 92680 93105 +425 - Misses 12231 12321 +90 + Partials 2625 2620 -5 

Flag	Coverage Δ
tests	86.17% <80.11%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

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).

[TheBlueMatt]

nit: If that's the only difference let's say create_channel_to_trusted_peer_0_reserve? Nice to be explicit, imo.

[TheBlueMatt]

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...

[TheBlueMatt]

Needs rebase now :/

[tankyleo]

Needs rebase now :/

Let me know if you prefer I rebase first

[TheBlueMatt]

Feel free to go ahead and rebase and squash, yea.

[tankyleo]

Squash diff (do not click compare just above, I pushed the wrong branch, and later corrected it):

https://github.com/lightningdevkit/rust-lightning/compare/253db4d9a05cda43f74c2835448126d3205b27b8..43be438f70ba28ad7918e407026a78763ac2b368

[tankyleo]

git range-diff main 43be438 7fde002

1: bfba6e993 ! 1: b916c3596 Add inbound and outbound checks for zero reserve channels
@@ lightning/src/ln/channel.rs: impl FundingScope {
// 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(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS));
+- get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS);
- let holder_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis(
- post_channel_value,
- context.counterparty_dust_limit_satoshis,
@@ lightning/src/ln/channel.rs: impl FundingScope {
+ == 0
+ {
+ // If we previously had a 0-value reserve, continue with the same reserve
-+ Some(0)
++ 0
+ } else {
-+ Some(get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS))
++ get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS)
+ };
+ let holder_selected_channel_reserve_satoshis =
+ if prev_funding.holder_selected_channel_reserve_satoshis == 0 {
@@ lightning/src/sign/tx_builder.rs: fn get_next_commitment_stats(
);
@@ lightning/src/sign/tx_builder.rs: fn get_available_balances(
- let fee_spike_buffer_htlc =
if channel_type.supports_anchor_zero_fee_commitments() { 0 } else { 1 };
+ // Note that the feerate is 0 in zero-fee commitment channels, so this statement is a noop
- let local_feerate = feerate_per_kw
+ let spiked_feerate = feerate_per_kw
* if is_outbound_from_holder && !channel_type.supports_anchors_zero_fee_htlc_tx() {
2: 1f8ca6c66 = 2: 53fc9a354 Add 0-reserve to `accept_inbound_channel_from_trusted_peer`
3: 1b94f7cc3 = 3: da0520818 Add `ChannelManager::create_channel_to_trusted_peer_0reserve`
4: 19b8d7943 = 4: 1905f94e7 Shakedown zero reserve channels
5: de2366da4 = 5: 4e6a7527a Format `ChannelManager::create_channel_internal` and...
6: 43be438f7 = 6: 7fde002e7 Update `chanmon_consistency` to include 0FC and 0-reserve channels

[ldk-reviews-bot]

✅ Added second reviewer: @joostjager

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 2nd Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 3rd Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[carlaKC]

First pass - haven't gone through the tests yet.

[carlaKC]

Shouldn't we be setting the option_zero_reserve feature in lightning/bolts#1140?

[tankyleo]

Discussed offline, I'll hold off on signaling for now pending further spec discussions

[tankyleo]

git diff bbe7a62 2967e6f, all significant changes in commit 5eb530d

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 8f8ab4a9b..fa394ee6e 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -15586,6 +15586,10 @@
}
let is_manual_broadcast = Some(self.context.is_manual_broadcast);
+ let has_0reserve =+ (self.funding.counterparty_selected_channel_reserve_satoshis.is_some_and(|r| r == 0)+ || self.funding.holder_selected_channel_reserve_satoshis == 0)+ .then_some(());
let holder_commitment_point_previous_revoked =
self.holder_commitment_point.previous_revoked_point();
let holder_commitment_point_last_revoked =
@@ -15655,6 +15659,7 @@
// 65 was previously used for quiescent_action
(67, pending_outbound_held_htlc_flags, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags, optional_vec), // Added in 0.2
+ (70, has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked, option), // Added in 0.3
(73, holder_commitment_point_last_revoked, option), // Added in 0.3
(75, inbound_committed_update_adds, optional_vec),
@@ -16028,6 +16033,7 @@
let mut malformed_htlcs: Option<Vec<(u64, u16, [u8; 32])>> = None;
let mut monitor_pending_update_adds: Option<Vec<msgs::UpdateAddHTLC>> = None;
+ let mut _has_0reserve: Option<()> = None;
let mut holder_commitment_point_previous_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_last_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_current_opt: Option<PublicKey> = None;
@@ -16098,6 +16104,7 @@
// 65 quiescent_action: Added in 0.2; removed in 0.3
(67, pending_outbound_held_htlc_flags_opt, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags_opt, optional_vec), // Added in 0.2
+ (70, _has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked_opt, option), // Added in 0.3
(73, holder_commitment_point_last_revoked_opt, option), // Added in 0.3
(75, inbound_committed_update_adds_opt, optional_vec),
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 432070129..184bc4052 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -3555,7 +3555,7 @@
///
/// Note that there is no guarantee that the counterparty accepts such a channel themselves.
ZeroReserve,
-	/// Sets combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]+	/// Sets the combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]
ZeroConfZeroReserve,
}

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Happy to land this to get the API changes in now, but a few small tweaks we'll want to change/address before release.

[TheBlueMatt]

I've never heard of a subtrahend 😂

[tankyleo]

haha yes I looked it up do you have a suggestion ? value_to_subtract comes to mind :)

[TheBlueMatt]

I assume this needs a bLIP update? Given this is for a bLIP and not a BOLT (IIRC?) should we not be using a +1000 or whatever range?

[tankyleo]

This is a BOLT proposal. Using the staging bit makes sense to me yes.

[TheBlueMatt]

We've always supported counterparty-selected reserve of 0, so I don't think we want to break downgrade for that?

[carlaKC]

lgtm!

[tankyleo]

Thank you for the review ! Happy to hold off on the merge and get this PR just right :)

[carlaKC]

RIP subtrahend, word of the week ✊

[tankyleo]

Fixes#1801

[ldk-reviews-bot]

👋 Thanks for assigning @carlaKC as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

❌ Patch coverage is 80.11696% with 170 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.17%. Comparing base (d830f10) to head (32a67f8).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	71.12%	148 Missing and 3 partials ⚠️
lightning/src/ln/channelmanager.rs	84.78%	14 Missing ⚠️
lightning/src/ln/msgs.rs	94.87%	4 Missing ⚠️
lightning/src/sign/tx_builder.rs	99.37%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #4428 +/- ##
==========================================
- Coverage 86.18% 86.17% -0.02% 
==========================================
Files 160 160 Lines 107536 108046 +510 Branches 107536 108046 +510 ==========================================
+ Hits 92680 93105 +425 - Misses 12231 12321 +90 + Partials 2625 2620 -5 

Flag	Coverage Δ
tests	86.17% <80.11%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

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).

[TheBlueMatt]

nit: If that's the only difference let's say create_channel_to_trusted_peer_0_reserve? Nice to be explicit, imo.

[TheBlueMatt]

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...

[TheBlueMatt]

Needs rebase now :/

[tankyleo]

Needs rebase now :/

Let me know if you prefer I rebase first

[TheBlueMatt]

Feel free to go ahead and rebase and squash, yea.

[tankyleo]

Squash diff (do not click compare just above, I pushed the wrong branch, and later corrected it):

https://github.com/lightningdevkit/rust-lightning/compare/253db4d9a05cda43f74c2835448126d3205b27b8..43be438f70ba28ad7918e407026a78763ac2b368

[tankyleo]

git range-diff main 43be438 7fde002

1: bfba6e993 ! 1: b916c3596 Add inbound and outbound checks for zero reserve channels
@@ lightning/src/ln/channel.rs: impl FundingScope {
// 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(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS));
+- get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS);
- let holder_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis(
- post_channel_value,
- context.counterparty_dust_limit_satoshis,
@@ lightning/src/ln/channel.rs: impl FundingScope {
+ == 0
+ {
+ // If we previously had a 0-value reserve, continue with the same reserve
-+ Some(0)
++ 0
+ } else {
-+ Some(get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS))
++ get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS)
+ };
+ let holder_selected_channel_reserve_satoshis =
+ if prev_funding.holder_selected_channel_reserve_satoshis == 0 {
@@ lightning/src/sign/tx_builder.rs: fn get_next_commitment_stats(
);
@@ lightning/src/sign/tx_builder.rs: fn get_available_balances(
- let fee_spike_buffer_htlc =
if channel_type.supports_anchor_zero_fee_commitments() { 0 } else { 1 };
+ // Note that the feerate is 0 in zero-fee commitment channels, so this statement is a noop
- let local_feerate = feerate_per_kw
+ let spiked_feerate = feerate_per_kw
* if is_outbound_from_holder && !channel_type.supports_anchors_zero_fee_htlc_tx() {
2: 1f8ca6c66 = 2: 53fc9a354 Add 0-reserve to `accept_inbound_channel_from_trusted_peer`
3: 1b94f7cc3 = 3: da0520818 Add `ChannelManager::create_channel_to_trusted_peer_0reserve`
4: 19b8d7943 = 4: 1905f94e7 Shakedown zero reserve channels
5: de2366da4 = 5: 4e6a7527a Format `ChannelManager::create_channel_internal` and...
6: 43be438f7 = 6: 7fde002e7 Update `chanmon_consistency` to include 0FC and 0-reserve channels

[ldk-reviews-bot]

✅ Added second reviewer: @joostjager

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 2nd Reminder

Hey @TheBlueMatt@joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[ldk-reviews-bot]

🔔 3rd Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[carlaKC]

First pass - haven't gone through the tests yet.

[carlaKC]

Shouldn't we be setting the option_zero_reserve feature in lightning/bolts#1140?

[tankyleo]

Discussed offline, I'll hold off on signaling for now pending further spec discussions

[tankyleo]

git diff bbe7a62 2967e6f, all significant changes in commit 5eb530d

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 8f8ab4a9b..fa394ee6e 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -15586,6 +15586,10 @@
}
let is_manual_broadcast = Some(self.context.is_manual_broadcast);
+ let has_0reserve =+ (self.funding.counterparty_selected_channel_reserve_satoshis.is_some_and(|r| r == 0)+ || self.funding.holder_selected_channel_reserve_satoshis == 0)+ .then_some(());
let holder_commitment_point_previous_revoked =
self.holder_commitment_point.previous_revoked_point();
let holder_commitment_point_last_revoked =
@@ -15655,6 +15659,7 @@
// 65 was previously used for quiescent_action
(67, pending_outbound_held_htlc_flags, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags, optional_vec), // Added in 0.2
+ (70, has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked, option), // Added in 0.3
(73, holder_commitment_point_last_revoked, option), // Added in 0.3
(75, inbound_committed_update_adds, optional_vec),
@@ -16028,6 +16033,7 @@
let mut malformed_htlcs: Option<Vec<(u64, u16, [u8; 32])>> = None;
let mut monitor_pending_update_adds: Option<Vec<msgs::UpdateAddHTLC>> = None;
+ let mut _has_0reserve: Option<()> = None;
let mut holder_commitment_point_previous_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_last_revoked_opt: Option<PublicKey> = None;
let mut holder_commitment_point_current_opt: Option<PublicKey> = None;
@@ -16098,6 +16104,7 @@
// 65 quiescent_action: Added in 0.2; removed in 0.3
(67, pending_outbound_held_htlc_flags_opt, optional_vec), // Added in 0.2
(69, holding_cell_held_htlc_flags_opt, optional_vec), // Added in 0.2
+ (70, _has_0reserve, option), // Added in 0.3 to prevent downgrades
(71, holder_commitment_point_previous_revoked_opt, option), // Added in 0.3
(73, holder_commitment_point_last_revoked_opt, option), // Added in 0.3
(75, inbound_committed_update_adds_opt, optional_vec),
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 432070129..184bc4052 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -3555,7 +3555,7 @@
///
/// Note that there is no guarantee that the counterparty accepts such a channel themselves.
ZeroReserve,
-	/// Sets combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]+	/// Sets the combination of [`TrustedChannelFeatures::ZeroConf`] and [`TrustedChannelFeatures::ZeroReserve`]
ZeroConfZeroReserve,
}

[ldk-reviews-bot]

🔔 1st Reminder

Hey @TheBlueMatt@carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[TheBlueMatt]

Happy to land this to get the API changes in now, but a few small tweaks we'll want to change/address before release.

[TheBlueMatt]

I've never heard of a subtrahend 😂

[tankyleo]

haha yes I looked it up do you have a suggestion ? value_to_subtract comes to mind :)

[TheBlueMatt]

I assume this needs a bLIP update? Given this is for a bLIP and not a BOLT (IIRC?) should we not be using a +1000 or whatever range?

[tankyleo]

This is a BOLT proposal. Using the staging bit makes sense to me yes.

[TheBlueMatt]

We've always supported counterparty-selected reserve of 0, so I don't think we want to break downgrade for that?

[carlaKC]

lgtm!

[tankyleo]

Thank you for the review ! Happy to hold off on the merge and get this PR just right :)

[carlaKC]

RIP subtrahend, word of the week ✊