multi: the great tlv wire migration#4747
Closed
Roasbeef wants to merge 38 commits into
Closed
Conversation
With this change, errors from migrations will have the proper local line number.
In this commit, we modify the way we write wire messages across the entire database. We'll now ensure that we always write wire messages with a length prefix. We update the `codec.go` file to always write a 2 byte length prefix, this affects the way we write the `CommitDiff` and `LogUpdates` struct to disk. We also need to migrate the network results bucket in the switch as it includes a wire message without a length prefix.
In this commit, we create a new `ExtraOpaqueData` based 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.
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 is requires to ensure the migration is able to run without issue as the stored contents won't contain any optional TLV data.
Roasbeef
requested review from
cfromknecht,
halseth and
wpaulino
as code owners
Roasbeef
added
database
halseth
suggested changes
Nov 5, 2020
Contributor
halseth
left a comment
halseth
left a comment
There was a problem hiding this comment.
Great PR, not too bad to review even though it is large.
My main concern is the usage of lnwire within the old migrations. I would be more confident if we just copied the old code we need in there before we start changing the behavior oflnwire
| // openChan -> nodeID -> chanPoint | ||
| // | ||
| // TODO(roasbeef): flesh out comment | ||
| openChannelBucket = []byte("open-chan-bucket") |
Contributor
There was a problem hiding this comment.
move keys to the migration itself.
|
|
||
| closedChanBucket := tx.ReadWriteBucket(closedChannelBucket) | ||
| if closedChannelBucket == nil { | ||
| return fmt.Errorf("no closed channels found") |
Contributor
There was a problem hiding this comment.
could this happen? Maybe the error message should be changed to say the bucket was not found.
| return err | ||
| } | ||
|
|
||
| msgLen := uint16(len(msgBuf.Bytes())) |
Contributor
There was a problem hiding this comment.
use uint32 or uint64 to allow longer messages in the future?
| func NewRevokeAndAck() *RevokeAndAck { | ||
| return &RevokeAndAck{} | ||
| return &RevokeAndAck{ | ||
| ExtraData: make([]byte, 0), |
Contributor
There was a problem hiding this comment.
why this change? Not done for other messages.
| return &Init{ | ||
| GlobalFeatures: gf, | ||
| Features: f, | ||
| ExtraData: make([]byte, 0), |
Contributor
There was a problem hiding this comment.
same question, why is this done?
|
|
||
| // ProtocolVersionTLV is the current modern protocol version. When | ||
| // reading/writing messages with this version, decoding will continue | ||
| // until the entire payload has been ready. When writing with this |
| func (e *ExtraOpaqueData) Decode(r io.Reader, pver uint32) error { | ||
| // Only if we're using the modern protocl version will we attempt to | ||
| // keep on decoding past the end of the "main message". | ||
| if pver != ProtocolVersionTLV { |
Contributor
There was a problem hiding this comment.
Can you point to the place in the migrations where this was a problem? I'm trying to understand why this is neccassary.
This also restates that the migrations shouldn't be depending on code that might change from underneath them. I would prefer rather than this change, copying the legacy lnwire encoding into the migrations, so we won't have to worry about this later.
| // Serialize the message with its wire encoding. | ||
| var b bytes.Buffer | ||
| if _, err := lnwire.WriteMessage(&b, msg, 0); err != nil { | ||
| _, err := lnwire.WriteMessage(&b, msg, lnwire.ProtocolVersionTLV) |
| // set of extra opaque data. | ||
| func (e *ExtraOpaqueData) Decode(r io.Reader) error { | ||
| func (e *ExtraOpaqueData) Decode(r io.Reader, pver uint32) error { | ||
| // Only if we're using the modern protocl version will we attempt to |
| // Only if we're using the modern protocl version will we attempt to | ||
| // keep on decoding past the end of the "main message". | ||
| if pver != ProtocolVersionTLV { | ||
| return nil |
Contributor
There was a problem hiding this comment.
also, doesn't this change the behavior for the legacy protocol? Before this PR the messages that had ExtraOpaqueData defined would be read to the end, while after this we will stop reading (so if migrations are done for these messages, that data won't be copied).
Contributor
|
Replaced by #4923 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is a combination of #4099 and #3966 as they're interdependent and can't be merged in isolation yet leave master in a clean slate. The only new commit here is the last one that ensures the prior added migration uses the new protocol version to gate the "proper" behavior w.r.t always reading/writing the extra bytes for each migration.
Putting this up now so it can start building, there're a few relevant comments from the prior PRs that I'll address as fix up commits here. I know it looks large, but the bulk of the commits are mostly small changes in the
lnwirepackage. If requested, I can squash in those 30 or so commits into a single one if it actually makes things easier to review.First PR Description
In this Pr, we create a new ExtraOpaqueData based 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 ExtraOpaqueData also comes with some propsective helper methods for TLV encoding/decoding. I envision these will be used either within the top-level Encode/Decode methods, or by callers to populate new (optional) pointer fields that get added in the wire messages similar to the structs in htlcswitch/hop.
Second PR Description
In this commit, we modify the way we write the CommitSIg in certain locations to match the way it's encoded on the wire. With this change, we'll now properly add a length prefix in front of the message, meaning we'll be able to read the extra data included (if present), and still be able to serialize content after this message.
We add a simple migration to migrate all the current pending commitments, along with a test. We opt to do the simple enclose-copy method for the migration, which results in a larger diff, but IMO an easier to understand migration test.