Add missing deserialization of Event::HTLCHandlingFailed #1700
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1700 +/- ##
==========================================
+ Coverage 90.90% 91.17% +0.27%
==========================================
Files 86 86
Lines 46304 48276 +1972
Branches 46304 48276 +1972
==========================================
+ Hits 42091 44014 +1923
- Misses 4213 4262 +49
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Should have been caught in review. |
| let mut failed_next_destination_opt = None; | ||
| read_tlv_fields!(reader, { | ||
| (0, prev_channel_id, required), | ||
| (2, failed_next_destination_opt, ignorable), |
There was a problem hiding this comment.
Why do we read it as an Option when it's marked required when writing?
There was a problem hiding this comment.
I believe it's because HTLCDestination::NextHopChannel may fail to deserialize when serialized with a version prior to 0.0.110? And instead of handing the error up the chain and quitting deserialization altogether, we just don't read this event? If that's the case, a comment explaining this would indeed be appreciated.
There was a problem hiding this comment.
Two reasons - (a) we can't construct a "dummy" HTLCDestination, which we have to do before we call read_tlv_fields so we construct an Option and check it after and (b) what @tnull said - its ignorable (ie deserialized with MaybeReadable) not optional in the read macro nomenclature.
There was a problem hiding this comment.
I believe it's because HTLCDestination::NextHopChannel may fail to deserialize when serialized with a version prior to 0.0.110? And instead of handing the error up the chain and quitting deserialization altogether, we just don't read this event?
Prior to 0.0.110 wouldn't we just ignore the event all together based on the odd type (25)?
There was a problem hiding this comment.
Errr, sorry, the opposite of that - we may fail to read an HTLCDestination (its MaybeReadable) if a future version of LDK writes a new variant that we don't understand.
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Generally looks good, mod the comment above.
| let mut failed_next_destination_opt = None; | ||
| read_tlv_fields!(reader, { | ||
| (0, prev_channel_id, required), | ||
| (2, failed_next_destination_opt, ignorable), |
There was a problem hiding this comment.
I believe it's because HTLCDestination::NextHopChannel may fail to deserialize when serialized with a version prior to 0.0.110? And instead of handing the error up the chain and quitting deserialization altogether, we just don't read this event? If that's the case, a comment explaining this would indeed be appreciated.
TheBlueMatt
force-pushed
the
2022-09-missing-event-deser
branch
from
ba0793f to
089ccbb
Compare
|
Squashed without further changes. |
5 participants
17e6c37 added the
HTLCHandlingFailedevent, including serialization thereof, however failed to add corresponding deserialization. This corrects that oversight by adding said deserialization.Thanks to @wpaulino for catching the oversight.