Skip to content
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

Clean up: remove two unused features, assume four more #1092

Merged
merged 7 commits into from
May 20, 2024

Conversation

rustyrussell
Copy link
Collaborator

@rustyrussell rustyrussell commented Jun 30, 2023

Removed:

  • initial_routing_sync (only had an effect if gossip_queries not supported)
  • option_anchor_outputs (only supported by older experimental-only CLN builds)

I looked at all node_announcements on my node. There are 449 nodes apparently running a 4-year-old LND version (features hex 2200), which have 3+ year old channels. @Roasbeef points out that they already will have their channel_updates ignored due to lack of htlc_maximum_msat which is now required by LND and CLN, at least).

Features you can now assume (you should probably still set it for now, but you can stop checking it).

  • var_onion_optin (all but 6 nodes)
  • option_data_loss_protect (all but 11 nodes)
  • option_static_remotekey (all but 16 nodes)

gossip_queries is now slightly repurposed: if you don't offer this, it means you won't give useful gossip (so you should not query such nodes, nor consider them when trying to figure out if you've got all the gossip).

Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 🧹 🚀

| 6/7 | `gossip_queries` | ASSUMED | | | |
| 8/9 | `var_onion_optin` | ASSUMED | | | |
| 10/11 | `gossip_queries_ex` | Gossip queries can include additional information | IN | | [BOLT #7][bolt07-query] |
| 12/13 | `option_static_remotekey` | ASSUMED | | | |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should make this one ASSUMED, as it removes the option to cleanly deprecate it. If I'm a node that only wants to support anchor_outputs (which we'll want soon), I want to be able to explicitly disable the option_static_remotekey feature bit from my node_announcement and init, to let other nodes know that they shouldn't connect to me if they want option_static_remotekey channels.

It is a good idea however to remove the dependency between anchor_outputs and option_static_remotekey (to allow setting only the anchor_outputs feature bit without the option_static_remotekey feature bit), and it would be useful to update all implementations to accept that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR essentially reduces the basic channel types to anchors and no anchors. So if we want to require anchor channels, then for now we can set bit 22 (require option_anchors_zero_fee_htlc_tx).

But if there's a new channel type in the future that doesn't depend on anchors, and we want to support both anchors and the new type, we won't have a way to signal that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if there's a new channel type in the future that doesn't depend on anchors, and we want to support both anchors and the new type, we won't have a way to signal that.

Exactly, that's my concern (and why I've previously said that making option_static_remotekey an explicit feature dependency of anchor_outputs was a mistake, which I think we should correct now).

I'm thinking in particular of the next channel type (that I hope can be implemented in ~1/2 years) which would be 0-fee commit txs with ephemeral anchor (once the policy change for bitcoin is deployed). When that ships, I believe updated nodes will want to first support anchor_outputs and ephemeral_anchor_outputs, but deprecate support for plain option_static_remotekey. This will be possible if we break that dependency now (this way we can be confident most nodes will be updated in ~1 year).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that I hope can be implemented in ~1/2 years

Someone's feeling very optimistic 😉

My lofty estimate is something more like 4x that, so ~2 years given the rollout that still needs to happen on the Bitcoin p2p network.

I think everyone just setting it to be required achieves the same goal in practice? Those that don't support it will fail the feature bit connection check. IIRC in the past when we did this, we had some issues with peers that had old channels, but hadn't yet updated their feature bits: new and old were unable to reconnect as they bit check failed even though there was an existing channel open.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think everyone just setting it to be required achieves the same goal in practice?

I'm not sure I follow, my goal here is to be able to easily deprecate option_static_remotekey. Setting it to required definitely does not allow that?

Maybe what you mean here is that making option_static_remotekey assumed instead of explicitly set lets us deprecate it cleanly? Which somewhat means we should break the feature link between anchor_outputs and option_static_remotekey?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR essentially reduces the basic channel types to anchors and no anchors. So if we want to require anchor channels, then for now we can set bit 22 (require option_anchors_zero_fee_htlc_tx).

But if there's a new channel type in the future that doesn't depend on anchors, and we want to support both anchors and the new type, we won't have a way to signal that.

