I believe that's just a leftover that we should remove from the spec, feel free to open a PR! We should loop until we've exhausted the shared secrets (which matches the number of hops we actually used for the payment path), not until a specific hard-coded value.

I believe that's just a leftover that we should remove from the spec, feel free to open a PR! We should loop until we've exhausted the shared secrets (which matches the number of hops we actually used for the payment path), not until a specific hard-coded value.

Isn't the fixed number of decryptions needed to prevent any intermediate nodes from learning the source/destination via timing? If so, it would make more sense to increase the hard-coded value to 27 instead of removing it.

Maybe we're not talking about the same steps in the onion process. I thought @ziggie1984 meant what is done by the sender of a payment when it receives an update_fail_htlc message and tries to decrypt the error contained in that message. In that case I don't think there is any timing-related attacks that applies?

Yeah the spec just refers to the origin node in this part:

The origin node:

once the return message has been decrypted:
SHOULD store a copy of the message.
SHOULD continue decrypting, until the loop has been repeated 20 times.
SHOULD use constant ammag and um keys to obfuscate the route length.

https://github.com/lightning/bolts/blob/master/04-onion-routing.md#returning-errors

So I think we can remove it.

I am also confused about the last part:
SHOULD use constant ammag and um keys to obfuscate the route length.

I am not sure what is meant by obfuscate, because the origin node only cares about de-obfuscating, the ammag and um keys are locked in at this point for the routing hops (defined when building the route?). So not sure why this is important. I thought the erring nodes decides the size of the failure message and all the following nodes should keep the size constant?

And not sure about the first requirement as well:

SHOULD store a copy of the message.

Should we really require this, it's up to the sender to decide whether he wants to keep the data or not, I think relay protection is not really relevant for the Erring Messages?

I believe @morehouse is right, those 3 requirements were added to be somewhat "constant-time" on the sender side when decrypting an update_fail_htlc error. What those requirements suggest is that after correctly decrypting the error message after N steps (where N is the position of the erring node in the route) you should keep doing dummy decryption steps to make it look like the error came from the 20th node in the route. Since the route is usually smaller than 20, you don't have ammag and um keys for the fake hops, so the requirements suggest using dummy constant ones.

I believe all of this is useless (timing leaks are dominated by network latency in retries), so we have not implemented this in eclair and we instead stop as soon as we get the decrypted message. Those requirements are only a SHOULD, they aren't a MUST, so it's perfectly ok to ignore them.

Ok now I understand, it specifically relates to the case where we would try the same payment via a different route and if we do not account for timing we would kind of reveal to the nodes in the path at which position they are probably in.

So I think 20 seems like good enough. I think it probably makes sense to add more context (Rationale) to this part of the requirement I can add a PR describing it if we want that or even increase the hop size to 27?