Clean up: remove two unused features, assume four more by rustyrussell · Pull Request #1092 · lightning/bolts

rustyrussell · 2023-06-30T19:15:13Z

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

t-bast

🧹 🧹 🚀

t-bast · 2023-07-04T08:05:21Z

09-features.md

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


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.

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.

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

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.

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?

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?

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

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

t-bast · 2023-07-04T08:09:24Z

05-onchain.md

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


nit:

Suggested change

1. if `option_anchors` applies:

- if `option_anchors` applies:

t-bast · 2023-07-04T08:11:30Z

03-transactions.md

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


This isn't true anymore if we assume option_static_remotekey?

t-bast · 2023-07-04T08:16:16Z

02-peer-protocol.md

+We decide on
+`option_anchors` at this point when we first have to generate


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

morehouse

Concept ACK

morehouse · 2023-07-06T16:43:53Z

09-features.md

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


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.

carlaKC · 2023-07-17T18:39:05Z

02-peer-protocol.md

 affect the channel operation).

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


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

carlaKC · 2023-07-17T18:50:40Z

09-features.md

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


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

Roasbeef · 2023-07-31T18:31:52Z

02-peer-protocol.md

-      - the `channel_type` is `option_static_remotekey` (bit 12)
    - otherwise:
-      - the `channel_type` is empty
+      - the `channel_type` is `option_static_remotekey` (bit 12)


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

Roasbeef · 2023-07-31T18:34:30Z

03-transactions.md


 And, here are the test vectors themselves:

-    name: simple commitment tx with no HTLCs


09-features.md

Roasbeef · 2023-07-31T18:42:19Z

09-features.md

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


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.

Roasbeef · 2023-07-31T18:43:41Z

09-features.md

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


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?

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?

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

Roasbeef · 2023-07-31T18:44:34Z

03-transactions.md


 The fee for an HTLC-success transaction:
-  - If `option_anchors_zero_fee_htlc_tx` applies:
+  - If `option_anchors` applies:


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

BOLT 9: assume var_onion_optin.

Roasbeef · 2023-07-31T18:45:55Z

09-features.md

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


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

Roasbeef · 2023-07-31T18:47:07Z

07-routing-gossip.md

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


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

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

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

lightning/bolts#1092 makes some features compulsory and lets us assume that other nodes have support for them.

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)

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)

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)

Static remote key is mandatory for the ACINQ peer.i See lightning/bolts#1092

This fixes an issue where legacy phoenix could not connect following lightning/bolts#1092

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

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>

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.

t-bast

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

Roasbeef

LGTM 🪓

vincenzopalazzo

ACK b5ce609

t-bast · 2024-05-21T08:54:52Z

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.

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

t-bast reviewed Jul 4, 2023

View reviewed changes

morehouse reviewed Jul 6, 2023

View reviewed changes

t-bast mentioned this pull request Jul 12, 2023

Lightning Specification Meeting 2023/07/17 #1094

Closed

30 tasks

carlaKC reviewed Jul 17, 2023

View reviewed changes

benthecarman mentioned this pull request Jul 19, 2023

July Topics austinbitdevs/austinbitdevs.com#22

Closed

t-bast mentioned this pull request Jul 27, 2023

Lightning Specification Meeting 2023/07/31 #1098

Closed

27 tasks

Roasbeef reviewed Jul 31, 2023

View reviewed changes

This was referenced Jul 31, 2023

Make more features compulsory! ElementsProject/lightning#6471

Closed

option_simple_close (features 60/61) #1096

Closed

t-bast mentioned this pull request Aug 10, 2023

Lightning Specification Meeting 2023/08/14 #1101

Closed

25 tasks

t-bast mentioned this pull request Aug 23, 2023

Lightning Specification Meeting 2023/08/28 #1103

Closed

23 tasks

t-bast added a commit to ACINQ/eclair that referenced this pull request Aug 29, 2023

Assume widely supported features

fc2a411

lightning/bolts#1092 makes some features compulsory and lets us assume that other nodes have support for them.

t-bast mentioned this pull request Aug 29, 2023

Assume widely supported features ACINQ/eclair#2732

Merged

t-bast mentioned this pull request Sep 8, 2023

Lightning Specification Meeting 2023/09/11 #1107

Closed

24 tasks

t-bast mentioned this pull request Sep 21, 2023

