NodeRelayer should index payments by payment_hash || payment_secret

[t-bast]

Right now, the NodeRelayer indexes child actors by payment_hash only.
It means that senders must know beforehand the total amount they will send through each trampoline node.
This works fine for single-trampoline scenarios, but will not work with multi-trampoline scenarios: if the payer splits his payment between several trampoline routes and one fails, he may end up sending more additional HTLCs to a previous route when retrying.
Right now, we will reject them because the payment_secret doesn't match, or we'll simply fail to relay them (if the payment_secret is reused) and wait for a timeout from the final recipient.

We should instead index by payment_hash || payment_secret, but that requires extracting the payment_secret a bit higher up in our call stack.

[pm47]

That one looks subtle 🤓

It means that senders must know beforehand the total amount they will send through each trampoline node.

Can you clarify?

We should instead index by payment_hash || payment_secret,

Wouldn't that have the side effect of us not being able anymore to differentiate between a wrong payment_secret and a missing payment part?

[t-bast]

Imagine Alice tries to make the following payment, where Bob and Carol are distinct trampoline nodes:

  +-------> Bob -------+
  |                    |
Alice                 Dave
  |                    |
  +-------> Carol -----+

Alice sends N htlcs to Bob and M htlcs to Carol, all part of the same payment to Dave.
Bob correctly forwards to Dave, which starts waiting for the remaining parts.
But Carol doesn't have enough liquidity so she fails the payment.
Alice retries ignoring Carol and ends up sending another batch of htlcs through Bob.
Right now these htlcs will be instantly rejected by Bob, so the payment cannot succeed.
But if that second batch uses a different payment_secret (which it should), Bob could treat it separately from the first batch and Dave will receive the complete payment.

Wouldn't that have the side effect of us not being able anymore to differentiate between a wrong payment_secret and a missing payment part?

I don't think we have a choice, we must allow having multiple payments for the same payment_hash but with distinct payment_secrets to unblock the multi-trampoline scenario...there is not such thing as a "wrong" payment_secret for intermediate trampoline nodes, different payment_secrets mean different payment attempts by the sender.

[ecdsa]

is this currently deployed on the ACINQ node?

[t-bast]

@ecdsa yes it is, let me know if you're seeing issues with it.