Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tlv: add library for new message/payload serialization format #3061

Merged
merged 9 commits into from
Aug 7, 2019

Conversation

cfromknecht
Copy link
Contributor

An implementation of lightning/bolts#607

tlv/stream.go Outdated Show resolved Hide resolved
@cfromknecht cfromknecht force-pushed the wire-tlv branch 5 times, most recently from 6ef4874 to 5fcad7f Compare June 1, 2019 02:05
@Roasbeef Roasbeef added P2 should be fixed if one has time spec wire protocol Encoding of messages for the communication between nodes labels Jun 18, 2019
@Roasbeef Roasbeef requested a review from joostjager July 2, 2019 02:26
watchtower/wtwire/wtwire.go Outdated Show resolved Hide resolved
watchtower/wtwire/wtwire.go Outdated Show resolved Hide resolved
watchtower/wtwire/wtwire.go Outdated Show resolved Hide resolved
tlv/stream_test.go Outdated Show resolved Hide resolved
tlv/stream.go Outdated Show resolved Hide resolved
tlv/stream_test.go Outdated Show resolved Hide resolved
tlv/stream_test.go Outdated Show resolved Hide resolved
tlv/stream_test.go Outdated Show resolved Hide resolved
tlv/stream_test.go Outdated Show resolved Hide resolved
tlv/stream_test.go Outdated Show resolved Hide resolved
@joostjager
Copy link
Contributor

Nice work in this PR. It shows careful engineering.

