-
Notifications
You must be signed in to change notification settings - Fork 407
Channel Establishment for V3 Channels #3792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Useful for the commits that follow where we add more downgrade tests.
👋 Thanks for assigning @tankyleo as a reviewer! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, basically all LGTM.
@@ -16153,22 +16154,28 @@ mod tests { | |||
} | |||
|
|||
#[test] | |||
fn test_anchors_zero_fee_htlc_tx_fallback() { | |||
fn test_anchors_zero_fee_htlc_tx_downgrade() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats, you touched it, now you get to move it out of channelmanager
into some other test-specific file that isn't 15000 lines of code 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done this in a follow up in #3797 so that move + format can be reviewed separately.
lightning/src/util/config.rs
Outdated
/// back to a `anchors_zero_fee_htlc` (if [`Self::negotiate_anchors_zero_fee_htlc_tx`] | ||
/// is set) or `static_remote_key` channel. | ||
/// | ||
/// *Implies [`Self::negotiate_anchors_zero_fee_htlc_tx`].* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are in conflict - one says we'll fall back if its set, the other says that its implied (ie always set) if we set this flag.
👋 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. |
✅ Added second reviewer: @valentinewallace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after Matt's comments are addressed
3a36506
to
b9ec89c
Compare
Removed conflicting docs statement + opened followup for test separation (felt wrong to do move+format in the same PR, happy to include if we want it in here). |
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, here's a first pass :)
lightning/src/ln/channel.rs
Outdated
// `provided_channel_type_features`. The channel type must always support | ||
// `static_remote_key`. | ||
if !channel_type.requires_static_remote_key() { | ||
// `static_remote_key`, except for `option_zero_fee_commitments` which | ||
// assumes this feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feel free to dismiss: would reword the "except" here to something like this: "The channel type must always support static_remote_key
, either implicitly via option_zero_fee_commitments
or explicitly.
lightning/src/ln/channelmanager.rs
Outdated
#[cfg(test)] | ||
{ | ||
if config.channel_handshake_config.negotiate_anchor_zero_fee_commitments { | ||
features.set_anchor_zero_fee_commitments_optional(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit I believe we can remove one level of indentation:
#[cfg(test)]
if config.channel_handshake_config.negotiate_anchor_zero_fee_commitments {
features.set_anchor_zero_fee_commitments_optional();
}
let (commitment_feerate, anchor_outputs_value_msat) = | ||
if channel_type.supports_anchor_zero_fee_commitments() { | ||
(0, 0) | ||
} else if channel_type.supports_anchors_zero_fee_htlc_tx() { | ||
let feerate = fee_estimator | ||
.bounded_sat_per_1000_weight(ConfirmationTarget::AnchorChannelFee); | ||
(feerate, ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000) | ||
} else { | ||
let feerate = fee_estimator | ||
.bounded_sat_per_1000_weight(ConfirmationTarget::NonAnchorChannelFee); | ||
(feerate, 0) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to rebase TxBuilder
on top of this :) The goal with the TxBuilder
PR will be to remove ANCHOR_OUTPUT_VALUE_SATOSHI
and chan_utils::commit_tx_fee_sat
from channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do this here, shouldn't we do something similar in new_for_inbound
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal with the TxBuilder PR will be to remove ANCHOR_OUTPUT_VALUE_SATOSHI and chan_utils::commit_tx_fee_sat from channel.
Didn't realize the two would be overlapping so soon - thanks that would be great 🙏
If we do this here, shouldn't we do something similar in new_for_inbound ?
"do this here" meaning account for the anchor in our balance?
Since we don't need to set feerate for inbound, we've just got a check to set anchor balance when using legacy anchors - we don't need to add anything here because our anchor amount is also zero for zero_fee_commitments
.
Can drop a comment there noting that the new anchor type also has zero anchors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes ! All good, feel free to add a comment; it was sloppy on my side, staring a the code again made it click :)
I know we asserted that open_channel_fields.commitment_feerate_sat_per_1000_weight
was 0 for zero-fee commitments further above via check_remote_fee
, but wondering if we should add a quick debug assert right before the "can't even pay for initial commitment transaction fee of x sats" in new_for_inbound_channel
? Something like this?
+ // Sanity check that `commitment_tx_fee` is 0 for zero fee commitments
+ debug_assert!(!channel_type.supports_anchor_zero_fee_commitments() || commitment_tx_fee == 0)
if (funders_amount_msat / 1000).saturating_sub(anchor_outputs_value) < commitment_tx_fee {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug_assert!(!channel_type.supports_anchor_zero_fee_commitments() || commitment_tx_fee == 0)
I don't think that this adds much, given the checks that we have in check_remote_fee
?
- We've already errored out if
supports_anchor_zero_fee_commitments
isn't zero fee - All other channel types are checked against
bounded_sat_per_1000_weight
(which can't be smaller thanFEERATE_FLOOR_SATS_PER_KW=253
), which implicitly checks that they're non-zero
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it doesn't add anything beyond check_remote_fee
, I was thinking just in case someone in the future interferes with the ordering of these checks somehow. The check_remote_fee
check is "pretty far above" this point, so someone could easily miss that they depend on each other ?
I let you make the call here thank you for your answers :)
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another pass :)
lightning/src/ln/channel.rs
Outdated
if !channel_type.requires_static_remote_key() { | ||
// `static_remote_key`, except for `option_zero_fee_commitments` which | ||
// assumes this feature. | ||
if !channel_type.requires_static_remote_key() && !channel_type.requires_anchor_zero_fee_commitments(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing space before {
lightning/src/ln/channelmanager.rs
Outdated
// remote node supports the feature but does not accept it. | ||
let mut zero_fee_config = test_default_channel_config(); | ||
zero_fee_config.channel_handshake_config.negotiate_anchor_zero_fee_commitments = true; | ||
zero_fee_config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also test the case where negotiate_zero_fee_commitments = true; negotiate_zero_fee_htlc_tx = false
? And if so, assert that we downgrade from zero fee commitments directly to static remote key, skipping zero fee htlcs ?
lightning/src/ln/channel.rs
Outdated
let mut expected_channel_type = ChannelTypeFeatures::empty(); | ||
expected_channel_type.set_static_remote_key_required(); | ||
expected_channel_type.set_anchors_zero_fee_htlc_tx_required(); | ||
assert!(channel_a.funding.get_channel_type() != &expected_channel_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would replace this assert with:
assert!(channel_a.funding.get_channel_type() == &ChannelTypeFeatures::only_static_remote_key());
My thinking is if the sender node signals both zero_fee_htlc
and zero_fee_commitments
, but the receiver node does not support either of them, we should only accept ChannelTypeFeatures::only_static_remote_key()
here. Right now we would also accept ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()
here, which would be incorrect.
lightning/src/ln/channel.rs
Outdated
if channel_type.supports_anchor_zero_fee_commitments() { | ||
channel_type.clear_anchor_zero_fee_commitments(); | ||
channel_type.set_anchors_zero_fee_htlc_tx_required(); | ||
channel_type.set_static_remote_key_required(); | ||
|
||
self.feerate_per_kw = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::AnchorChannelFee); | ||
assert!(!channel_type.supports_anchor_zero_fee_commitments()); | ||
assert!(channel_type.supports_anchors_zero_fee_htlc_tx()); | ||
assert!(channel_type.supports_static_remote_key()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make sure here that ChannelHandshakeConfig.negotiate_zero_fee_htlc_tx == true
before doing this downgrade ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, good catch! I think that this is actually an issue on both ends?
- We have to check that we want to negotiate this type
- We need to check that the remote actually supports
anchors_zero_fee_htlc_tx_required
Previously this wasn't an issue because the static_remote
is a dependency of anchors_zero_fee_htlc_tx
- if they support anchors_zero_fee_htlc_tx
we know that we can drop down to static_remote
.
But now if they've set anchor_zero_fee_commitments
we can't just assume that they support anchors_zero_fee_htlc_tx
without going back to look at InitFeatures
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is actually an issue on both ends?
Yes !
But now if they've set anchor_zero_fee_commitments we can't just assume that they support anchors_zero_fee_htlc_tx without going back to look at InitFeatures?
Could we assume they have set it, and if not, we get another error back, and we come back here to downgrade to static remote key ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we assume they have set it, and if not, we get another error back, and we come back here to downgrade to static remote key ?
I don't love spending a round trip on a channel type that we know is going to fail. Looking at re-using the code in get_initial_channel_type
for downgrades to DRY this up a bit. Downside is that we need to thread remote features through, but I think it's a nicer approach - WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I don't like that roundtrip either :)
/// [`ChannelManager::accept_inbound_channel`]: crate::ln::channelmanager::ChannelManager::accept_inbound_channel | ||
/// [`DecodeError::InvalidValue`]: crate::ln::msgs::DecodeError::InvalidValue | ||
#[cfg(test)] | ||
pub negotiate_anchor_zero_fee_commitments: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume overall that someone could, strangely, set negotiate_anchor_zero_fee_commitments = true
and negotiate_anchors_zero_fee_htlc_tx = false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah indeed. Other options would be:
- Have one
negotiate_anchors
config option that tries zero fee and then regular anchors - Have this option imply
negotiate_anchors_zero_fee_htlc_tx
even if it isn't set
I don't have strong preference for either, IMO (1) makes testing a bit of a nightmare and (2) is more confusing to an end user. Assuming that we'll eventually turn these both on by default, I think I still like this approach most?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I like this approach too ! I noted this because it opens up this strange case where someone sets negotiate_anchor_zero_fee_commitments = true
and negotiate_anchors_zero_fee_htlc_tx = false
, and we would have to potentially handle a downgrade from zero fee commitments directly to static remote key, skipping zero fee htlcs. I wasn't sure how much support / testing we wanted to provide on such a downgrade.
let mut config = UserConfig::default(); | ||
config.channel_handshake_config.negotiate_anchor_zero_fee_commitments = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, should we also test the case where negotiate_anchor_zero_fee_commitments
and negotiate_anchors_zero_fee_htlc_tx
are both true ?
lightning/src/ln/channelmanager.rs
Outdated
@@ -16123,6 +16123,13 @@ mod tests { | |||
assert_eq!(accept_message.channel_reserve_satoshis, 2_000); | |||
} | |||
|
|||
#[test] | |||
fn test_inbound_zero_fee_commitments_acceptance() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we have test_inbound_anchors_manual_acceptance
above, so should this be test_inbound_zero_fee_commitments_manual_acceptance
? For consistency ?
fn test_inbound_zero_fee_commitments_acceptance() { | ||
let mut zero_fee_cfg = test_default_channel_config(); | ||
zero_fee_cfg.channel_handshake_config.negotiate_anchor_zero_fee_commitments = true; | ||
do_test_manual_inbound_accept_with_override(zero_fee_cfg, None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also test zero_fee_cfg
with overrides ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't really offer us much more coverage since config override is independent of the channel type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes I see it now thank you.
IIUC then this test right here double checks that if mannual_accept_cfg.manually_accept_inbound_channels = false
, and channel_handshake_config.negotiate_anchor_zero_fee_commitments = true
, that the receiver node complains with "No channels with anchor outputs accepted.". Also checks that when both of these settings are true, we do accept the channel.
Rather than duplicating our channel type preference ordering in downgrade logic, make a modified version of the remote peer's supported features and remove our current channel type from it to get the next preferred channel type.
To allow testing along the way in this PR, turn on negotiation of zero fee channels. Co-authored-by: Matt Corallo <[email protected]>
Sender: MUST set `feerate_per_kw` to zero Receiver: MUST fail the channel if `feerate_per_kw` != 0 Co-authored-by: Matt Corallo <[email protected]>
Like anchor channels, these channels require that the user reserves a UTXO to bump the channel. If we automatically accept this channel type and the user does not have such reserve available, they are at risk of losing funds because they cannot fee bump the channel.
b9ec89c
to
892d87a
Compare
Major change in push is using get_initial_channel_type in channel type downgrades to DRY up the code a bit. |
This PR updates the channel establishment flow to allow and validate V3 channels (behind
test
flag).