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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

bolt04: Multi-frame sphinx onion routing proposal #593

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@cdecker
Copy link
Collaborator

commented Mar 29, 2019

Following the discussion on the mailing list, and the two implementations in Go and C, I went ahead and drafted the proposed specification changes.

Aside from some copy-editing and reformatting the changes include:

  • The introduction of a few new terms:
    • frame as a 65 byte size unit that can be used to serialize data in (and the associated FRAME_SIZE constant to make it easier to spot).
    • num_frames_and_realm for the former realm byte which is being reused to specify both type/realm as well as how many additional frames should be consumed by the processing node.
    • raw_payload which is the serialized information that we want to communicate to the processing node in the first place.
    • hop_payload to refer to the the payload composed of multiple frames, that includes the num_frames_and_realm byte, the raw_payload, any padding, and the HMAC for the next hop.
  • Splitting the raw_payload parsing from the pure data extraction from the hop_payload, since we will soon have multiple ways to parse it once the TLV proposal is live (and I'd like to retire the v1 hop_data format in favor of using the more flexible TLV in the onion).
  • Creating the accepting and forwarding section, since this is no longer tightly coupled to the parsing of the hop_data format.
  • The technical description and implementation have been updated to match the Golang implementation I wrote, so that it is in sync (it also cuts down on the number of code lines, and adds a lot more comments, so it should be much clearer to follow)
  • Moved the test vectors into separate JSON documents.This has a couple of advantages:
    • It saved me quite a bit of ink while proof-reading the draft 馃槈
    • It allows programmatic verification of the test-vectors. I wrote the tool for Golang and C

Notice that this proposal is perfectly backwards compatible, given that we just rename some of the concepts that are already present and make the two shifts variable.

The reason I'd like to get the ball rolling again, is that there hasn't been a lot of pushback, and there are now quite a few nice features that rely on the ability of transporting more information in the onion, including rendezvous routing, spontaneous payments, simple multi-part payments, and trampoline payments (outsourcing route finding to some other node).

Finally these are the PRs that implement this proposal:

Closes #586

@cdecker cdecker added this to the v1.1 milestone Mar 29, 2019

@cdecker cdecker referenced this pull request Mar 29, 2019

Open

Test Vectors in JSON #576

Therefore the number of frames allocated to the current hop is given by `num_frames = (num_frames_and_realm >> 4) + 1`.
For simplification we will use `hop_payload_len` to refer to `num_frames * FRAME_SIZE`.

In order to have sufficient space to serialized the `raw_payload` into the `hop_payload` while minimizing the number of used frames the number of frames used for a single `hop_payload` MUST be equal to

This comment has been minimized.

Copy link
@t-bast

t-bast Apr 24, 2019

nit: tiny typo (serialized should be serialize)

1. type: `per_hop` (for `realm` 0)
1. type: `hop_payload`
2. data:
* [`1`: `num_frames_and_realm`]

This comment has been minimized.

Copy link
@t-bast

t-bast Apr 24, 2019

Are there use-cases where one realm could use multiple frame sizes? And if that's the case, how would the application interpret that? It seems to me that the application would always need to map num_frames + realm to an object of a specific size.

I saw in another PR a proposal to rename this field packet_type and I expected each packet type to have a constant, fully-specified length right? If that's the case we could still rename this field to packet_type (instead of num_frames_and_realm), keep it a full byte, and the packet value would implicitly let the application know the number of frames (and the raw_payload_len).

This comment has been minimized.

Copy link
@t-bast

t-bast Apr 24, 2019

Maybe I'm mislead because I haven't dived enough in the proposals to integrate TLV. Did you make it this way to let TLV handle the definition of the raw_payload? But even then, it seems that encoding the num_frames in this field would be redundant with the length encoded in the TLV...

This comment has been minimized.

Copy link
@cdecker

cdecker Apr 25, 2019

Author Collaborator

It is indeed intentional that we have two fields in a single byte. num_frames are the MSB 4 bits, whereas the realm, or packet_type as it is called elsewhere, consists of the lower 4 bits. The reason why we are handling it this way is that the former realm byte is the only byte that has contained metadata about the payload so far, and we keep it this way for simplicity. It still allows for a total of 16 realms/packet types/payload types and up to 15 additional frames. The latter may appear limiting, however remember that the onion construction is limited to 20 frames in total anyway, so being able to use 16 (1 default + 15 additional ones) for a single payload seems extremely nice to me 馃槈

The reason we don't defer the determination of the length to the TLV payload itself, is that it'd be a layering violation (the payload would be telling us its length, and therefore how many frames to use in the onion which is the payload's transport layer). It'd also require us to parse the content before being able to forward the packet, which is a timing side-channel on the content of the payload.

This comment has been minimized.

Copy link
@t-bast

t-bast Apr 26, 2019

Thanks for the detailed explanation.
The layer violation makes a lot of sense, I get why you made that choice now.
I'm completely fine with the proposal then, I believe the simplicity and nice layering separation make up for the small amount of potentially wasted bytes (in the padding).

This comment has been minimized.

Copy link
@t-bast

t-bast Apr 26, 2019

I'll start working on an implementation in scala for eclair ;)

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Apr 30, 2019

Member

so being able to use 16 (1 default + 15 additional ones) for a single payload seems extremely nice to me

But it doesn't allow a node to use all payloads for something like a single hop route containing the max amount of information. I don't think we need to unnecessarily paint our selves into a corner with limiting work arounds like this. Instead, the realm can stay as it is, and a byte of the hop payload be used for type purposes. This would also allow for "type name-spacing", as I can send you frames, each using a distinct TLV namespace. The current proposal doesn't allow such composition, and seems to assume that 255 types will be enough for the foreseeable future.

This comment has been minimized.

Copy link
@t-bast

t-bast Apr 30, 2019

Instead, the realm can stay as it is, and a byte of the hop payload be used for type purposes

The issue with that is that it breaks the current component layering.
What's nice with having the frame count in the first byte is that the sphinx layer knows that it only has to look at this first byte to split the packet correctly and pass it off to other components for actual interpretation of the plaintext payload.
If we want to put the frame count in another byte than the first one, it will be a breaking change (and we can't have backwards-compatibility so it's going to be a lot harder to deploy).

We said during yesterday's meeting that we will eventually need to make some changes around version management for the onion packet (to support per hop versioning). I think at that point we will introduce a version 1 with breaking changes, and we can re-visit the onion structure entirely to address these shortcomings. Does that sound acceptable?

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Apr 30, 2019

Member

If we want to put the frame count in another byte than the first one, it will be a breaking chang

I don't see why this is the case. Today, we have 13 unused bytes that are "padding", I'm proposing using one of those bytes as the number of frames, and using the realm byte as the packet type. Older nodes have their behavior unchanged as that byte is still zero for them. For any older nodes that don't signal this new feature, you wouldn't include any EOB data at all.

The current split also means we only have 16 possible packet types. In this day and age, I think we can all agree that's far too little for any sort of reasonable extensibility. If we use the entire realm/packet type, then we at least gain 255 types, which can then be augmented by having distinct TLV namespaces under each of those packet types.

ephemeralPrivKey := btcec.PrivKeyFromBytes(btcec.S256(), ephemeralKey.Bytes())
ephemeralPubKey := ephemeralPrivKey.PubKey()
// NewOnionPacket creates a new onion packet which is capable of
// obliviously routing a message through the mix-net path outline by

This comment has been minimized.

Copy link
@t-bast

t-bast Apr 24, 2019

nit: typo: outlined

@t-bast

t-bast approved these changes Apr 26, 2019

@btcontract btcontract referenced this pull request Apr 26, 2019

Closed

Multipart payments #946

@btcontract

This comment has been minimized.

Copy link

commented Apr 29, 2019

I'm especially interested in simple multi-part payments, is there a discussion about them anywhere?

@t-bast t-bast referenced this pull request Apr 29, 2019

Draft

Multi frame onion #976

1 of 2 tasks complete
@cdecker

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 29, 2019

I'm especially interested in simple multi-part payments, is there a discussion about them anywhere?

they're rather simple, since we just need to define a single TLV key which is used to specify the total amount to wait for.

overlay network): more specifically, _sending peers_ forward packets
to_receiving peers_.
- Route length: the maximum route length is limited to 20 hops.
- The longest route supported has 20 hops without counting the _origin node_

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Apr 30, 2019

Member

Perhaps we should differentiate between the objective and subjective routing lengths? So in other words, the "true" route length (number of nodes that the HTLC will travel through), and the packet-level route length (20 max).

- The terms _hop_ and _node_ are sometimes used interchangeably, but a _hop_
usually refers to an intermediate node in the route rather than an end node.
_origin node_ --> _hop_ --> ... --> _hop_ --> _final node_
usually refers to an intermediate node in the route rather than an end node.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Apr 30, 2019

Member

I think that prior example was helpful, it aides in identifying that a route with 3 nodes has 2 hops for example.

@@ -150,52 +155,75 @@ The overall structure of the packet is as follows:
2. data:
* [`1`:`version`]
* [`33`:`public_key`]
* [`20*65`:`hops_data`]
* [`20*FRAME_SIZE`:`hops_data`]

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Apr 30, 2019

Member

Perhaps we should also lift 20 into a MAX_HOPS constant while we're at it?

This comment has been minimized.

Copy link
@t-bast

t-bast Apr 30, 2019

Since FRAME_SIZE (65 bytes) is fixed and hops_data length is fixed too (1300 bytes), I suggest FRAME_COUNT (max doesn't really make sense because it implies we could use less, which would result in a smaller onion which isn't allowed).

1. type: `per_hop` (for `realm` 0)
1. type: `hop_payload`
2. data:
* [`1`: `num_frames_and_realm`]

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Apr 30, 2019

Member

What happened to the packet type? IMO we still shouldn't intermingle them, as it's feasible that a distinct realm may also warrant an entirely different interpretation of the TLV values.

1. type: `per_hop` (for `realm` 0)
1. type: `hop_payload`
2. data:
* [`1`: `num_frames_and_realm`]

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Apr 30, 2019

Member

so being able to use 16 (1 default + 15 additional ones) for a single payload seems extremely nice to me

But it doesn't allow a node to use all payloads for something like a single hop route containing the max amount of information. I don't think we need to unnecessarily paint our selves into a corner with limiting work arounds like this. Instead, the realm can stay as it is, and a byte of the hop payload be used for type purposes. This would also allow for "type name-spacing", as I can send you frames, each using a distinct TLV namespace. The current proposal doesn't allow such composition, and seems to assume that 255 types will be enough for the foreseeable future.

receiving peer in the route.
- The `hops_data` field is right-shifted by `hop_payload_len` bytes, discarding the last `hop_payload_len` bytes that exceed its 1300-byte size.
- The payload for the hop is serialized into that hop's `raw_payload`, using the desired format, and the `num_frames_and_realm` is set accordingly.
- The `num_frames_and_realm`, `raw_payload`, `padding` and `HMAC` are copied into the first `hop_payload_len` bytes of the `hops_data`, i.e., the bytes that were just shifted in.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Apr 30, 2019

Member

Shouldn't this be hop_payload_len_i?

// Before we assemble the packet, we'll shift the current
// mix-header to the right in order to make room for this next
// per-hop data.
rightShift(mixHeader[:], path[i].HopPayload.CountFrames()*frameSize)

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Apr 30, 2019

Member

Could use a comment here along the lines:

CountFrames() returns the total number of frames (factoring in any needed padding) required to fully encode the hop payload.

Show resolved Hide resolved 04-onion-routing.md
Show resolved Hide resolved 04-onion-routing.md
@@ -0,0 +1,43 @@
{
"comment": "A simple error returned by node hops[4], the public/private keys and shared_secrets are identical to the ones used in `onion-test-v0.json`",

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Apr 30, 2019

Member

Yay JSON!

This comment has been minimized.

Copy link
@cdecker

cdecker Apr 30, 2019

Author Collaborator

I know 馃槈 But it was the simplest way I could figure out to add a JSON-compatible comment.

cdecker added some commits Mar 28, 2019

bolt04: Formatting cleanup and fold clarifications into conventions
The clarifications were tacked on after the fact, but they should really be
part of the conventions. I also updated the links to use the reference style,
which results in better text flow and makes it easier to read the source.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
bolt04: Introduce the notion of frames and make payloads variable
Introduces `frame`s and all related concepts such as `FRAME_SIZE`. It also
fixes a conceptual issue where `hops_data` would be used both for the section
of the overall packet as well as the wire format for individual payloads. This
separation now lends itself better to the incremental wrapping of the onion.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
bolt04: Update the technical description of the multi-frame proposal
Signed-off-by: Christian Decker <decker.christian@gmail.com>
bolt04: Move the test vectors into JSON documents
They were cluttering the spec document, and not easily integrated into the
tests for implementations. This just cleans up the spec and allows automated
testing going forward.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
bolt04: Update spell check dictionary
Signed-off-by: Christian Decker <decker.christian@gmail.com>

@cdecker cdecker force-pushed the multi-frame branch from 74e17c7 to 89fb441 Apr 30, 2019

@cdecker

This comment has been minimized.

Copy link
Collaborator Author

commented May 2, 2019

(Sorry, I could not reply to the comment inline for some reason, so adding
this here)

But it doesn't allow a node to use all payloads for something like a single
hop route containing the max amount of information. I don't think we need to
unnecessarily paint our selves into a corner with limiting work arounds like
this. Instead, the realm can stay as it is, and a byte of the hop payload be
used for type purposes.

I think you meant keeping the realm byte the same (maybe renaming it to packet
type as per discussion), while the first byte of the second byte (first byte
of former payload) is used to count the frames that are to be read.

While I understand your objection to limiting the number of frames we can use
for a single payload, I think this limitation is a necessary evil because your
proposal has the following downsides:

  • It fundamentally is a layering violation if the the onion processing is
    required to interpret the realm byte and depending on that inspection
    either look into the payload or not. I'd like to keep the onion processing
    code as separate as possible from any payload interpretation logic, because
    the processing is already hard enough without having to jump back and forth
    between layers.
  • It optimizes for very short routes resulting in any application making
    use of the extra size not working on normal sized routes. We should not
    encourage applications that may only work for very short routes since that
    limits the scalability of the network ("sorry, you are 5 hops away, can't
    use the super nifty feature, please connect directly...").

TBH I'm personally not a big fan of this kind of byte-stuffing (splitting a
byte into two 4-bit nibbles and then giving them separate interpretations),
but to me it seems like the cleanest solution from an architecture point of
view.

This would also allow for "type name-spacing", as I can send you frames,
each using a distinct TLV namespace. The current proposal doesn't allow such
composition, and seems to assume that 255 types will be enough for the
foreseeable future.

With the current proposal we still have 15 packet types (0 being used for
the legacy hop_data format) we can use for this kind of TLV type extension
(assuming the TLV payload format is the end-all-be-all format). This results
in a total of 255 * 15 = 3825 TLV types. If we ever bump against this limit
we should likely rethink TLV anyway, maybe making the type a varint instead
of the single byte as per #572. However, I don't think this is a real concern
at the moment 馃槈

@Roasbeef

This comment has been minimized.

Copy link
Member

commented May 3, 2019

I think you meant keeping the realm byte the same (maybe renaming it to packet
type as per discussion), while the first byte of the second byte (first byte
of former payload) is used to count the frames that are to be read.

Yeah exactly!

It fundamentally is a layering violation if the the onion processing is
required to interpret the realm byte and depending on that inspection
either look into the payload or not.

This is a completely arbitrary distinction, and it's also how things work in this proposal as is (they need to check the realm to know if there're any additional frames). It's also possible to keep the realm as is, and use 2 of the filler bytes (type and number of frames). It really depends on what demarcates the packet header, and the payload itself. I'm proposing the "packet header" stops at the 2 extra (or just 1) padding byte.

We should not
encourage applications that may only work for very short routes since that
limits the scalability of the network

I disagree, it's just about lifting arbitrary limitations like only having 16 packet types or being able to only use 16 frames. It's up the developers what this space will be used for, not us, and we shouldn't make value judgements on certain classes of use cases that don't have an averse effect on network health.

TBH I'm personally not a big fan of this kind of byte-stuffing (splitting a
byte into two 4-bit nibbles and then giving them separate interpretations),
but to me it seems like the cleanest solution from an architecture point of
view.

