Extended Queries: use TLV format for optional data#1072
Merged
Conversation
Optional query extensions now use TLV instead of a custom format. Flags are encoded as varint instead of bytes as originally proposed. With the current proposal they will all fit on a single byte, but will be much easier to extends this way.
Codecov Report
@@ Coverage Diff @@
## extended-queries-optional #1072 +/- ##
=============================================================
+ Coverage 82.76% 82.76% +<.01%
=============================================================
Files 100 103 +3
Lines 7727 7776 +49
Branches 306 304 -2
=============================================================
+ Hits 6395 6436 +41
- Misses 1332 1340 +8
|
pm47
requested changes
Jul 24, 2019
Member
pm47
left a comment
pm47
left a comment
There was a problem hiding this comment.
This is just a first pass review. PR needs a merge from master, and I don't think the TLV part is properly implemented.
We add one new class for each TLV type, with specific TLV types, and encapsulate codecs.
TLVs that extend regular LN messages can be represented as a TlvStream and not an Option[TlvStream] since we don't need to explicitely terminate the stream (either by preprending its length or using a specific terminator) as we do in Onion TLVs. No TLVs simply means that the TLV stream is empty.
Checksums in ReplyChannelRange now have the same encoding as short channel ids and timestamps: one byte for the encoding type (uncompressed or zlib) followed by encoded data.
If a have a TLV stream of type MyTLV which is a subtype of TLV, and MyTLV1 and MYTLV2 are both subtypes of MyTLV then we can use stream.get[MyTLV1] to get the TLV record of type MYTLV1 (if any) in our TLV stream.
Checksums are just transmitted as a raw array, with optional compression as it would be useless here.
We will use them on mainnet as soon as lightning/bolts#557 has been merged.
pm47
requested changes
Aug 22, 2019
We remove the ugly and inefficient zipWithIndex we had before
pm47
reviewed
Aug 22, 2019
| socksProxy_opt: Option[Socks5ProxyParams], | ||
| maxPaymentAttempts: Int) { | ||
|
|
||
| require(feeBase <= MilliSatoshi(0xFFFFFFFFL)) |
Member
There was a problem hiding this comment.
This should be in NodeParams.makeNodeParams for consistency, with a human understandable message (and also a comment explaining why we have to cap the value)
pm47
approved these changes
Aug 22, 2019
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
4 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.
Optional query extensions now use TLV instead of a custom format.
Flags are encoded as varint instead of bytes as originally proposed. With the current proposal they will all fit on a single byte, but will be much easier to extends this way.