Fix bogus Event::PaymentSent serialization
#973
Conversation
`Event::PaymentSent` serialization has a bug where we double-serialize the payment_preimage field but do not attempt to read it twice. This results in a failure to read `ChannelManager`s from disk if we have a pending `Event::PaymentSent` pending awaiting handling when we serialize. Instead of attempting to read both versions, we opt to simply fix the serialization, assuming it is exceedingly rare for such a scenario to appear (and if it does, we can assist in manual recovery). The release notes have been updated to note this inconsistency. Found by the chanmon_consistency fuzz target.
Codecov Report
@@ Coverage Diff @@
## main #973 +/- ##
==========================================
- Coverage 90.65% 90.65% -0.01%
==========================================
Files 60 60
Lines 30398 30397 -1
==========================================
- Hits 27558 27556 -2
- Misses 2840 2841 +1
Continue to review full report at Codecov.
|
| @@ -160,7 +160,6 @@ impl Writeable for Event { | |||
| write_tlv_fields!(writer, { | |||
| (0, payment_preimage, required), | |||
| }); | |||
| payment_preimage.write(writer)?; | |||
There was a problem hiding this comment.
Darn! If anyone wants to post-mortem, this seems to be the commit where this occurred: 263bf9e#diff-3b5c370b404e2f5a8f3c35093b97406f149a9340c177c05252574083d68df0daR160-R163 At least serialization was a bit of a one-off big PR.
There was a problem hiding this comment.
Yea, good thing we have fuzzers (when I remember to check their output...).
There was a problem hiding this comment.
Yes, I believe so.
| * Due to a bug discovered in 0.0.98, if a `ChannelManager` is serialized on | ||
| version 0.0.98 while an `Event::PaymentSent` is pending processing, the | ||
| `ChannelManager` will fail to deserialize both on version 0.0.98 and later | ||
| versions. If you have such a `ChannelManager` available, a simple patch will |
There was a problem hiding this comment.
What would the patch ? Adding back a payment.preimage: Readable::read(reader)?
There was a problem hiding this comment.
Yea, just reading (and throwing away) an extra 32 bytes.
| @@ -160,7 +160,6 @@ impl Writeable for Event { | |||
| write_tlv_fields!(writer, { | |||
| (0, payment_preimage, required), | |||
| }); | |||
| payment_preimage.write(writer)?; | |||
Event::PaymentSentserialization has a bug where wedouble-serialize the payment_preimage field but do not attempt to
read it twice. This results in a failure to read
ChannelManagersfrom disk if we have a pending
Event::PaymentSentpendingawaiting handling when we serialize.
Instead of attempting to read both versions, we opt to simply fix
the serialization, assuming it is exceedingly rare for such a
scenario to appear (and if it does, we can assist in manual
recovery).
The release notes have been updated to note this inconsistency.
Found by the chanmon_consistency fuzz target.