Make verifyAttempt more robust #10257
Conversation
2941b9e to
3ec398f
Compare
3ec398f to
db23434
Compare
We now return an error when blinded and non blinded attempts are combined. This was theoretically possible to register a legacy attempt in combination with a blinded payment. This would have been prevented by other checks in the code because legacy payments are not split into shards.
db23434 to
f4816c1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the robustness of payment attempt verification by adding a check to prevent mixing blinded and non-blinded attempts within the same payment. The implementation is straightforward and correct, introducing a new error and the corresponding validation logic in verifyAttempt. The changes are well-supported by an extensive suite of new unit tests, which significantly improves confidence in the payment verification logic. I've added one suggestion to further improve test coverage for the new validation.
| verifyAttempt(payment, &full), | ||
| ErrMixedBlindedAndNonBlindedPayments, | ||
| ) | ||
| } |
There was a problem hiding this comment.
This test covers adding a non-blinded attempt to a payment that already has a blinded attempt. For completeness, it would be beneficial to also test the reverse scenario: adding a blinded attempt to a payment that already has a non-blinded attempt.
)
// Now test the other direction: a payment with a non-blinded attempt
// should not accept a blinded attempt.
nonBlindedExisting := makeLastHopAttemptInfo(
4, lastHopArgs{amt: 2000, mpp: record.NewMPP(total, testHash)},
)
paymentWithNonBlinded := makePayment(
total,
HTLCAttempt{HTLCAttemptInfo: nonBlindedExisting},
)
blindedAttempt := makeLastHopAttemptInfo(
5, lastHopArgs{amt: 2000, total: total, encrypted: []byte{1}},
)
require.ErrorIs(
t,
verifyAttempt(paymentWithNonBlinded, &blindedAttempt),
ErrMixedBlindedAndNonBlindedPayments,
)
}
We now return an error when blinded and non blinded attempts are
combined. This was theoretically possible to register a legacy
attempt in combination with a blinded payment. This would have
been prevented by other checks in the code because legacy payments
are not split into shards.
resulted from #9147 (comment)