Make invoice's `s` flag mandatory ?

[ariard]

See CVE 2020-26896 for context.

[cdecker]

While technically not possible without breaking backward compatibility (recipient has no idea whether the sender supports it) I think at this point it's unlikely to be a major issue, since all implementations support this feature by now.

[t-bast]

We could test the current network for nodes that don't advertise payment_secret and see what percentage of the network would be negatively impacted before we decide.

[t-bast]

I've run this against our current network DB.
I have 5667 node announcements, but only 3090 of those advertise support for payment_secret...

[Roasbeef]

Well the only nodes that matter are those sending nodes. Which are usually mobile phones, so they aren't counted in the public node feature bit count.

Tentatively, lnd plans to flip this to required in all our invoices for our next upcoming release (drops this month).

[t-bast]

It's a good point, node_announcements aren't a good measure, we don't really care whether intermediate routing nodes support it or not. Better would be to get in touch with wallets then, I'll try to contact some of them.

[cfromknecht]

While technically not possible without breaking backward compatibility (recipient has no idea whether the sender supports it)

IIRC the rationale behind invoice features was to skirt this issue so that it's now a soft error (feature bit incompatibility) rather finding out after making a payment attempt. As long as senders support invoice features they will at least be given a feature bit error rather than an ambiguous payment error after making an attempt.

I'd be very surprised to find folks running economically-relevant nodes that do not have sender-side payment secret support. Even if there are a few, I can't imagine a better use case for flipping a feature to required (slightly) prematurely than for a security-critical feature that prevents loss of funds in transit.

I personally wouldn't lose any sleep over choosing to protect all economically-active receivers at the expense of a handful of senders that haven't done a software update in over a year. After all, it's the receiver's money that continues to be at stake if we continue to delay for stragglers that otherwise have no real incentive to upgrade.

Better would be to get in touch with wallets then, I'll try to contact some of them.

Any updates on this? :)

[cfromknecht]

One question that came up while implementing this is what error to return when a legacy payment hits an invoice requiring payment secrets. My first thought was to continue overloading incorrect-payment-details, maybe there's a better approach?

[t-bast]

My first thought was to continue overloading incorrect-payment-details

That's what we do as well. As you stated before, it would only happen for very old senders who don't even understand the required feature bit in the invoice, so I think it's an ok behavior.

I personally wouldn't lose any sleep over choosing to protect all economically-active receivers at the expense of a handful of senders that haven't done a software update in over a year. After all, it's the receiver's money that continues to be at stake if we continue to delay for stragglers that otherwise have no real incentive to upgrade.

I completely agree, I would also be in favor of setting this to required.

Any updates on this? :)

It looks like Breez and BLW correctly support payment_secret, and I'd be surprised if zap didn't.
I don't know anyone at other wallets, maybe if someone can ping BlueWallet and WalletOfSatoshi, we'd have a pretty good percentage of wallet users covered?

[Roasbeef]

BlueWallet and WoS both use lnd, and assuming they observed the latest CVE they're running 0.11. So far we're planning on enabling it by default (to require the mpp payload) in 0.12. We're doing it in a manner that'll still l et any old invoices that are non-expired that only had the optional bit set be settled still.

[t-bast]

Most implementations have made this mandatory in their latest releases, so I think we can (finally) close this issue! 🎉

[TheBlueMatt]

Shouldn’t the BOLT be updated for this?

…

On Aug 10, 2021, at 01:30, Bastien Teinturier ***@***.***> wrote:  Closed #809. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[t-bast]

Don't you think having most nodes on the network advertise it as required is enough? It will prevent nodes that don't support it from even connecting to the network.

But we can also add it to the spec, maybe bundle it with this change #887 that updates the Bolt 11 test vectors to contain a payment_secret? I'll give that a go.

[t-bast]

@TheBlueMatt done in ec1d4dc, let me know if that sounds good