OK, so we're partially on the same page here. The distinction of a "layer" in this case is completely arbitrary. It's all the same packet, the only distinction is what we deem to be the packet header vs the body/payload. 15 packets type are simply not enough.

With the current proposal we still have 15 packet types

And by using another byte or two, we can avoid this bit packing, and gain access to 255 packet types and 65K total TLV types. IMO it's short sighted to conclude that 15 types is anywhere near enough given all the developer excitement around this feature (at the SF Lightning Dev meetup last night, multiple devs asked me when the EOB space was finally going to be opened up).

@cdecker

This comment has been minimized.

Copy link
Collaborator Author

commented May 4, 2019

This is a completely arbitrary distinction, and it's also how things work in this proposal as is (they need to check the realm to know if there're any additional frames). It's also possible to keep the realm as is, and use 2 of the filler bytes (type and number of frames). It really depends on what demarcates the packet header, and the payload itself. I'm proposing the "packet header" stops at the 2 extra (or just 1) padding byte.

It is not arbitrary. I want to separate the processing of the onion as much from the processing of the payload as possible. If we use 2 bytes as you suggest the second byte is both in the payload region (legacy hop_data format) and in the onion part (1 byte num_frames for non-0 packet types), so we suddenly have this distinction in the onion processing code, and we just lost our clean separation of concerns.

