Attributable failures (feature 36/37)

[joostjager]

Failure attribution is important to properly penalize nodes after a payment failure occurs. The goal of the penalty is to give the next attempt a better chance at succeeding. In the happy failure flow, the sender is able to determine the origin of the failure and penalizes a single node or pair of nodes.

Unfortunately it is possible for nodes on the route to hide themselves. If they return random data as the failure message, the sender won't know where the failure happened.

This PR proposes a new failure message format that lets each node commit to the failure message. If one of the nodes corrupts the failure message, the sender will be able to identify that node.

For more information, see https://lists.linuxfoundation.org/pipermail/lightning-dev/2022-October/003723.html.

LND implementation: lightningnetwork/lnd#7139

LDK implementation: lightningdevkit/rust-lightning#3611

Eclair implementation: ACINQ/eclair#2519

[thomash-acinq]

I've started implementing it in eclair, do you have some test vectors so we can check that we are compatible?
The design seems good to me, but as I've said previously, I think keeping hop payloads and hmacs for 8 nodes only (instead of 27) is enough for almost all use cases and would give us huge size savings.

[joostjager]

I don't have test vectors yet, but I can produce them. Will add them to this PR when ready.

Capping the max hops at a lower number is fine to me, but do you have a scenario in mind where this would really make the difference? Or is it to more generally that everything above 8 is wasteful?

[joostjager]

@thomash-acinq added a happy fat error test vector.

[joostjager]

I think this big gap in the bits has emerged here because of tentative spec changes that may or may not make it. Not sure why that is necessary. I thought for unofficial extensions, the custom range is supposed to be used?

I can see that with unofficial features deployed in the wild, it is easier to keep the same bit when something becomes official. But not sure if that is worth creating the gap here? An alternative is to deploy unofficial features in the custom range first, and then later recognize both the official and unofficial bit. Slightly more code, but this feature list remains clean.

[joostjager]

Added fat error signaling to the PR.

[thomash-acinq]

I've spent a lot of time trying to make the test vector pass and I've finally found what was wrong:
In the spec you write that the hmac covers

• failure_len, failuremsg, pad_len and pad.

• The first y+1 payloads in payloads. For example, hmac_0_2 would cover all three payloads.

• y downstream hmacs that correspond to downstream node positions relative to x. For example, hmac_0_2 would cover hmac_1_1 and hmac_2_0.

implying that we need to concatenate them in that order. But in your code you follow a different order:

// Include payloads including our own.
_, _ = hash.Write(payloads[:(NumMaxHops-position)*payloadLen])

// Include downstream hmacs.
var hmacsIdx = position + NumMaxHops
for j := 0; j < NumMaxHops-position-1; j++ {
	_, _ = hash.Write(
		hmacs[hmacsIdx*sha256.Size : (hmacsIdx+1)*sha256.Size],
	)

	hmacsIdx += NumMaxHops - j - 1
}

// Include message.
_, _ = hash.Write(message)

I think the order message + hop payloads + hmacs is more intuitive as it matches the order of the fields in the packet.

[joostjager]

Oh great catch! Will produce a new vector.

[joostjager]

@thomash-acinq updated vector

[joostjager]

Updated LND implementation with sender-picked fat error structure parameters: lightningnetwork/lnd#7139

[joostjager]

@thomash-acinq, shall we pick up attributable errors again where we left?

[thomash-acinq]

@thomash-acinq, shall we pick up attributable errors again where we left?

Yes we should. I'm on vacation right now but I will find time for it in the next weeks.

[joostjager]

We could also consider an alternative system where the use-attributable-errors bit is in the update_add_htlc message instead of being inside the onion. That way we always have access to it even if the onion is invalid (or if we decide to reject the HTLC before even reading the onion, for instance if the CLTV is too low). And by adding a new error message to wrap a legacy error, we could even get partial attributability if the first nodes of the route support attributable errors but not the last ones.

Looking at this with fresh eyes, I am wondering if a variation on this could be a interesting option (not sure if it has already been brought up previously - this projects now spans multiple years):

Extend update_fail_htlc with a tlv field at the end that holds the attributable error. A failure source that supports it, would populate both the legacy failure field as well as the attr error tlv field.

Legacy intermediate nodes would just drop the attributable error again. Intermediate nodes that do support attr errs would process both failure fields. If such an intermediate nodes encounters a missing attr errs tlv field, it will fill that field with the wrapped legacy failure, to support partial paths.

For legacy senders, the legacy failure with its single hmac will always be available. Senders supporting attr errs would process the attr err tlv field, if available.

The advantage of this setup is that no signaling in the forward path is needed. Nodes could still advertise the feature in their node announcement, but perhaps even that isn't necessary. At some point, senders may want to avoid nodes not supporting attributable errors. They may be able to identify such nodes via attr errors that wrap a legacy failure, indicating that the node directly downstream doesn't support them.

Perhaps it isn't even necessary to duplicate the failure message itself in the attr errs tlv field. Instead it can take it from the legacy field (minus the legacy hmac), so that the attr errs tlv field just contains the payloads and hmacs.

[GeorgeTsagk]

Great proposal! Quite an elegant idea
There are some elusive details that make this work, which might need some more coverage

[GeorgeTsagk]

could elaborate a bit more on why these hmacs can be discarded / considered irrelevant

[joostjager]

Added a bit more explanation.

[GeorgeTsagk]

should also include the "unhappy" path here, focusing on:

• why origin attributes blame/error on first decoding failure (if that's still the intended way in which things work)
• how an intermediary node that maliciously flips bits cannot shake the blame off themselves

[joostjager]

What are origin attributes?

I think I've elaborated on this more in the latest iteration of the PR.

[GeorgeTsagk]

can the forwarding node always pick the correct pruning positions if it doesn't know its own position in the path?

[joostjager]

Yes, the forwarding node can pick the correct pruning positions. The block of hmacs that the forwarding node receives contains series of hmacs for all possible path lengths up to 20 hops. The forwarding node obviously knows that it is also part of the path, so there can never be an additional 20 hops (this would bring the total to 21). This also means that the last hmac for every hmac series won't ever be useful.

Will try to add some more text to explain this.

[GeorgeTsagk]

to 210 hmacs

there was the (max_hops * (max_hops + 1)) / 2 formula in a previous diff, explaining how this number was produced, seems to have been trimmed

[joostjager]

Re-added explanation of 210.

[joostjager]

One thing I now realize is that even if there's a node on the path that messes with the data and/or hmacs, the upstream nodes will still have valid hmacs and also valid hold_time_ms.

Information that isn't all that useful in that case though. The sender will simply proceed with penalizing the bad node, retry, and only process timing information after "proper" failures.

[joostjager]

If we go for the idea described in #1044 (comment), it may be unnecessary to keep that first payload byte that is either 0 or 1 to indicate intermediate or failing node. Because the 'legacy' hmac is still present, that can also be used to identify the failing node.

[joostjager]

Thanks for your review @GeorgeTsagk. Comments addressed.

[joostjager]

PR rebased and updated to reflect the new approach with a tlv extension to update_fail_htlc.

[joostjager]

Decided to derive a new key rather than continue pulling bits for chacha from the ammag key, to minimize the chance of somehow reusing the stream. Alternatively could use a different nonce, but I believe that would be new in lightning?

[joostjager]

Updated test vectors