tlv/record.go Show resolved Hide resolved
tlv/record.go Outdated Show resolved Hide resolved
tlv/stream.go Outdated Show resolved Hide resolved
tlv/stream.go Outdated Show resolved Hide resolved
panic(ErrNoDecoder.Error())
}
if record.typ == math.MaxUint64 {
overflow = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the final type is what overflows, then it appears we won't panic at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the final type is math.MaxUint64 that's okay, so long as there isn't a number after it. overflow in this case signifies that a record hit math.MaxUint64, and any other records should be forbidden

tlv/stream.go Outdated
panic(ErrNoEncoder.Error())
}
if record.decoder == nil {
panic(ErrNoDecoder.Error())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on the context in which this is used within lnd itself, we may actually want to return an error instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, i'll change this to use Must paradigm employed by the go std library

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, got a better name than tlv.MustStream?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MustNewStream? lol

Either one works IMO. Must can can just wrap the regular and panic instead of panicing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the case where we only decode a stream and don't have an encoder available. Should record.encoder be set to a dummy encoder? (same question for encode-only)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I actually rely on the decoder being nil for my TLV-EOB PR within the router. When we receive a fully serialized TLV record stream over the RPC from the user (custom route for SendToRoute) I attach a shim encoder that just writes the raw bytes, and I have no need for a decoder since we'll never decode into the record. As a work around, I guess I can provide one that just does nothing instead since it's a special case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps instead we could make a nop encoder/decoder and automatically set them if no encoder or decoder is provided

// inspect each record that is parsed and check to see if it has a corresponding
// Record to facilitate deserialization of that field. If the record is unknown,
// the Stream will discard the record's bytes and proceed to the subsequent
// record.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the record is unknown,
// the Stream will discard the record's bytes and proceed to the subsequent
// record.

Is this subject to the even/odd rule? Also perhaps we should allow a "strict" stream that'll always error our rather than silently go to the next record to be decoded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes a strict mode would definitely be useful. the original version of this pr included that, but i removed it for now to simplify the diff

// We permit an io.EOF error only when reading the type byte which signals that
// the last record was read cleanly and we should stop parsing. All other io.EOF
// or io.ErrUnexpectedEOF errors are returned.
func (s *Stream) Decode(r io.Reader) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker for this PR, but this method would make for a good fuzz testing target. Even before the, we can assert compliance of our encoder by using testing/quick here to ensure we can always ingest what we produce, and don't inadvertently create any invalid streams.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooo good idea, maybe we can add it to the fuzzing harnesses that @Crypt-iQ has been working on

tlv/record.go Show resolved Hide resolved
tlv/primitive.go Show resolved Hide resolved
@Roasbeef
Copy link
Member

Reviewee bump!

Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, pretty stoked about finally starting to circle in on a sane serialization format! 🤓

tlv/record.go Outdated Show resolved Hide resolved
tlv/record.go Outdated Show resolved Hide resolved
tlv/varint.go Outdated Show resolved Hide resolved
tlv/varint.go Outdated Show resolved Hide resolved
tlv/stream.go Show resolved Hide resolved
tlv/stream.go Show resolved Hide resolved
// val is not a **btcec.PublicKey.
func EPubKey(w io.Writer, val interface{}, _ *[8]byte) error {
if pk, ok := val.(**btcec.PublicKey); ok {
_, err := w.Write((*pk).SerializeCompressed())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need methods for PublicKey, or could we just require the caller to use [33]byte?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test vectors in the spec require us to fail when reading an invalid point, so only parsing [33]byte isn't strict enough. arguably that check could be deferred to a higher level, but it this is the safest approach

tlv/primitive.go Show resolved Hide resolved
@cfromknecht cfromknecht force-pushed the wire-tlv branch 2 times, most recently from 8df5fa4 to 2058189 Compare August 7, 2019 02:04
@cfromknecht cfromknecht changed the title [tlv]: wire tlv proposal tlv: add library for new message/payload serialization format Aug 7, 2019
tlv/stream.go Outdated
panic(ErrNoEncoder.Error())
}
if record.decoder == nil {
panic(ErrNoDecoder.Error())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MustNewStream? lol

Either one works IMO. Must can can just wrap the regular and panic instead of panicing.

tlv/tlv_test.go Show resolved Hide resolved
// value was not minimally encoded.
var ErrTUintNotMinimal = errors.New("truncated uint not minimally encoded")

// numLeadingZeroBytes16 computes the number of leading zeros for a uint16.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there test vectors in the spec for these truncated variants?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the spec tests the 32 and 64 bit variants, tho not the uint16

Copy link
Contributor

@joostjager joostjager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a few non-blocking questions. LGTM

tlv/varint_test.go Outdated Show resolved Hide resolved
tlv/varint_test.go Outdated Show resolved Hide resolved
tlv/stream.go Outdated
panic(ErrNoEncoder.Error())
}
if record.decoder == nil {
panic(ErrNoDecoder.Error())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the case where we only decode a stream and don't have an encoder available. Should record.encoder be set to a dummy encoder? (same question for encode-only)


// BenchmarkEncodeCreateSession benchmarks encoding of the non-TLV
// CreateSession.
func BenchmarkEncodeCreateSession(t *testing.B) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran the benchmark and observed the difference between tlv and non-tlv. Pretty impressive. I was wondering, would the gap be much wider if the non-tlv case was optimized more? (mainly thinking about also reusing a buffer)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes there's a good bit of optimization that could be done to non-tlv messages. amongst other things, this was one of the optimizations included in btcsuite/btcd#1426. i don't think we'd see as significant of gains for lnwire messages since the primary speed up was due to reducing RTTs with the buffer pool, but this certainly helps remove unnecessary allocations and gc pressure

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, interesting. Yes, my main thinking was that for a fair comparison, both non-tlv and tlv should be as optimized as possible. But then there is also the "custom tlv encoder/decoder" method, where (unrolled) code is generated for a specific stream. From a theoretical pov it would be nice to see how all three of them compare. What the absolute minimum overhead of tlv is. Definitely not the most important question atm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, i too would be interested to see that. my goal here was to squeeze some low hanging fruit out of tlv, though as you said the actual overhead might be a little higher if we optimize the traditional encoding. there's also the question of even if those optimizations were possible, would they ever be deployed. if we think tlv will be the base encoding for our messages going forward, we may have to just accept whatever it is :P

tlv/truncated.go Show resolved Hide resolved
tlv/tlv_test.go Show resolved Hide resolved
tlv/tlv_test.go Outdated Show resolved Hide resolved
tlv/tlv_test.go Show resolved Hide resolved
tlv/varint_test.go Outdated Show resolved Hide resolved
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💿

This varint has the same serialization as the varint in btcd and
bitcoind, but has different behavior wrt returned errors. In order to
ensure the inner loop properly detects cleanly written records,
ReadVarInt will not only return EOF if it can't read the first byte, as
that means the reader has zero bytes left.

It also modifies the API to allow the caller to provided a static byte
array, which can be reused across all encoding and decoding and
increases performance.
This commit adds concrete encoding methods for primitive integral types.
When external libs need to create custom encoders, this allows them to
do so without incurring an extra allocation on the heap. Previously, the
need to pass a pointer to the integer using an interface{} would cause
the argument to escape, which we avoid by having them copied directly.
This commit adds the truncated integer encodings used in the
variable-size onion payloads. The amount and cltv delta both use the
truncated encoding to shave bytes in the overall size, and will likely
be used in the future for additional extensions where size is a
constraint.
@Roasbeef Roasbeef merged commit ea77ff9 into lightningnetwork:master Aug 7, 2019
@cfromknecht cfromknecht deleted the wire-tlv branch August 7, 2019 22:51
@cfromknecht cfromknecht added tlv P2 should be fixed if one has time and removed P2 should be fixed if one has time labels Aug 8, 2019
@Chinwendu20
Copy link
Contributor

Do you think it would be a good idea to include a boolean to the primitives?

@Chinwendu20 Chinwendu20 mentioned this pull request Oct 4, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 should be fixed if one has time spec tlv wire protocol Encoding of messages for the communication between nodes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants