Don't fail on invalid extra tlv data when decoding a payment #10334
Conversation
ziggie1984
force-pushed
the
fix-decoding-extra-tlv-data
branch
from
4d5ac7f to
32a4bef
Compare
guggero
left a comment
guggero
left a comment
There was a problem hiding this comment.
tACK, LGTM 🎉
Thanks a lot for the quick fix!
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
LGTM! 🏆
Please add a regression test (draft sent privately).
ziggie1984
force-pushed
the
fix-decoding-extra-tlv-data
branch
from
801f34c to
7e028dc
Compare
|
unit test added ! |
ziggie1984
force-pushed
the
fix-decoding-extra-tlv-data
branch
from
7e028dc to
e74fa5b
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a change to gracefully handle invalid TLV data when decoding payments, which can occur with older payments. This is achieved by wrapping TLV parsing errors in lnwire.ChannelUpdate1.Decode and then specifically checking for this error type in payments/db/kv_store.go:deserializeHTLCFailInfo. Instead of failing, it now logs a warning and continues, making the system more robust to legacy data formats. The changes are well-tested with a new unit test that constructs a corrupted TLV stream to verify the new behavior. The logic is sound and the implementation is correct. I have one suggestion to improve the readability of the error handling logic.
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
One last thing about the %w: %w usage, otherwise good to go!
Older payments might have a invalid tlv stream, we now catch this case and add a unit test.
ziggie1984
force-pushed
the
fix-decoding-extra-tlv-data
branch
from
e74fa5b to
870d603
Compare
|
@guggero I wonder how often this happened in case of your node, maybe we do some have some older version which produces wrong InboundFee tlv values which we need to investigate. |
I only seem to have a single payment with such a failure. So I don't think any further investigation is necessary, seems to have been a fluke. |
4 participants
Older payments might have a invalid tlv stream, we now catch this
case.