Lightning Specification Meeting 2023/09/25 #1114

Closed

24 tasks

t-bast added a commit to ACINQ/eclair that referenced this pull request Sep 26, 2023

Assume widely supported features

bfc995c

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

Assume widely supported features (#2732)

37f3fbe

lightning/bolts#1092 makes some features compulsory and lets us assume that other nodes have support for them.

Roasbeef mentioned this pull request Sep 29, 2023

feature: begin flipping well deployed feature bits to the required bit lightningnetwork/lnd#8048

Closed

2 tasks

t-bast mentioned this pull request Oct 5, 2023

Lightning Specification Meeting 2023/10/23 #1115

Closed

22 tasks

t-bast mentioned this pull request Oct 30, 2023

Lightning Specification Meeting 2023/11/06 #1116

Closed

20 tasks

t-bast mentioned this pull request Nov 16, 2023

Lightning Specification Meeting 2023/11/20 #1118

Closed

20 tasks

dpad85 added a commit to ACINQ/eclair that referenced this pull request Nov 22, 2023

Support mandatory StaticRemoteKey feature

8b390a9

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

Use eclair-core 0.4.23-android-phoenix

da40a47

This fixes an issue where legacy phoenix could not connect following lightning/bolts#1092

t-bast mentioned this pull request Mar 7, 2024

Lightning Specification Meeting 2024/03/11 #1146

Closed

21 tasks

rustyrussell added 4 commits March 14, 2024 21:19

BOLT 9: Add ASSUMED marker we can use on features.

0730135

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

BOLT 9: Remove option_anchor_outputs, in favor of option_anchors_zero…

a3f4d37

…_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>

BOLT 9: rename option_anchors_zero_fee_htlc_tx to option_anchors.

6881f6d

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

t-bast mentioned this pull request Mar 20, 2024

Lightning Specification Meeting 2024/03/25 #1150

Closed

21 tasks

tnull reviewed Mar 21, 2024

View reviewed changes

09-features.md Outdated Show resolved Hide resolved

tnull mentioned this pull request Mar 21, 2024

Signal GossipQuery support when using IgnoringMessagHandler lightningdevkit/rust-lightning#2959

Merged

rustyrussell mentioned this pull request Mar 25, 2024

Reallow no gossip queries ElementsProject/lightning#7174

Merged

t-bast mentioned this pull request Mar 27, 2024

Relax assumptions about gossip_queries ACINQ/eclair#2842

Merged

Roasbeef mentioned this pull request Apr 8, 2024

Lightning Specification Meeting 2024/04/08 #1152

Closed

21 tasks

rustyrussell added 3 commits April 9, 2024 05:48

BOLT 9: assume var_onion_optin.

f375334

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>

BOLT 9: Remove initial_routing_sync.

b5ce609

This only had an effect when `gossip_queries` was not negotiated, which is now assumed. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/remove-old-features branch from e0ee59f to b5ce609 Compare April 8, 2024 20:20

t-bast reviewed Apr 17, 2024

View reviewed changes

t-bast mentioned this pull request Apr 18, 2024

Lightning Specification Meeting 2024/04/22 #1155

Closed

22 tasks

Roasbeef approved these changes Apr 24, 2024

View reviewed changes

t-bast mentioned this pull request May 2, 2024

Lightning Specification Meeting 2024/05/06 #1161

Closed

25 tasks

vincenzopalazzo approved these changes May 6, 2024

View reviewed changes

t-bast mentioned this pull request May 14, 2024

Lightning Specification Meeting 2024/05/20 #1164

Closed

23 tasks

niftynei merged commit fc687e8 into lightning:master May 20, 2024

t-bast mentioned this pull request May 21, 2024

Clean-up: follow-up on removing spec features #1165

Merged

TheBlueMatt mentioned this pull request May 21, 2024

Assume variable length onion, data loss protect, and static remotekey lightningdevkit/rust-lightning#3074

Open

tnull mentioned this pull request Jun 7, 2024

Add anchor support lightningdevkit/ldk-node#141

Merged

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

		We decide on
		`option_anchors` at this point when we first have to generate


		And, here are the test vectors themselves:

		name: simple commitment tx with no HTLCs

Conversation

rustyrussell commented Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morehouse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

t-bast commented May 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

rustyrussell commented Jun 30, 2023 •

edited

Loading