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

Make payment secret compulsory on new invoices, and assume unknown nodes support TLV onions #4646

Merged

Conversation

rustyrussell
Copy link
Contributor

Most of this work is removing our assumptions that payment_secret is unnecessary (i.e. making us use modern TLV onion for the terminal everywhere!).

We also switch our assumptions about nodes for which we don't have a node_announcement: we assume they do support TLV onions. With the next release, we might be able to remove legacy sending support altogether (and then finally, legacy receive/forward).

This makes it easier to access (rather than decoding bolt11).

Changelog-Added: JSON-RPC: `invoice` now outputs explicit `payment_secret` it its own field.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
They're about to become compulsory.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the guilt/payment-secret-compulsory branch 2 times, most recently from 04a3948 to 0b885f8 Compare July 12, 2021 10:09
@rustyrussell rustyrussell added this to the v0.10.1 milestone Jul 12, 2021
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Making things compulsory always feels dangerous but in this case it looks ok

ACK 0b885f8

Comment on lines 312 to 317
for (size_t i = 0; i < tal_count(ki->payload->fields); i++) {
plugin_log(cmd->plugin, LOG_DBG,
"field[%zu] = %zu:%s",
i, ki->payload->fields[i].numtype,
tal_hex(tmpctx, ki->payload->fields[i].value));
}
Copy link
Member

Choose a reason for hiding this comment

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

Better to add a line saying what we're printing here and indenting slightly, otherwise we have random values appearing in the logs without any context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, debug!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, thanks!

It's about to become compulsory.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's going to be compulsory soon!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In fact, we make it compulsory, which means if you don't understand it
you'll hang up on us!

Add some logging for that in future.

Changelog-Changed: Protocol: All new invoices require a payment_secret (i.e. modern TLV format onion)
Changelog-Changed: Protocol: We can no longer connect to peers which don't support `payment_secret`.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This grandfathers in old invoices for the moment.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is in preparation for removing support (next release?).

Changelog-Changed: Protocol: We now assume nodes support TLV onions (non-legacy) unless we have a node_announcement which says they don't.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the guilt/payment-secret-compulsory branch from 0b885f8 to 0433cec Compare July 12, 2021 20:49
@niftynei
Copy link
Collaborator

niftynei commented Jul 14, 2021

ACK 0433cec

@niftynei niftynei merged commit 28553e9 into ElementsProject:master Jul 14, 2021
SomberNight added a commit to SomberNight/electrum that referenced this pull request Jun 29, 2023
we now require payment_secret both for sending and for receiving
(previously was optional for both)

see
lightning/bolts#898
ACINQ/eclair#1810
ElementsProject/lightning#4646

note: payment_secret depends on var_onion_optin, so that becomes mandatory as well,
however this commit does not yet remove the ability of creating legacy onions
SomberNight added a commit to SomberNight/electrum that referenced this pull request Jun 29, 2023
we now require payment_secret both for sending and for receiving
(previously was optional for both)

see
lightning/bolts#898
ACINQ/eclair#1810
ElementsProject/lightning#4646

note: payment_secret depends on var_onion_optin, so that becomes mandatory as well,
however this commit does not yet remove the ability of creating legacy onions
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.

3 participants