You would offer both the new bit and the anchor bit if you allow both, surely?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think everyone just setting it to be required achieves the same goal in practice?

I'm not sure I follow, my goal here is to be able to easily deprecate option_static_remotekey. Setting it to required definitely does not allow that?

It actually does. You set it required, and ignore the bit in the peer. We should fix the definition of negotiated, however, to make this clear. If you set it required, and they keep talking to you, you assume it's negotiated (this is how CLN works, for example).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit convoluted though...deprecating option_static_remotekey by not setting the corresponding feature bit anymore would be way more explicit, wouldn't it? But maybe that ship has sailed since we initially created that dependency, and we just need to make sure we don't recreate the same kind of trap for future channel types.

07-routing-gossip.md Outdated Show resolved Hide resolved
timely inclusion in a block.
- MAY combine it with other transactions.
2. if `option_anchors_zero_fee_htlc_tx` applies:
1. if `option_anchors` applies:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
1. if `option_anchors` applies:
- if `option_anchors` applies:

@@ -631,8 +631,6 @@ also subtract two times the fixed anchor size of 330 sats from the funder
Each commitment transaction uses a unique `localpubkey`, and a `remotepubkey`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't true anymore if we assume option_static_remotekey?

Comment on lines +456 to +992
We decide on
`option_anchors` at this point when we first have to generate
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
We decide on
`option_anchors` at this point when we first have to generate
We decide on `option_static_remotekey` or `option_anchors`
at this point when we first have to generate

Copy link
Contributor

@morehouse morehouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