I disagree, it's just about lifting arbitrary limitations like only having 16 packet types or being able to only use 16 frames. It's up the developers what this space will be used for, not us, and we shouldn't make value judgements on certain classes of use cases that don't have an averse effect on network health.

But it is our job to ensure the incentives are set correctly, otherwise we end up with a network that is full of single-hop payments, and we might as well strike the "network" part off the LN name.

I disagree, it's just about lifting arbitrary limitations like only having 16 packet types or being able to only use 16 frames. It's up the developers what this space will be used for, not us, and we shouldn't make value judgements on certain classes of use cases that don't have an averse effect on network health.

Like I mentioned above it is not an arbitrary distinction, and 16 payload types ought to be enough for the foreseeable future.

@cdecker

This comment has been minimized.

Copy link
Collaborator Author

commented May 4, 2019

And by using another byte or two, we can avoid this bit packing, and gain access to 255 packet types and 65K total TLV types. IMO it's short sighted to conclude that 15 types is anywhere near enough given all the developer excitement around this feature (at the SF Lightning Dev meetup last night, multiple devs asked me when the EOB space was finally going to be opened up).

Wouldn't that concern be better addressed by making the type in the TLV definition a varint instead of separating the type into a TLV-type-byte and the context packet-type? That way the TLV type space is self-contained and we don't have a composite type made up of packet-type and TLV-type.

