Conversation
|
Thanks for this proposal @cfromknecht! It helps clarify some choices made in #603 by showing alternative choices. After reading both proposals, I still think that we should decide on a single TLV encoding, enforced everywhere (wire, onion, etc), that combines your proposal with rusty's. Having distinct encodings is error-prone and confusing and I don't think the gains justify it (but it might be because some gains aren't obvious to me, feel free to detail them more if you disagree). I'm in favor of keeping a I'm in favor of strict canonical encoding as you propose, but allowing multiple occurrences of the same type in a TLV stream. I don't think the sentinel value is needed at all (and I think the result of the IRC discussion was a consensus on that, is that correct?). I think it's still useful to keep the even/odd distinction to avoid feature flags bloat (I'm afraid we would need an explosion of feature flags which becomes more complex to track than the simple even/odd rule). I agree with you that the optimization for integer values is a layering violation and I prefer your suggestion to let it be defined by the TLV type itself. |
|
Thanks for the feedback @t-bast! I sincerely apologize in advance for the brevity of this reply, but it felt like a good time to do a brain dump from the last 6 months or so of research and explain the rationale behind certain aspects of the proposal that maybe haven't been discussed as much.
Yes that was the consensus, thanks to @cdecker @niftynei for helpful discussion! it's been removed in the fixups, along with the portions about retaining unknown values (which they convinced me is more of an implementation detail).
I'm not married to the fixed-size encoding for the length, using a varint was actually how this proposal was drafted a few months back :) It is nice that in the fixed size case, this limit is enforced directly rather than needing to enforce the constraint as an additional check on the parsed value, since we should only need to handle up to two-byte values for wire messages. I'm much more skeptical of making
Serialization under the wire format doesn't necessarily mean that the message is going to be encrypted, messages can be written to databases, over rpc connections, into message digests, etc. I agree tho that the overhead in moving from 2-byte length to varint (with realistic max of 65KB) is not so bad and can save space, however I don't think the same argument holds for Consider a Keep in mind we want to remain friendly to low-power devices with more constrained resources ;)
I'm with you on the strict canonical encoding! There are a multitude of reasons however why I'm strongly opposed to allowing duplicate fields, mostly performance related (i swear, this is the shortened list):
Compressing all repeated values into a single, unique record solves all of above the issues, with no real downsides AFAICT. Either the field is defined as a vector or it isn't, but those details remain in
On the wire, the sender can't just arbitrarily make fields required without knowing if the receiver supports it, doing so will result in a disconnect (since it'd be equivalent to failing to the parse the message). The receiver needs to opt-in to such fields before the sender can safely send them; we already have a mechanism for signaling this, what is the purpose in having the redundancy? For example, all new fields added to gossip messages I think need to be optional, otherwise they won't propagate through older nodes. Having the even/odd rule there, or any wire message really, would only serve to halve the range of valid Onion TLV is different, since we can't rely on connection-level feature bit negotiation, whether we have the node's latest announcement, or if the node is still running the same software/configuration as it was when the invoice or announcement was created. At the same time, failure to parse a required onion TLV record only results in a failed HTLC, and not connection-level instability. I support having even/odd rule in onion, but I don't think the rule needs to be inherited by all TLV namespaces. IMO it should be applied as an additional constraint in the onion parsing. In my mind, the distinction is purely: // Standard TLV parsing for virtually everything
var parser tlv.Parser
if err := parser.Decode(r); err != nil {
return err
}
// For onion TLV, apply even/odd rule as additional constraint
if parser.FoundUnknownRequiredFields() {
return tlv.ErrUnknownRequiredFields
}I think this gets us towards a more generic, unified TLV implementation that allows certain applications to apply restrictions as necessary, rather than restricting all other use cases for a single known instance where the even/odd rule is indeed very helpful.
Yes, I concur that it would be nice to agree on one unified approach, but want to be clear that I don't necessarily want distinct formats. Though if the tradeoffs can't be reconciled entirely, I still think it's possible to agree on a set of fundamental requirements that permit a high degree of code reuse between the two. TL;DRWith all that in mind and attempting to incorporate the pertinent feedback, i've updated the proposal to:
I thinkkk this captures the majority of the feedback that's been given over the past few weeks. I would then propose that we define the onion tlv format solely by adding a check for unknown required fields to the wire tlv requirements. Let me know if I've blatantly missed/overlooked anything, further feedback is greatly appreciated! Hopefully that also further elaborates on some distinctions I see in wire vs onion tlv, or perhaps shows my ignorance as to why they should be treated the same. To anyone who read the whole thing, I owe you a beer :) Aside: is it worth having a separate BOLT just for the TLV spec? I'm starting to think it might be helpful to have a place where we define useful encodings, e.g. compressed integers, so we can reference a single location for common |
|
That's great, thanks a lot for this detailed post, it's very helpful (especially for newcomers such as myself). I'll hold you to that beer when we meet IRL ;) Your argument for removing duplicate occurrences of a TLV type makes a lot of sense. As you suggest, it's a lot cleaner (and simplifies a lot the implementation) to let the type itself define whether it's an array or not. I'm fully on board with that (and the strict canonical encoding). I've thought more about the feature flags vs even/odd rule, and reading your comment I now agree with you that the wire encoding doesn't need the even/odd rule. We will always need feature flags to know that the other peer understand our required TLV types before sending a message. And once we have such feature flags, we should consider all the other TLV types in the message as optional. It's especially obvious when we think about gossip re-transmission (staggered broadcast): if Bob re-transmits a message from Alice to his peers, for example to Carol, Carol should ignore every TLV type she doesn't understand (otherwise Alice could indirectly force a connection-close between Bob and Carol which is undesirable). We don't want Bob to have to compare each message's inner TLV types to each of his peers' feature flags to know if it's safe or not to re-transmit the message. I would even argue that onion TLV doesn't need the even/odd rule either. Since we want to maximize payment path success, we will use channel feature flags to decide whether to include a peer in a payment path or not (because it's risky and costly to blindly include a node that would fail to parse a TLV type and thus fail the HTLC). So we know that this node understands all our required TLV types, and we can safely treat all other TLV types as optional (for example optional routing hints for next trampoline hop). One nice benefit of removing the even/odd rule from onion TLV is that there is now no differences between onion and wire TLV (yay single encoding!). Do note though that I think that the even/odd rule is useful for top-level messages (as defined in Bolt #1). But for TLV types I think the even/odd rule is unnecessary because all unknown TLV types should be safely ignored (and never fail a connection). So we should be in agreement with using feature flags instead of the even/odd rule for TLV. However I'm still not convinced about I don't have a strong opinion on putting the TLV proposal in its own BOLT, I think it's a good idea but I'm most certainly not the most suited person to judge that. I feel that we're making a lot of progress and we're getting close :) Phew that ended up being a long comment too, I'll have to buy the second round of beers to our readers! |
There was a problem hiding this comment.
Few nits on wording. Otherwise looks good.
Here's a quick list of the differences between this and #603:
|
Thank you @niftynei for the review and @t-bast for further input 🎉
Indeed, though I should note that we don't need a feature bit if the field is truly optional, it only needs to be defined if we wish to make it required. My hope is that this wouldn't create feature bits like Since they are optional, the sender is free to write it, but everyone can save on bandwidth if the field is serialized conditionally on the peer's support for that feature.
Yeah I'm not totally certain in either direction, I suppose one could even make a case for all onion TLV fields to be required. But deferring additional constraints to a higher-level means we can retroactively add these constraints where necessary. Perhaps those will be identified as we move forward with defining the MPP onion types.
Which ones exactly? Since
A wise professor once told me that if you're designing a protocol and think a certain limit is enough, double it—in reference to IPv4 address space lol. I left this out of the proposal, but my original idea was strip out the 4 and 8 bit encodings from the If we use this for both The implementation obviously is pretty trivial, just its mostly a copy-pasta of
Sure feels like it!
Same here, as I don't think we can really move forward conclusively without his input on the tradeoffs :)
Looking forward to it! I suppose the length of this message warrants a third round 🍻 |
|
One other comment regarding using a varint type: encountering an EOF while parsing the type doesn't necessarily mean the stream is well-formed like it would with 1-byte types. We’d need to modify the language to consider an EOF well-formed only if zero bytes were read. |
I completely agree: a feature flag should indicate support for a bunch of required TLV types (in a given context) - for example
😆 I'm not very clear on your proposal for the
How do you know you finished parsing Thanks for all those suggestions and the continued discussion :) |
|
In the process of integrating the current wire TLV with @Roasbeef’s multi-frame implementation of #593, we encountered some interesting events that reminded me of some other reasons for having a sentinel. When the bytes containing a This happens when parsing payloads in #593, since only complete frames are interpreted without specifying the number of used bytes in the payload. This leaves us with two options:
Writing the length will require prepending 1 or 3 bytes to the payload. On the other hand, using a sentinel type will require at most 1 byte, let me explain. The original proposal serialized the sentinel’s length, which I’ve realized now is not necessary. We can simply stop after parsing a sentinel type, making the sentinel smaller. For either 8-bit or varint types, we’d choose a value that only requires 1 byte. The original proposal also made the sentinel record optional, which means that when the payload fits exactly in the provided frames, it can simply be omitted. Thus, when the When it doesn’t fit exactly, we use one extra byte, but at that point the frame has already been consumed and the remainder of the bytes can’t be reclaimed. In either case, no additional frames will ever be consumed using a sentinel value. We can simplify the encoding procedure even more, by defining the sentinel value as 0x00. Observe that after encoding the In addition to the above, using a sentinel value offers another significant performance benefit over explicitly writing the length, in that the Given that it’s optional, is it worth sharing this between wire and onion TLV? I don’t see how it hurts, and IMO would make the base TLV requirements more flexible as a whole |
f6126b4 to
2c69fc2
Compare
|
I agree that for non-onion messages even/odd could be valuable because it provides more flexibility and easier backwards-compatibility. But in the case of the onion I feel like this won't be used in practice. You really want to avoid seeing your payment fail because someone in the path didn't understand a required field in your onion. So in my opinion you will always check the feature bits of everyone on the payment path to make sure they understand the required types that you're including in your onion, which makes all other types inherently optional. Do you see a use-case where that wouldn't be true? I can understand however that you might want to make this rule apply to the onion as well because it becomes messy to have this rule in some places but not everywhere. For the sake of clarity / avoiding spec complexity, I can agree to that (and if we have a
I think it's wasteful to add potentially 3 bytes (or more) for the total length of the TLV stream inside the onion. Couldn't we simply say that the end of an onion TLV stream needs to use padding with only |
7dec534 to
73b8a2d
Compare
|
I've updated the proposal from the result of our last meeting and the discussion between rusty and I on irc. A summary of the current proposal:
|
|
I also updated the PoC implementation here: lightningnetwork/lnd#3061 Depending on how your decoder is implemented, one gotcha with the latest variant is that you may need to handle the case where |
|
@t-bast pushed a fixup which i think improves the clarity, lmk what you think |
rustyrussell
left a comment
There was a problem hiding this comment.
Excellent, and nice and canonical with the spec wording, too.
|
catching up after being out Monday, quick question/clarification about how the 'length of the TLV-stream' is represented in wire messages* -- will a tlv-stream be prepended by the total length of the TLV-stream portion of the message, or is there an implicit "stream-like" quality to it in that 'any additional bytes on this message are assumed to be TLV'? in other words would wire messages have: or *via the IRC meeting notes from Monday, it seems like onion formats will have to specify the frame length, so they're already doing |
niftynei
left a comment
There was a problem hiding this comment.
Small nits; clarifications.
01-messaging.md
Outdated
| The sending node: | ||
| - MUST order `tlv_record`s in a `tlv_stream` by monotonically-increasing `type`. | ||
| - MUST minimally encode `type` and `length`. | ||
| - SHOULD NOT use the varbytes encoding for `value`s containing byte arrays. |
There was a problem hiding this comment.
this is the first time we're using varbytes in the spec, should we provide a definition (or at least a link to a definition)?
There was a problem hiding this comment.
ended up removing the mention of varbytes in favor of rusty's language from the irc meeting, which says not to use redundant, variable-length encodings
| The receiving node: | ||
| - if zero bytes remain before parsing a `type`: | ||
| - MUST stop parsing the `tlv_stream`. | ||
| - if a `type` or `length` is not minimally encoded: |
There was a problem hiding this comment.
clarification: is minimally encoded referring to their being a varint type, i.e. you should encode '0x01' as '0x01', not '0xfe00000001'?
There was a problem hiding this comment.
@niftynei correct, i would assume most impls of compact size already check this, tho i'm not positive
There was a problem hiding this comment.
As an example, btcd's wire package includes checks to ensure encoded integers are canonical: https://github.com/btcsuite/btcd/blob/master/wire/common.go#L491
Yes, the assumption rn is that there is an implicit The critical thing is that the decoder knows where it should stop reading. In the case of the onion, we prepend the length directly to the |
|
@cfromknecht thanks for the clarification |
|
Hey @cfromknecht do you think we should add test vectors to this PR? |
|
Hey @t-bast! Yes I think that’s a good idea, I’ve been meaning to convert some of the tests in the PoC to json, but haven’t had time. If you post yours here I can add them and extend with any others I think of |
|
I don't know what the expected JSON format would be, but here is the content of my test suite. If you can provide the JSON format, I can convert some of them to JSON and add a commit to this PR. Some of the test cases might not be convertible to a JSON spec imo because they depend on context for the stream parser (for example to test that a stream containing an unknown even type should trigger a failure). Because of the need to configure parsers with sample TLV types, I find it a bit hard to spec the test vectors...if you think of a better format let me know! Test VectorsKnown namespaces and TLVsThe following tests assume that two separate TLV namespaces exist: The
The
TLV decoding failureThe following TLV in namespace
TLV decoding successThe following TLV in namespace
TLV stream decoding failureThe following TLV streams in namespace
TLV stream decoding successThe following TLV streams in namespace
|
TLV (tag-length-value) types and TLV streams have been defined in the following spec PR: lightning/bolts#607 New Lightning Messages should use TLV extensively instead of ad-hoc per-message encoding. This also allows ignoring unknown odd TLV types, which lets implementers safely test new features on mainnet without impacting legacy nodes. It also allows type re-use which speeds up new features development. Also cleaned-up and refactored common codecs.
| - MUST minimally encode `type` and `length`. | ||
| - SHOULD NOT use redundant, variable-length encodings in a `tlv_record`. | ||
|
|
||
| The receiving node: |
There was a problem hiding this comment.
What about rejecting redundant types in the stream?
There was a problem hiding this comment.
I guess this is covered by the "strictly increasing" requirement.
These are based on @t-bast's vectors from lightning#607, with a few more cases. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
These are based on @t-bast's vectors from lightning#607, with a few more cases. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
These are based on @t-bast's vectors from lightning#607, with a few more cases. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
These are based on @t-bast's vectors from lightning#607, with a few more cases: 1. Explicitly test encodings for 253, 254 and 255. 2. Use BigSize and make sure tests break badly if endian parsing is wrong.' 3. Test wrap-around of type encodings in stream. Many thanks to @t-bast and @cfromknecht for their contributions and testing Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
These are based on @t-bast's vectors from #607, with a few more cases: 1. Explicitly test encodings for 253, 254 and 255. 2. Use BigSize and make sure tests break badly if endian parsing is wrong.' 3. Test wrap-around of type encodings in stream. Many thanks to @t-bast and @cfromknecht for their contributions and testing Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This proposal outlines a TLV format intended for wire messages. It contains additional considerations for gossip messages that would require maintaining the ability to verify signatures over unknown fields, and the ability to propagate those messages to peers.This proposal makes different tradeoffs from #603 in terms of:requiring strict canonical encodingfixed-size length encodingsupporting reserialization of unknown recordsusing 255 as the sentinel type identifierremoving even/odd requirement from the parsing logic, favoring negotiation via feature bits or application of such constraints external to the fundamental parsing requirementsAfter significant feedback and discussion, the proposal now entails:
typeandlengthboth use the bitcoin varint format, minimally-encodedtypeonlyThe proposal is meant to encompass a single TLV format for use in wire and onion payloads.
Reference implementation: lightningnetwork/lnd#3061