| 6/7 | `gossip_queries` | ASSUMED | | | |
| 8/9 | `var_onion_optin` | ASSUMED | | | |
| 10/11 | `gossip_queries_ex` | Gossip queries can include additional information | IN | | [BOLT #7][bolt07-query] |
| 12/13 | `option_static_remotekey` | ASSUMED | | | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR essentially reduces the basic channel types to anchors and no anchors. So if we want to require anchor channels, then for now we can set bit 22 (require option_anchors_zero_fee_htlc_tx).

But if there's a new channel type in the future that doesn't depend on anchors, and we want to support both anchors and the new type, we won't have a way to signal that.

@@ -208,7 +208,6 @@ arbitrary combination (they represent the persistent features which
affect the channel operation).

The currently defined basic types are:
- no features (no bits set)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit/commit message: advertised by all by (/but) 11 nodes

09-features.md Outdated
@@ -53,7 +52,7 @@ The Context column decodes as follows:

## Definitions

We define `option_anchors` as `option_anchor_outputs || option_anchors_zero_fee_htlc_tx`.
We define `option_anchors` as `option_anchors_zero_fee_htlc_tx`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need option_anchors here - couldn't we just use option_anchors_zero_fee_htlc_tx everywhere now? Ah, removed in next commit.

09-features.md Show resolved Hide resolved
- otherwise:
- the `channel_type` is empty
- the `channel_type` is `option_static_remotekey` (bit 12)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually a functional change? IIUC, in the case there were no channel_type bits set, it would fall back to the existing "implicit" negotiation that selected based on feature bits (which maybe is actually under-defined?).

@@ -1207,377 +1203,6 @@ And, here are the keys needed to create the transactions:

And, here are the test vectors themselves:

name: simple commitment tx with no HTLCs
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🫡

09-features.md Show resolved Hide resolved
| 6/7 | `gossip_queries` | ASSUMED | | | |
| 8/9 | `var_onion_optin` | ASSUMED | | | |
| 10/11 | `gossip_queries_ex` | Gossip queries can include additional information | IN | | [BOLT #7][bolt07-query] |
| 12/13 | `option_static_remotekey` | ASSUMED | | | |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that I hope can be implemented in ~1/2 years

Someone's feeling very optimistic 😉

My lofty estimate is something more like 4x that, so ~2 years given the rollout that still needs to happen on the Bitcoin p2p network.

I think everyone just setting it to be required achieves the same goal in practice? Those that don't support it will fail the feature bit connection check. IIRC in the past when we did this, we had some issues with peers that had old channels, but hadn't yet updated their feature bits: new and old were unable to reconnect as they bit check failed even though there was an existing channel open.

@@ -42,8 +42,7 @@ The Context column decodes as follows:
| 14/15 | `payment_secret` | Node supports `payment_secret` field | IN9 | `var_onion_optin` | [Routing Onion Specification][bolt04] |
| 16/17 | `basic_mpp` | Node can receive basic multi-part payments | IN9 | `payment_secret` | [BOLT #4][bolt04-mpp] |
| 18/19 | `option_support_large_channel` | Can create large channels | IN | | [BOLT #2](02-peer-protocol.md#the-open_channel-message) |
| 20/21 | `option_anchor_outputs` | Anchor outputs | IN | `option_static_remotekey` | [BOLT #3](03-transactions.md) |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the expected behavior if we peer sends bits 20/21 for a channel type open? Do we assume that it's actually option_anchors_zero_fee_htlc_tx, or fail as we've all removed the feature bits from our codebases so that shows up as an undefined combination?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should fail in that case, because that is likely an old node that supports the non-zero-fee variant so this won't work. A node that wants option_anchors_zero_fee_htlc_tx will just set the correct set of bits for it, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you still support it, you'd accept it? But pretty soon, nobody will...

1. MUST be 0.
- Otherwise, MUST be calculated to match:
1. Multiply `feerate_per_kw` by 663 and divide by 1000 (rounding down).

The fee for an HTLC-success transaction:
- If `option_anchors_zero_fee_htlc_tx` applies:
- If `option_anchors` applies:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like left over from the last commit? Doesn't match the commit title of:

BOLT 9: assume var_onion_optin.

@@ -36,14 +36,14 @@ The Context column decodes as follows:
| 3 | `initial_routing_sync` | Sending node needs a complete routing information dump | I | | [BOLT #7][bolt07-sync] |
| 4/5 | `option_upfront_shutdown_script` | Commits to a shutdown scriptpubkey when opening channel | IN | | [BOLT #2][bolt02-open] |
| 6/7 | `gossip_queries` | More sophisticated gossip control | IN | | [BOLT #7][bolt07-query] |
| 8/9 | `var_onion_optin` | Requires/supports variable-length routing onion payloads | IN9 | | [Routing Onion Specification][bolt04] |
| 8/9 | `var_onion_optin` | ASSUMED | | | |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment here re also advising this is set to required.

If a peer receives a legacy onion, should they fail back or proceed? Or are we assuming ppl have ripped out all the logic, so they'd just fail to even parse it in the first place (malformed onion)?

to send this, otherwise `gossip_queries` negotiation means no gossip
messages would be received.
a specific range. A node which wants any gossip messages has
to send this, otherwise no gossip messages would be received.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also codify the behavior we talked about re "all or nothing" re the timestamp range? So that implementations only ever use this to signal that they want to receive updates, or don't want to receive any updates at all (so no legacy announcement back fill).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me, but I think it would fit better in a separate PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to this PR, I agree it should be a separate one.

t-bast added a commit to ACINQ/eclair that referenced this pull request Aug 29, 2023
lightning/bolts#1092 makes some features compulsory
and lets us assume that other nodes have support for them.
t-bast added a commit to ACINQ/eclair that referenced this pull request Sep 26, 2023
lightning/bolts#1092 makes some features compulsory
and lets us assume that other nodes have support for them.
t-bast added a commit to ACINQ/eclair that referenced this pull request Sep 27, 2023
lightning/bolts#1092 makes some features compulsory
and lets us assume that other nodes have support for them.
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Nov 12, 2023
As suggested in lightning/bolts#1092.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: `option_data_loss_protect` is now required (advertized by all but 11 nodes)
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Nov 12, 2023
As suggested in lightning/bolts#1092.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: `option_gossip_queries` is now required (advertized by all but 11 nodes)
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Nov 12, 2023
As suggested in lightning/bolts#1092.

We still support channels opened without it, but you can no longer open new ones without it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: `option_gossip_queries` is now required (advertized by all but 16 nodes)
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Nov 12, 2023
As suggested in lightning/bolts#1092.

We still support channels opened without it, but you can no longer open new ones without it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: `option_gossip_queries` is now required (advertized by all but 16 nodes)
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Nov 13, 2023
As suggested in lightning/bolts#1092.

We still support channels opened without it, but you can no longer open new ones without it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: `option_gossip_queries` is now required (advertized by all but 16 nodes)
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Nov 13, 2023
As suggested in lightning/bolts#1092.

We still support channels opened without it, but you can no longer open new ones without it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: `option_gossip_queries` is now required (advertized by all but 16 nodes)
cdecker pushed a commit to rustyrussell/lightning that referenced this pull request Nov 13, 2023
As suggested in lightning/bolts#1092.

We still support channels opened without it, but you can no longer open new ones without it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: `option_gossip_queries` is now required (advertized by all but 16 nodes)
dpad85 added a commit to ACINQ/eclair that referenced this pull request Nov 22, 2023
Static remote key is mandatory for the ACINQ peer.i
See lightning/bolts#1092
dpad85 added a commit to ACINQ/phoenix that referenced this pull request Dec 1, 2023
This fixes an issue where legacy phoenix could not connect
following lightning/bolts#1092
gudnuf pushed a commit to gudnuf/lightning that referenced this pull request Mar 1, 2024
As suggested in lightning/bolts#1092.

We still support channels opened without it, but you can no longer open new ones without it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: `option_gossip_queries` is now required (advertized by all but 16 nodes)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…now assumed.

These still have names and numbers, since they appear in `channel_type`.  They are somewhat tangled with each other, so let's tie them together as assumed.

option_data_loss_protect is advertized by all by 11 nodes(*), and option_static_remotekey all but 16 nodes.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
[* there are 449 three-year old LND nodes which advertize `2200` as their features, which have already been trimmed from most gossip for not having htlc_maximum_msat in their channel_updates]
…_fee_htlc_tx.

It's supported only by pre-23.05 core-lightning nodes built with
EXPERIMENTAL_FEATURES.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
09-features.md Outdated Show resolved Hide resolved
t-bast added a commit to ACINQ/eclair that referenced this pull request Mar 27, 2024
lightning/bolts#1092 initially made the
`gossip_queries` feature mandatory and assumed: however it generally
doesn't make sense for mobile wallets. It was thus relaxed in the last
commits of this BOLTs PR, so we revert our change to `mandatory` to
make this optional and avoid sending gossip queries to nodes who don't
activate the feature.
Advertized as supported by all but 6 nodes (and those can no longer
route payments since people only send the modern onion these days)

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Supported by all but 11 nodes*.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
[* there are 449 three-year old LND nodes which advertize `2200` as their features, which have already been trimmed from most gossip for not having htlc_maximum_msat in their channel_updates]
This only had an effect when `gossip_queries` was not negotiated, which is now assumed.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
t-bast added a commit to ACINQ/eclair that referenced this pull request Apr 16, 2024
lightning/bolts#1092 initially made the
`gossip_queries` feature mandatory and assumed: however it generally
doesn't make sense for mobile wallets. It was thus relaxed in the last
commits of this BOLTs PR, so we revert our change to `mandatory` to
make this optional and avoid sending gossip queries to nodes who don't
activate the feature.
Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should merge that PR, shouldn't we? I have fixed my initial comments and added a bit more clean-up in t-bast@fdaf7d6, you should be able to just grab this commit and I believe we should be ready to go @rustyrussell

Copy link
Collaborator

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🪓

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b5ce609

@niftynei niftynei merged commit fc687e8 into lightning:master May 20, 2024
@t-bast
Copy link
Collaborator

t-bast commented May 21, 2024

It looks like this was merged without including the changes from #1092 (review) (which I believe are necessary otherwise this is incomplete). I'll open a PR for this commit.

t-bast added a commit to t-bast/bolts that referenced this pull request May 21, 2024
This is a follow-up to lightning#1092
that fixes the following issues:

- fix a few typos
- remove non-zero-fee anchors test cases
- remove `remote_pubkey` rotation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants