Fix Router Serialization Roundtrip #523
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
2020-02-router-ser-fix
branch
from
566bd25 to
0a18f6f
Compare
TheBlueMatt
force-pushed
the
2020-02-router-ser-fix
branch
from
0a18f6f to
409ce89
Compare
ariard
reviewed
Mar 3, 2020
TheBlueMatt
force-pushed
the
2020-02-router-ser-fix
branch
from
409ce89 to
4c36578
Compare
|
Addressed both comments, also neatly testing the router outbound initial syncs. Or, at least, that they terminate. |
TheBlueMatt
force-pushed
the
2020-02-router-ser-fix
branch
from
4c36578 to
636d96f
Compare
|
ACK 636d96f, I compiled on my machine and the code makes sense to me. |
This provides a simple wrapper for deserializing right into an Arc<ChannelManager>, which improves UX a tiny bit when working with SimpleArcChannelManager types.
This makes Readable symmetric with Writeable and makes sense - something which is Readable should be Readable for any stream which implements std::io::Read, not only for a stream type it decides on. This solves some lifetime-compatibility issues in trying to read() from a LengthLimitingReader in arbitrary Readable impls.
This is a cheap way to fix an error in Router serialization roundtrip due to us calling read_to_end during the read of channel/node announcement/updates. During normal message reading, we only have limited bytes to read (specifically the message buffer) so this is fine, however when we read them inside Router, we have more data from other fields of the Router available as well. Thus, we end up reading the entire rest of the Router into one message field, and failing to deserialize. Because such fields are always stored in Option<>s, we can simply use a LengthLimitingStream in the Option<> serialization format and make only the correct number of bytes available. By using a variable-length integer for the new field, we avoid wasting space compared to the existing serialization format.
This tests Router serialization round-trip at the end of each functional test in the same way we do ChannelMonitors and ChannelManagers to catch any cases where we were able to get into a state which would have prevented reading a Router back off disk. We further walk all of the announcements which both the original and deserialized Routers would send to peers requesting initial sync to ensure they match.
TheBlueMatt
force-pushed
the
2020-02-router-ser-fix
branch
from
636d96f to
5f3986d
Compare
|
Rebased on #534 without changes. |
Codecov Report
@@ Coverage Diff @@
## master #523 +/- ##
==========================================
+ Coverage 89.73% 89.97% +0.24%
==========================================
Files 33 33
Lines 19118 19150 +32
==========================================
+ Hits 17156 17231 +75
+ Misses 1962 1919 -43
Continue to review full report at Codecov.
|
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.
In channel/node announcement/updates, we will read_to_end on the input stream to ensure we store any excess bytes at the end. This is great, until we try to go read a Router that contains a bunch of these messages in them, causing us to read data that is unrelated into the messages. To fix, we modify the Option format to write out the length of the object, and then use a LengthLimitingReader to read the T.
This requires we change the Readable trait to have a template in the read function instead of the trait due to lifetime issues, but this change stands on its own as it makes our Readable/Writeable traits symmetric.