Make impl_writeable_tlv_based_enum* actually upgradable
#3160
Conversation
TheBlueMatt
force-pushed
the
2024-07-better-enum-upgradable-ser
branch
from
8ac9205
to
7fb8f80
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3160 +/- ##
==========================================
+ Coverage 89.74% 89.76% +0.01%
==========================================
Files 121 121
Lines 99858 100730 +872
Branches 99858 100730 +872
==========================================
+ Hits 89622 90421 +799
- Misses 7561 7618 +57
- Partials 2675 2691 +16 ☔ View full report in Codecov by Sentry. |
TheBlueMatt
force-pushed
the
2024-07-better-enum-upgradable-ser
branch
from
7fb8f80
to
008a291
Compare
|
Updated after the merge of #3085 to change the format of |
|
Is there anywhere a canonical breakdown of when to use which of:
I wonder if it might be useful as the method names become chaotic. |
Don't ever write The upgradable-vs-not thing is a question of what you want to read - if you do |
| ($st: ident, $(($variant_id: expr, $variant_name: ident) => | ||
| {$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*} | ||
| ),* $(,)*; | ||
| $(($tuple_variant_id: expr, $tuple_variant_name: ident)),* $(,)*) => { | ||
| $(($tuple_variant_id: expr, $tuple_variant_name: ident)),+ $(,)?) => { |
There was a problem hiding this comment.
To check my understanding, this change ensures that we don't use this legacy macro unless there is a non-empty tuple variant present?
There was a problem hiding this comment.
Right, the + here mandates that you actually use a tuple variant, cause if you're not you shouldn't be using legacy. The * -> ? change is just an MSRV cleanup.
TheBlueMatt
force-pushed
the
2024-07-better-enum-upgradable-ser
branch
from
008a291
to
7fd9bfb
Compare
Right, I meant in the code or the docs, as opposed to in the PR :) |
|
Yea, I figured "legacy" for serialization stuff was relatively clear :). |
|
Feel free to squash! |
In cc78b77 it was discovered that `impl_writeable_tlv_based_enum_upgradable` wasn't actually upgradable - tuple variants weren't written with length-prefixes, causing downgrades with new tuple variants to be unreadable by older clients as they wouldn't know where to stop reading. This was fixed by simply assuming that any new variants will be non-tuple variants with a length prefix, but no code write-side changes were made, allowing new code to freely continue to use the broken tuple-variant serialization. Here we address this be defining yet more serialization macros which aren't broken, and convert existing usage of the existing macros using non-length-prefixed tuple variants to renamed `*_legacy` macros. Note that this changes the serialization format of `impl_writeable_tlv_based_enum[_upgradable]` when tuple fields are written, and as such deliberately changes the call semantics for such tuples. Only the serialization format of `MessageContext` is changed here which is fine as it has not yet reached a release of LDK.
|
Squashed without change. |
TheBlueMatt
force-pushed
the
2024-07-better-enum-upgradable-ser
branch
from
7fd9bfb
to
1d1f47c
Compare
In cc78b77 it was discovered that
impl_writeable_tlv_based_enum_upgradablewasn't actually upgradable - tuple variants weren't written with length-prefixes, causing downgrades with new tuple variants to be unreadable by older clients as they wouldn't know where to stop reading.This was fixed by simply assuming that any new variants will be non-tuple variants with a length prefix, but no code write-side changes were made, allowing new code to freely continue to use the broken tuple-variant serialization.
Here we address this be defining yet more serialization macros which aren't broken, and convert existing usage of the existing macros using non-length-prefixed tuple variants to renamed
*_legacymacros.Note that this changes the serialization format of
impl_writeable_tlv_based_enum[_upgradable]when tuple fields are written, and as such deliberately changes the call semantics for such tuples.Given this is a public/exported macro, we may wish to not do this and instead give it a new name to ensure users are forced to recon with the changes rather than just seeing a compilation failure due to new semantics and fixing those (without reading the release notes I guess?). Not sure if its worth it.