TLV-ize enum serialization and a few additional structs #935
Conversation
TheBlueMatt
force-pushed
the
2021-05-enum-tlvs
branch
3 times, most recently
from
6d08b25 to
f546dcc
Compare
Codecov Report
@@ Coverage Diff @@
## main #935 +/- ##
==========================================
+ Coverage 90.56% 90.61% +0.04%
==========================================
Files 60 60
Lines 30698 30393 -305
==========================================
- Hits 27803 27540 -263
+ Misses 2895 2853 -42
Continue to review full report at Codecov.
|
|
CI fail is just the route graph test needs to be updated. |
| payment_secret.write(writer)?; | ||
| amt.write(writer)?; | ||
| user_payment_id.write(writer)?; | ||
| write_tlv_fields!(writer, { |
There was a problem hiding this comment.
Why not use _impl_writeable_tlv_enum for Event?
There was a problem hiding this comment.
Two reasons: (a), which we could change, is we don't write out the time_forwardable field in PendingHTLCsForwardable and (b) We don't write out (well, actually, read) the FundingGenerationReady event.
There was a problem hiding this comment.
For (b), should this be the responsibility of the caller to determine if an Event should not be serialized? Then Event wouldn't need a custom implementation.
There was a problem hiding this comment.
Though I guess that may just trickle up another level, so maybe not worth it. :/
There was a problem hiding this comment.
Right, in principle maybe we could (serialization is ultimately exposed to the user) and move the logic up to the ChannelManager? I think we can do it as a followup, though, as the actual on-disk format should be the same (with just extra variants).
lightning/src/util/ser_macros.rs
Outdated
| /// variants stored directly. | ||
| /// The format is, for example | ||
| /// impl_writeable_tlv_based_enum!(EnumName, | ||
| /// (0, VariantName) => { (0, variant_field)}, {(1, variant_optional_field)}, {}; |
There was a problem hiding this comment.
For variant_optional_field, is this odd because it is optional? (i.e., an Option field may be even or odd but is typically odd)
There was a problem hiding this comment.
As an example, yes, I assume future-added optional fields will be odd (ie "if you dont know how to read this, ignore it"), but there's no written rule - the Option serialization section means it may or may not be there, but is separate from the "should old versions ignore or consider the object invalid" which is controlled by odd-or-even.
| {$(($reqtype: expr, $reqfield: ident)),* $(,)*}, | ||
| {$(($type: expr, $field: ident)),* $(,)*}, | ||
| {$(($vectype: expr, $vecfield: ident)),* $(,)*} | ||
| ),* $(,)*; |
There was a problem hiding this comment.
Regarding the separators, is there a need for trailing separators and also possibly different separators like semicolon? It's been a while, but when I wrote the macros for features I was able to separate using | but without needing to have one as a trailing separator.
There was a problem hiding this comment.
The optional trailing separators are so that you can write the list with trailing commas, ie without them you wouldn't be able to say (0, A) => {}, {}, {}, (2, B) => {}, {}, {}, } which is very natural when they're on separate lines, not that its a big deal either way. I'm ok swapping ; for | but I'm not sure that's really any more natural?
There was a problem hiding this comment.
Not suggesting to change the character. Just pointing out an example, but I found it a little odd that the three {} are not enclosed by another set of braces, which I think would allow using a comma instead? But maybe too verbose.
As discussed offline, an alternative we may consider is doing a syntax similar to protocol buffers, which would be more explicit, e.g.
optional foo = 1;
required bar = 2;
repeated baz = 4;
But no need to change in this PR if we'd prefer that.
There was a problem hiding this comment.
Hmm, yea, I think that would be cool, but then we'd want to do it for write_tlv_fields!() as well, which is a much larger change.
TheBlueMatt
force-pushed
the
2021-05-enum-tlvs
branch
from
a1cb7bb to
3ff1281
Compare
VecReadWrapper is only used in TLVs so there is no need to prepend a length before writing/reading the objects - we can instead simply read until we reach the end of the TLV stream.
These are used in the next commit(s).
TheBlueMatt
force-pushed
the
2021-05-enum-tlvs
branch
from
3ff1281 to
fd7845e
Compare
|
(trivially) rebased after merge of #933. |
| }, | ||
| &Event::PendingHTLCsForwardable { time_forwardable: _ } => { | ||
| 4u8.write(writer)?; | ||
| write_tlv_fields!(writer, {}, {}); |
There was a problem hiding this comment.
Checking my understanding: will this just write the fact that there's length = 0 bytes to read?
TLV-ize enum serialization and a few additional structs
Nothing super exciting here, just more TLVs inside enum variants with a new macro.