Notice also that my prior observation about having packet-type-len * TLV-type-len is wrong since we can only ever select a single packet-type for the entire payload. If we select packet-type a, we cannot use TLV-types that are defined for packet-type b, which would force us to redundantly define TLV-types that may appear in other packet-types. Having a varint TLV-type allows us to fully utilize all type without having to use the cumbersome composite typing.

Another compromise then would be to use 3 bytes for packet-type (given that we now have 4 billion possible TLV-types, we don't need the hack of using packet-type bits to namespace the TLV-types), then we could use 5 bits for the num_frames portion and really address all 20 possible frames. Would that be acceptable @Roasbeef ?

rustyrussell added a commit to niftynei/lightning-rfc that referenced this pull request May 6, 2019

extract-formats: allow UPPERCASE in field lengths.
For example, @cdecker used a FRAME_SIZE constant in lightningnetwork#593 which broke
parsing.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
bolt04: Added multi-frame test vector
I seem to have forgotten this the first time around, so here it goes.
@t-bast

This comment has been minimized.

Copy link

commented on bolt04/onion-test-multi-frame.json in 6575ffa May 6, 2019

Is this comment up-to-date? The third hop has a multi-frame payload (since that's what we want to test ;)...).
And in that case the realm byte for the third hop should contain the number of frames, shouldn't it (unless we accept Rusty's proposal instead)?

This comment has been minimized.

Copy link
Collaborator Author

replied May 6, 2019

Aw, forgot to update that one, sorry.

@Roasbeef

This comment has been minimized.

Copy link
Member

commented May 10, 2019

It is not arbitrary. I want to separate the processing of the onion as much from the processing of the payload as possible

The arbitrary line is what counts as the "payload" and what constitutes the "packet header". In my model, the payload starts after these 1 or 2 bytes which were previously the padding.

Wouldn't that concern be better addressed by making the type in the TLV definition a varint instead of separating the type into a TLV-type-byte and the context packet-type?

A var in with what max size? In any case, even with more bytes for the TLV types themselves, having packet types be able to modify the namespace is still warranted, as it creates an explicit new context for any TLV namespaces.

we cannot use TLV-types that are defined for packet-type b, which would force us to redundantly define TLV-types that may appear in other packet-types.

If each frame specifies a packet type, then there's no issue here.

Another compromise then would be to use 3 bytes for packet-type

Which 3 bytes? Why's this better than just using 1 byte for the packet type and 1 for the number of frames?

@cdecker

This comment has been minimized.

Copy link
Collaborator Author

commented May 10, 2019

A var in with what max size? In any case, even with more bytes for the TLV types themselves, having packet types be able to modify the namespace is still warranted, as it creates an explicit new context for any TLV namespaces.

Do you really care whether we use 1, 2 or 4 bytes? All foreseeable use-cases would be 1 byte or 2 bytes anyway, and if developers want to waste extra space encoding TLV custom types they can. I'm now convinced that varint types for TLV are way more sensible because they keep the type information in one location instead of being spread across a realm and a type byte. If you really want namespacing we can define ranges in the TLV type number space, but let's not spread type information into two fields that have very different scopes.

If each frame specifies a packet type, then there's no issue here.

Please, just no. That'd mean we have to split any payload that doesn't align with the 65 byte frame boundary in order to be able to have that special realm byte, and we'd lose the nice property of just being able to read memory instead of re-assembling the data. The varint TLV types are much cleaner in this case.

Which 3 bytes? Why's this better than just using 1 byte for the packet type and 1 for the number of frames?

My bad, I meant bits. So the split of the realm byte (8 bits) would be 3 bits for packet types and 5 bits for num_frames, giving us a total of 8 packet types (since we would be using varints there would be no need to abuse them for TLV type namespacing) and 32 addressable frames (i.e., all 20 frames in the onion can be used for a single payload).

@Roasbeef

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Do you really care whether we use 1, 2 or 4 bytes?

Given the limited space in the payload, it's a pertinent question as it also determines the total number of types as well as lost space due to signalling overhead. I'm OK with just extending the number of types rather than splitting the context across another field. Though this is out of the scope of this PR as it's a matter of the TLV format in the onion.

So the split of the realm byte (8 bits) would be 3 bits for packet types and 5 bits for num_frames, giving us a total of 8 packet types (since we would be using varints there would be no need to abuse them for TLV type namespacing)

Hmm, down to 8 packet types...I guess it all depends on what we think the packet types will be used for in the future. Rusty's modified proposal introduces the concept of using them to re-define the format of the existing hop payload. Beyond that we also have the onion packet type itself to modulate which grants additional freedom.

@@ -280,6 +280,13 @@ The following `address descriptor` types are defined:
`[32:32_byte_ed25519_pubkey] || [2:checksum] || [1:version]`, where
`checksum = sha3(".onion checksum" | pubkey || version)[:2]`.

The following feature bits are currently assigned by this specification:

This comment has been minimized.

Copy link
@t-bast

t-bast May 14, 2019

nit: shouldn't feature flags be speced in Bolt 9 instead of scattered accross bolts?

This comment has been minimized.

Copy link
@cdecker

cdecker May 14, 2019

Author Collaborator

Good catch, I misread bolt 09 to be specific for globalfeatures and localfeatures. Fixup appended.

@t-bast

This comment has been minimized.

Copy link

commented May 14, 2019

Just to make sure I understood properly yesterday's decision in the IRC meeting: if we were to merge this version of the PR, we would do a follow-up PR to spec that num_frames_and_realm value 0x01 (1 frame, realm 1) must be a 1-frame payload that internally uses TLV, right?

That would address the discrepancy between this PR and #604 (in #604 this 1-frame TLV payload is realm value 0x01 too).

fixup! bolt07: Add feature bits for multi-frame support
Signed-off-by: Christian Decker <decker.christian@gmail.com>
@cdecker

This comment has been minimized.

Copy link
Collaborator Author

commented May 14, 2019

Just to make sure I understood properly yesterday's decision in the IRC meeting: if we were to merge this version of the PR, we would do a follow-up PR to spec that num_frames_and_realm value 0x01 (1 frame, realm 1) must be a 1-frame payload that internally uses TLV, right?

That would address the discrepancy between this PR and #604 (in #604 this 1-frame TLV payload is realm value 0x01 too).

Yes, that's the plan. The followup would define these realm bytes for TLV payloads:

|-------------------|--------------|------------|
| Additional frames | Total frames | realm byte |
|-------------------|--------------|------------|
| 0                 | 1            | 0x1        |
| 1                 | 2            | 0x9        |
| 2                 | 3            | 0x11       |
| 3                 | 4            | 0x19       |
| 4                 | 5            | 0x21       |
| 5                 | 6            | 0x29       |
| 6                 | 7            | 0x31       |
| 7                 | 8            | 0x39       |
| 8                 | 9            | 0x41       |
| 9                 | 10           | 0x49       |
| 10                | 11           | 0x51       |
| 11                | 12           | 0x59       |
| 12                | 13           | 0x61       |
| 13                | 14           | 0x69       |
| 14                | 15           | 0x71       |
| 15                | 16           | 0x79       |
| 16                | 17           | 0x81       |
| 17                | 18           | 0x89       |
| 18                | 19           | 0x91       |
| 19                | 20           | 0x99       |

Due to the switch to a 5bit + 3 bit split the numbers don't align on the nibble in the hex representation anymore, but that's just cosmetic at this point 馃槈

@t-bast

This comment has been minimized.

Copy link

commented May 14, 2019

Perfect that makes total sense, I'm on board with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.