lnwire: create new ExtraOpaqueData type for parsing TLV extensions#3966
lnwire: create new ExtraOpaqueData type for parsing TLV extensions#3966Roasbeef wants to merge 43 commits into
Conversation
cfromknecht
left a comment
cfromknecht
left a comment
There was a problem hiding this comment.
nice pr, looks pretty good on first pass! main comment is related to leveraging the codec to make the diff smaller. also would be a good opportunity to remove the max message size method from the message interface, since we now allow all messages to be up to 65KB.
Roasbeef
force-pushed
the
wire-uniform-tlv
branch
2 times, most recently
from
6851ed3 to
ab3b5b5
Compare
|
I ran into a snag with this PR: in certain places we store a message, but then pack in additional data after the wire message, like the commitment diffs we store for the state machine. The current logic here will attempt to read all data after the base message as extra TLV data, so it ends up consuming all the extra bytes, leading to an EOF error when we attempt to read information serialized after the wire message. A few solutions come to mind:
Thoughts? I favor the 1st option myself. Note that certain unit tests in this PR won't pass until we resolve this issue, as they fail with EOF errors. To make things easier, we'd create a If we opt to go with #1, I'll make a new PR for this logic, as to not increase the size of this PR any further. |
|
|
Trip down memory lane |
Roasbeef
force-pushed
the
wire-uniform-tlv
branch
from
ab3b5b5 to
f1ee009
Compare
Roasbeef
added
enhancement
Roasbeef
force-pushed
the
wire-uniform-tlv
branch
from
f1ee009 to
47d5811
Compare
|
Here's a combined branch with the migration PR to show everything passes with both applied: https://github.com/Roasbeef/lnd/pull/new/combined-wire-tlv The tests won't pass for this PR in isolation, as since the new wire parsing tries to read until EOF, it can read any of the HTLC in the commitment diff as an example. |
|
Pushed up a new rebased version of https://github.com/Roasbeef/lnd/pull/new/combined-wire-tlv |
In order to prep for allowing TLV extensions for the `ReplyChannelRange` and `QueryChannelRange` messages, we'll need to remove the struct embedding as is. If we don't remove this, then we'll attempt to decode TLV extensions from both the embedded and outer struct. All relevant call sites have been updated to reflect this minor change.
In this commit, we convert the delivery address in the open and accept channel methods to be a TLV type. This works as an "empty" delivery address is encoded using a two zero bytes (uint16 length zero), and a tlv type of 0 is encoded in the same manner (byte for type, byte for zero length). This change allows us to easily extend these messages in the future, in a uniform manner.
In this commit, we update a series of unit tests in the code base to now pass due to the new wire message encode/decode logic. In many instances, we'll now manually set the extra bytes to an empty byte slice to avoid comparisons that fail due to one message having an empty byte slice and the other having a nil pointer.
…rsion for TLV In this commit, we update all the existing methods for encoding and decoding in the package to also observe the currently unused `pver` (protocol version) argument. With this change, we can enforce the new "TLV everywhere" rules for all the new code, while leaving the existing migrations untouched (using the legacy version). Otherwise, a legacy migration would possibly read too many bytes (as the ExtraOpaqueBytes reads until EOF), causing it to corrupt existing data on disk. The Encode/Decode methods of the ExtraOpaqueData struct will now observe the new `pver` value, and return early (not reading/writing) the extra bytes based on this value.
In this commit, update the encode/decode code concerning wire messages in the existing migrations to force them to specify that they want the legacy protocol version which doesn't attempt to read the extra bytes for all messages other than gossip messages.
|
This PR is going away now (in favor of the combined version), but I pushed up a rebased version for posterity. |
Roasbeef
force-pushed
the
wire-uniform-tlv
branch
from
a90edd0 to
afabedd
Compare
6 participants
In this Pr, we create a new
ExtraOpaqueDatabased on the field with the same name that's present in all the announcement related messages. In later commits, we'll embed this new type in each message, so we'll have a generic way to add/parse TLV extensions from messages.We then convert all the current known messages to include this new field, and parse it accordignly. The new
ExtraOpaqueDataalso comes with some propsective helper methods for TLV encoding/decoding. I envision these will be used either within the top-levelEncode/Decodemethods, or by callers to populate new (optional) pointer fields that get added in the wire messages similar to the structs inhtlcswitch/hop.