Cut 0.0.122 with just #2969, #2897, and #2937 #2985
Conversation
`impl_writeable_tlv_based_enum_upgradable` professed to supporting upgrades by returning `None` from `MaybeReadable` when unknown variants written by newer versions of LDK were read. However, it generally didn't support this as it didn't discard bytes for unknown types, resulting in corrupt reading. This is fixed here for enum variants written as a TLV stream, however we don't have a length prefix for tuple enum variants, so the documentation on the macro is updated to mention that downgrades are not supported for tuple variants.
If we are reading an object that is `MaybeReadable` in a TLV stream using `upgradable_required`, it may return early with `Ok(None)`. In this case, it will not read any further TLVs from the TLV stream. This is fine, except that we generally expect `MaybeReadable` always consume the correct number of bytes for the full object, even if it doesn't understand it. This could pose a problem, for example, in cases where we're reading a TLV-stream `MaybeReadable` object inside another TLV-stream object. In that case, the `MaybeReadable` object may return `Ok(None)` and not consume all the available bytes, causing the outer TLV read to fail as the TLV length does not match.
Whils this is generally not supported, issues in our `MaybeReadable` implementations may occur, and we should try to be robust against them.
TheBlueMatt
force-pushed
the
2024-03-fix-upgradable-enum-121
branch
from
f04ff0c
to
2983898
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## 0.0.122 #2985 +/- ##
===========================================
+ Coverage 88.86% 89.00% +0.13%
===========================================
Files 115 114 -1
Lines 92753 91225 -1528
Branches 92753 91225 -1528
===========================================
- Hits 82429 81197 -1232
+ Misses 7942 7904 -38
+ Partials 2382 2124 -258 ☔ View full report in Codecov by Sentry. |
|
Maybe worth including the various CI fixes to assert it passes? |
|
Eh, only |
Well, seems now even more failed, but for unrelated reasons. I guess we'll have 0.0.123 coming up anyways.. |
|
Right there's also the ENOSPACE issue on 1.63 :/. In any case all the failures here are relatively harmless... |
TheBlueMatt
force-pushed
the
2024-03-fix-upgradable-enum-121
branch
from
2983898
to
ed7d7c1
Compare
|
Squash-pushed the sp fixup from @jkczyz |
`Route`'s blinded_path serialization logic writes a blinded path `Option` per path hop, however on read we (correctly) only read one blinded path `Option` per path. This causes serialization of `Route`s with blinded paths to fail to round-trip. Here we fix this by writing blinded paths per path.
When the `max_total_routing_fee_msat` parameter was added to `RouteParameters`, the serialization used `map` to get the max fee, accidentally writing an `Option<Option<u64>>`, but then read it as an `Option<u64>`. Thus, any `Route`s with a `route_params` written will fail to be read back. Luckily, this is an incredibly rarely-used bit of code, so only one user managed to hit it.
When we hit lnd bug 6039, we end up sending error messages to peers in a loop. This should be fine, but because we used the generic `PersistenceNotifierGuard::notify_on_drop` lock above the specific handling, we end up writing `ChannelManager` every time we manage a round-trip to our peer. This can add up quite quickly, and isn't actually changing, so we really need to avoid writing the `ChannelManager` in this case.
TheBlueMatt
force-pushed
the
2024-03-fix-upgradable-enum-121
branch
from
ed7d7c1
to
66ebe7e
Compare
TheBlueMatt
changed the title
|
Will wait for CI to make sure it at least only fails on the things it was already failing on, then will cut this! |
|
Okay, only beta and the 1.63 build failed, so gonna cut this. |
This is just the three commits form #2969 backported to 0.0.121 with a CHANGELOG entry for 0.0.122.
As discussed in #2969 I think we should ship this as a standalone 0.0.122 and then do 0.0.123 with all the various additional things since 0.0.121.