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 mandatory #1810

Merged
merged 1 commit into from
May 17, 2021
Merged

Make payment_secret mandatory #1810

merged 1 commit into from
May 17, 2021

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented May 17, 2021

This is a security feature that has been introduced a long time ago and is widely supported across the network.
We can safely make it mandatory which closes probing attack vectors.

A separate follow-up PR will remove some legacy code that handled cases where payment_secret could be missing.
I'd rather first release the activation bits and then remove that legacy code instead of bundling the two together.

This is a security feature that has been introduced a long time ago and is
widely supported across the network.

We can safely make it mandatory which closes probing attack vectors.
@t-bast t-bast marked this pull request as ready for review May 17, 2021 10:54
@t-bast t-bast requested a review from pm47 May 17, 2021 10:54
@pm47
Copy link
Member

pm47 commented May 17, 2021

cc @dpad85 may have some impact on old Eclair Mobile IIUC

Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

Should we make our onion type stricter and simplify the relayers at the same time? #1770 (comment). Or do you think we should wait because there may be pre-existing payment requests that have not yet expired?

@t-bast
Copy link
Member Author

t-bast commented May 17, 2021

Should we make our onion type stricter and simplify the relayers at the same time? #1770 (comment). Or do you think we should wait because there may be pre-existing payment requests that have not yet expired?

I'd rather do it in a separate PR, in the release after we make payment_secret mandatory.
I started removing all legacy code related to var_onion_optin and payment_secret, and I think it's quite a lot of subtle changes so I prefer doing it after the release, for the next one.

@t-bast
Copy link
Member Author

t-bast commented May 17, 2021

cc @dpad85 may have some impact on old Eclair Mobile IIUC

Support for payment_secret was added to eclair mobile in the v0.4.7 release in July 2019. Only wallets older than that will not be able to connect to our node anymore and will need to update. A lot has happened since then, so it's really recommended that they upgrade anyway!

@t-bast t-bast merged commit 9141998 into master May 17, 2021
@t-bast t-bast deleted the mandatory-payment-secret branch May 17, 2021 13:09
t-bast added a commit that referenced this pull request Jun 8, 2021
The `payment_secret` feature was made mandatory in #1810 and is the default
in other implementations as well. We can thus force it to be available when
decoding onion payloads, which simplifies downstream components (no need
to handle the case where a `payment_secret` may be missing anymore).
t-bast added a commit that referenced this pull request Jun 11, 2021
The `payment_secret` feature was made mandatory in #1810 and is the default
in other implementations as well. We can thus force it to be available when
decoding onion payloads, which simplifies downstream components (no need
to handle the case where a `payment_secret` may be missing anymore).

We also rename messages in `PaymentInitiator` to remove the confusion with
Bolt 11 payment requests.
@joostjager
Copy link

@t-bast could it be that payment_secret is mandatory on the ACINQ node, but that its feature bits are still communicated as optional?

I am running into INVALID_ONION_PAYLOAD when probing the ACINQ node without a payment secret.

@t-bast
Copy link
Member Author

t-bast commented Aug 24, 2021

Yes that's exactly it.

We set the feature bit as mandatory in invoices (because that's where this feature bit makes sense) but not in our init / node_announcement. Since probing is done without an invoice, that's why you're not getting this information.

Depending on what library you're probing with, this may already be fixed if you upgrade (see #1913).

@joostjager
Copy link

Why don't you set the feature bit as mandatory in the init / node_announcement too?

@t-bast
Copy link
Member Author

t-bast commented Aug 24, 2021

Because there's no reason to yet (as a relaying-only node you don't care at all about payment_secret, you don't even need to set it) and some of our peers don't have it in their init which would break our connection to them for no good reason.

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