Simplify Message Serialization and Parse TLV Suffix #1068
Merged
Conversation
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
TheBlueMatt
force-pushed
the
2021-09-ser-cleanup
branch
from
8de4670 to
f3a0830
Compare
Codecov Report
@@ Coverage Diff @@
## main #1068 +/- ##
==========================================
+ Coverage 90.87% 92.39% +1.52%
==========================================
Files 65 65
Lines 33229 43459 +10230
==========================================
+ Hits 30196 40153 +9957
- Misses 3033 3306 +273
Continue to review full report at Codecov.
|
TheBlueMatt
force-pushed
the
2021-09-ser-cleanup
branch
5 times, most recently
from
678c3b7 to
030c25e
Compare
TheBlueMatt
force-pushed
the
2021-09-ser-cleanup
branch
from
030c25e to
27c6ee2
Compare
valentinewallace
approved these changes
Sep 9, 2021
TheBlueMatt
force-pushed
the
2021-09-ser-cleanup
branch
from
7acf5e3 to
41cf0c2
Compare
|
Reordered the commits a bit and updated writer docs. Note that at this point we should probably remove writer, but I'll leave that as a followup huge-diff change. |
valentinewallace
approved these changes
Sep 18, 2021
In order to avoid significant malloc traffic, messages previously explicitly stated their serialized length allowing for Vec preallocation during the message serialization pipeline. This added some amount of complexity in the serialization code, but did avoid some realloc() calls. Instead, here, we drop all the complexity in favor of a fixed 2KiB buffer for all message serialization. This should not only be simpler with a similar reduction in realloc() traffic, but also may reduce heap fragmentation by allocating identically-sized buffers more often.
Going forward, all lightning messages have a TLV stream suffix, allowing new fields to be added as needed. In the P2P protocol, messages have an explicit length, so there is no implied length in the TLV stream itself. HTLCFailureMsg enum variants have messages in them, but without a size prefix or any explicit end. Thus, if a HTLCFailureMsg is read as a part of a ChannelManager, with a TLV stream at the end, there is no way to differentiate between the end of the message and the next field(s) in the ChannelManager. Here we add two new variant values for HTLCFailureMsg variants in the read path, allowing us to switch to the new values if/when we add new TLV fields in UpdateFailHTLC or UpdateFailMalformedHTLC so that older versions can still read the new TLV fields.
The network serialization format for all messages was changed some time ago to include a TLV suffix for all messages, however we never bothered to implement it as there isn't a lot of use validating a TLV stream with nothing to do with it. However, messages are increasingly utilizing the TLV suffix feature, and there are some compatibility concerns with messages written as a part of other structs having their format changed (see previous commit). Thus, here we go ahead and convert most message serialization to a new macro which includes a TLV suffix after a series of fields, simplifying several serialization implementations in the process.
TheBlueMatt
force-pushed
the
2021-09-ser-cleanup
branch
from
41cf0c2 to
997dc5f
Compare
|
Squashed with no further changes, will land after CI. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
See individual commits