htlcswitch/hop: use InvalidOnionVersion for replayed packets

[Crypt-iQ]

The link will send an update_fail_malformed_htlc, so we need to set the BADONION bit. Since there isn't a replay-specific error, we set the failure code to InvalidOnionVersion which has the BADONION bit.

It would be nice if there was a replay-specific failure code in the spec

Fixes #7851, #5792, #5443

[carlaKC]

Seems like this should be followed up with defining a spec error code, but something with BADONION is def a better choice than temporary channel failure 👍

[morehouse]

Why isn't testSphinxReplayPersistence failing before this PR?

Specifically:

lnd/itest/lnd_misc_test.go

Lines 221 to 222 in 13568fd

	ht.AssertPaymentStatusFromStream(payStream, lnrpc.Payment_FAILED)
	ht.AssertLastHTLCError(fred, lnrpc.Failure_INVALID_ONION_KEY)

[yyforyongyu]

Why isn't testSphinxReplayPersistence failing before this PR?

@morehouse This is because when relaying HTLCs, the relaying node will convert the TemporaryChannelFailure to FailInvalidOnionKey. Looking at the test,

First, Dave sends a update_malformed_htlc based on the check here, his logs,

2023-09-01 17:39:14.703 [ERR] HSWC: unable to process onion packet: sphinx packet replay attempted
2023-09-01 17:39:14.703 [ERR] HSWC: ChannelLink([ChanPoint: Carol=>Dave]): unable to decode onion hop iterator: TemporaryChannelFailure
...
2023-09-01 17:39:14.703 [DBG] PEER: Peer([Carol]): Sending UpdateFailMalformedHTLC(chan_id=902748dd38578ed72c6eba0307e5a004716a807f664b5d41eb483b0ae8faa39e, id=1, fail_code=TemporaryChannelFailure) to [Carol]@127.0.0.1:54338

Carol will check the failure code and send FailInvalidOnionKey because of the check here, her logs,

2023-09-01 17:39:14.703 [DBG] PEER: Peer([Dave]): Received UpdateFailMalformedHTLC(chan_id=902748dd38578ed72c6eba0307e5a004716a807f664b5d41eb483b0ae8faa39e, id=1, fail_code=TemporaryChannelFailure) from [Dave]@127.0.0.1:5569
...
2023-09-01 17:39:14.703 [WRN] HSWC: ChannelLink([ChanPoint: Carol=>Dave]): unexpected failure code received in UpdateFailMailformedHTLC: TemporaryChannelFailure
...
2023-09-01 17:39:15.394 [INF] HSWC: Converting malformed HTLC error for circuit for Circuit(a8d2fa107cd88a5acf214564027a72e525968b1e85a2af19e69b74aa08a1c3be: (449:1:0, 0) <-> (442:1:0, 1))

Fred would receive the InvalidOnionKey error as shown in his logs,

2023-09-01 17:39:16.143 [WRN] CRTR: Attempt 1 for payment a8d2fa107cd88a5acf214564027a72e525968b1e85a2af19e69b74aa08a1c3be failed: InvalidOnionKey(onion_sha=fbd635e56867841888333edf3c7e450a11f9d075a44211607d50eb7e9401e383)@1

After this change, Dave would now send InvalidOnionKey,

2023-09-01 18:19:41.208 [DBG] PEER: Peer([Carol]): Sending UpdateFailMalformedHTLC(chan_id=a545313560caeed68ce9a4003af01904154b1e9621fd891a99a0155449371102, id=1, fail_code=InvalidOnionKey) to [Carol]@127.0.0.1:55071

The current test sends the payment using fred -> carol -> dave. If we wanna test it, we can also send a payment using carol -> dave and assert the expected failure code is returned.

[morehouse]

@yyforyongyu thanks for the thorough explanation!

The current test sends the payment using fred -> carol -> dave. If we wanna test it, we can also send a payment using carol -> dave and assert the expected failure code is returned.

We should definitely test the new behavior. Extending testSphinxReplayPersistence is probably the easiest route.

[lightninglabs-deploy]

@Crypt-iQ, remember to re-request review from reviewers when ready

[Crypt-iQ]

If we wanna test it, we can also send a payment using carol -> dave and assert the expected failure code is returned.

The unsafe-replay option only works if there's a multi-hop so testing carol -> dave won't work. I just modified the code to use InvalidOnionVersion and changed the itest so it now passes. In the future we might be able to use a specific failure code

[morehouse]

Needs a rebase.

[morehouse]

LGTM

[yyforyongyu]

Pending release notes, otherwise LGTM🙏

[Crypt-iQ]

added release notes

[saubyk]

Tagged it for 17.1. Need to update the release notes to 17.1

[Roasbeef]

@Crypt-iQ release notes need to be updated for 0.17.1, then this can land.

[Crypt-iQ]

Updated the 0.17.1 release notes, but this is still based on 0-18-staging, what branch should this be based on instead?

[guggero]

Since the merge window is open again, this can be based on master directly.

[Crypt-iQ]

Updated to be based on master

[yyforyongyu]

weird the CI seems to be halted