BOLT 4: Remove legacy format, make var_onion_optin compulsory. #962

rustyrussell · 2022-02-22T02:52:51Z

My measurements a few weeks ago reveal that only 5 nodes do not advertize this feature, of over 17000. I have a patch to remove support from c-lightning, too.

t-bast

The future is here! Concept ACK, glad to remove this!

09-features.md

04-onion-routing.md

t-bast

LGTM, just a typo to fix and should be ready to go.

04-onion-routing.md

t-bast

ACK 7a6291d, I'll prepare a branch to remove the legacy code in eclair as well.

t-bast · 2022-02-25T10:32:25Z

Actually while updating eclair, I realized that we should also update the test vectors:

remove https://github.com/lightning/bolts/blob/master/bolt04/onion-test-v0.json
udpate https://github.com/lightning/bolts/blob/master/bolt04/onion-test-multi-frame.json to only contain variable-size payloads

That does mean a bit more work to do (sorry!).

We previously supported a 65-bytes fixed-size sphinx payload, which has been deprecated in favor of variable length payloads containing a tlv stream (see lightning/bolts#619). It looks like the whole network now supports the variable-length format, so we can stop accepting the old one. It is also being removed from the spec (see lightning/bolts#962).

rustyrussell · 2022-02-28T02:57:33Z

Hmm, I'm out of time to update the multiframe test, though you're right :(

rustyrussell · 2022-02-28T02:57:58Z

Ping @cdecker ?

As per proposal in lightning/bolts#962 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Removed: protocol: support for legacy onion format removed, since everyone supports the new one.

Roasbeef · 2022-02-28T19:11:04Z

Concept ACK, we'll likely start with just making our existing feature bit required, as we have some existing tests that still assume this is relevant.

As per proposal in lightning/bolts#962 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Removed: protocol: support for legacy onion format removed, since everyone supports the new one.

In this commit, we start to advertise the required feature bit for the new modern TLV onion format. In a future change, we'll remove the logic for being able to encode+encode the payload. For now, we still have tests that exercise being able to use the new and old formats (as well as both of them in a single route), so we'll continue to retain that behavior for now. Implements lightning/bolts#962.

As per proposal in lightning/bolts#962 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Removed: protocol: support for legacy onion format removed, since everyone supports the new one.

t-bast · 2022-03-25T14:06:39Z

I'm looking at our node's statistics, and a non-negligible portion (~30%) of the payments we relay are still using the legacy format.
I'm afraid most nodes advertise the feature, but without actually using it by default when sending payments.
Can you check relay statistics on your nodes to see if it matches what I'm seeing?
Are all implementations always generating onion with the new format (and since which version)?

Roasbeef · 2022-03-25T22:52:26Z

Are all implementations always generating onion with the new format (and since which version)?

lnd has been generating with the new format as of version v0.8.0 (released October 2019). In our path finding code, we'll use the old/new depending on which feature bits the node advertises (so we support mixing old/new along a route).

t-bast · 2022-03-28T06:51:41Z

In our path finding code, we'll use the old/new depending on which feature bits the node advertises (so we support mixing old/new along a route).

In that case that's probably not the culprit since more than 99% of nodes (including ours) advertise var_onion_optin.
I'm suspecting a popular wallet (or a few of them) is still using the legacy encoding for their payments...

vincenzopalazzo · 2022-03-28T09:39:20Z

on c-lightning IRC we have a crash report due to the old onion format too!

my node seems to be crashing due to invalid onion messages
Unexpected message WIRE_OBS2_ONION_MESSAGE:
using master at commit 7abc491f4c214f2bcf9e8f2b9066ca05dee0342b

t-bast · 2022-03-28T09:42:57Z

on c-lightning IRC we have a crash report due to the old onion format too!

I find c-lightning should definitely not be crashing, even with the legacy encoding disabled it should simply fail the adds with a required_node_feature_missing or something similar, shouldn't it?

Roasbeef · 2022-03-28T19:21:21Z

Some ppl on Twitter hypothesized that maybe a lot of the traffic is actually probing. I know in the past, some systems/applications would use an API for lnd that accept a raw routes for probing. I dug into things on our end, and realized that unless a user is setting some custom TLV record in the route (also not using mpp/amp), we'll default to using the legacy payload (there's a bool that defaults to false). So this might be contributing to that 30% number mentioned above...

We have a new major release coming up, so this could be an opportunity to change our API to default to the modern payload (can't thing of any downside really).

Roasbeef

LGTM ❄️

rustyrussell · 2022-09-26T21:30:11Z

Payments using the legacy encoding have fallen below 0,2% of the total payments relayed on our node in the last week tada Let me know if other nodes see similar data, this is a good indication that we may be able to finally remove this soon!

Confirmed: my node has seen no legacy since 17 June (over 12000 htlcs fwd since then), and the one before that was 2 July.

He's dead, Jim!

rustyrussell · 2022-09-26T21:37:18Z

Squashed into a single commit, and included the test vector fixups. Will test them today...

rustyrussell · 2022-09-27T01:39:04Z

Ack test vectors!

t-bast

A couple of nits, otherwise LGTM, let's kill this thing!

04-onion-routing.md

t-bast · 2022-09-27T06:40:01Z

bolt04/onion-test.json

@@ -0,0 +1,37 @@
+{
+  "comment": "A testcase for a variable length hop_payload. The third payload is 256 bytes long.",


Looks like I forgot to update this comment, the third payload isn't 256 bytes long!

Yep, fixed to "275 bytes long".

Apologies for leaving this comment only after the PR has been merged, but I believe the "comment" is still incorrect as it's the fifth payload that is 275 bytes long, no?
There's also the second payload that's 83 bytes long, so there's now two payloads that have a different size than the rest of the payloads, so perhaps it'd be worth mentioning both if we want to explicitly mention the payloads that have a different size. If you'd like me to, I can of course open a PR editing the "comment" :).

My measurements a few weeks ago reveal that only 5 nodes do not advertize this feature, of over 17000. I have a patch to remove support from c-lightning, too. [ 6 months later: t-bast notes that they only see 0.2% of htlcs using legacy, and my node hasn't seen one for 2 months w/ 12000 htlcs --RR ] Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

To only use valid tlv payloads instead of fixed-size legacy ones and invalid tlv streams. [ Minor typo change: third payload is 275 not 256 bytes long --RR ]

This has been removed from the spec recently, see: lightning/bolts#962

* Add support for arbitrary length onion errors The specification recommends using a length of 256 for onion errors, but it doesn't say that we should reject errors that use a different length. We may want to start creating errors with a bigger length than 256 if we need to transmit more data to the sender. In order to prepare for this, we keep creating 256-bytes onion errors, but allow receiving errors of arbitrary length. * Remove support for legacy fixed-size onion payload This has been removed from the spec recently, see: lightning/bolts#962

see lightning/bolts#962

In this commit, we start to ignore the option to allow the caller to use the legacy onion payload. The new payload is much more flexible and efficient, so there's really no reason to still use it, other than for backwards compatibility tests. Our existing tests that exercise the legacy feature uses a build tag, which forces nodes to not advertise the new payload format, which then forces path finding to include the legacy payload, so we can be confident that route is still being tested. The existence of this option (which actually makes the TLV payload opt-in for `SendToRoute` users) makes it harder to remove it from the protocol all together. With this PR, we take a step forward to allowing such a change which is being tracked on the spec level at: lightning/bolts#962. In a future release, we'll move to remove the field all together. Ignoring the field today doesn't seem to have any clear downsides, as most payments always include the MPP payload (due to payment secrets), so this shouldn't impact users in a significant way.

t-bast reviewed Feb 22, 2022

View reviewed changes

09-features.md Outdated Show resolved Hide resolved

04-onion-routing.md Show resolved Hide resolved

t-bast reviewed Feb 23, 2022

View reviewed changes

04-onion-routing.md Outdated Show resolved Hide resolved

t-bast approved these changes Feb 25, 2022

View reviewed changes

This was referenced Feb 25, 2022

Remove support for legacy Sphinx payloads ACINQ/eclair#2190

Merged

Lightning Specification Meeting 2022/02/28 #965

Closed

TheBlueMatt approved these changes Feb 25, 2022

View reviewed changes

rustyrussell mentioned this pull request Feb 28, 2022

Require modern onion ElementsProject/lightning#5058

Merged

Roasbeef mentioned this pull request Mar 14, 2022

Lightning Specification Meeting 2022/03/14 #969

Closed

23 tasks

Roasbeef mentioned this pull request Mar 15, 2022

feature: advertise required feature bit for TLV onions lightningnetwork/lnd#6336

Open

t-bast mentioned this pull request Mar 24, 2022

Lightning Specification Meeting 2022/03/28 #971

Closed

22 tasks

t-bast mentioned this pull request Mar 30, 2022

Route Blinding (Feature 24/25) #765

Merged

t-bast mentioned this pull request Sep 22, 2022

Lightning Specification Meeting 2022/09/26 #1028

Closed

27 tasks

Roasbeef approved these changes Sep 26, 2022

View reviewed changes

rustyrussell force-pushed the remove-legacy-onion branch from 7a6291d to 63b8b25 Compare September 26, 2022 21:36

t-bast reviewed Sep 27, 2022

View reviewed changes

rustyrussell mentioned this pull request Sep 28, 2022

Remove legacy onion support, simplify internal APIs ElementsProject/lightning#5639

Merged

rustyrussell and others added 2 commits September 29, 2022 12:34

Update onion test vectors

e5cfdf2

To only use valid tlv payloads instead of fixed-size legacy ones and invalid tlv streams. [ Minor typo change: third payload is 275 not 256 bytes long --RR ]

rustyrussell force-pushed the remove-legacy-onion branch from 63b8b25 to e5cfdf2 Compare September 29, 2022 03:07

rustyrussell merged commit 7053463 into lightning:master Sep 29, 2022

t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Sep 29, 2022

Remove support for legacy fixed-size onion payload

a3d5097

This has been removed from the spec recently, see: lightning/bolts#962

t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Sep 30, 2022

Remove support for legacy fixed-size onion payload

a866417

This has been removed from the spec recently, see: lightning/bolts#962

t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Sep 30, 2022

Remove support for legacy fixed-size onion payload

5650ac9

This has been removed from the spec recently, see: lightning/bolts#962

t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Sep 30, 2022

Remove support for legacy fixed-size onion payload

5059aeb

This has been removed from the spec recently, see: lightning/bolts#962

t-bast mentioned this pull request Dec 21, 2022

Reflect legacy payload type handling implementation in BOLT #703

Closed

SomberNight added a commit to SomberNight/electrum that referenced this pull request Jun 29, 2023

lnonion: rm support for legacy (pre-TLV) onions

2e30969

see lightning/bolts#962

SomberNight added a commit to SomberNight/electrum that referenced this pull request Jun 29, 2023

lnonion: rm support for legacy (pre-TLV) onions

6b43eac

see lightning/bolts#962

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BOLT 4: Remove legacy format, make var_onion_optin compulsory. #962

BOLT 4: Remove legacy format, make var_onion_optin compulsory. #962

rustyrussell commented Feb 22, 2022

t-bast left a comment

t-bast left a comment

t-bast left a comment

t-bast commented Feb 25, 2022

rustyrussell commented Feb 28, 2022

rustyrussell commented Feb 28, 2022

Roasbeef commented Feb 28, 2022

t-bast commented Mar 25, 2022

Roasbeef commented Mar 25, 2022

t-bast commented Mar 28, 2022

vincenzopalazzo commented Mar 28, 2022

t-bast commented Mar 28, 2022

Roasbeef commented Mar 28, 2022 •

edited

Loading

Roasbeef left a comment

rustyrussell commented Sep 26, 2022

rustyrussell commented Sep 26, 2022

rustyrussell commented Sep 27, 2022

t-bast left a comment

t-bast Sep 27, 2022

rustyrussell Sep 29, 2022

ViktorTigerstrom Sep 30, 2022

		@@ -0,0 +1,37 @@
		{
		"comment": "A testcase for a variable length hop_payload. The third payload is 256 bytes long.",

BOLT 4: Remove legacy format, make var_onion_optin compulsory. #962

BOLT 4: Remove legacy format, make var_onion_optin compulsory. #962

Conversation

rustyrussell commented Feb 22, 2022

t-bast left a comment

Choose a reason for hiding this comment

t-bast left a comment

Choose a reason for hiding this comment

t-bast left a comment

Choose a reason for hiding this comment

t-bast commented Feb 25, 2022

rustyrussell commented Feb 28, 2022

rustyrussell commented Feb 28, 2022

Roasbeef commented Feb 28, 2022

t-bast commented Mar 25, 2022

Roasbeef commented Mar 25, 2022

t-bast commented Mar 28, 2022

vincenzopalazzo commented Mar 28, 2022

t-bast commented Mar 28, 2022

Roasbeef commented Mar 28, 2022 • edited Loading

Roasbeef left a comment

Choose a reason for hiding this comment

rustyrussell commented Sep 26, 2022

rustyrussell commented Sep 26, 2022

rustyrussell commented Sep 27, 2022

t-bast left a comment

Choose a reason for hiding this comment

t-bast Sep 27, 2022

Choose a reason for hiding this comment

rustyrussell Sep 29, 2022

Choose a reason for hiding this comment

ViktorTigerstrom Sep 30, 2022

Choose a reason for hiding this comment

Roasbeef commented Mar 28, 2022 •

edited

Loading