Skip to content

BOLT 1: Spec field formatting and parsing#622

Merged
rustyrussell merged 4 commits intolightning:masterfrom
rustyrussell:spec-field-formatting-and-parsing
Jul 9, 2019
Merged

BOLT 1: Spec field formatting and parsing#622
rustyrussell merged 4 commits intolightning:masterfrom
rustyrussell:spec-field-formatting-and-parsing

Conversation

@rustyrussell
Copy link
Collaborator

This rewrites tools/extract-formats.py to handle TLVs and subtypes (@niftynei did that, I just refactored as that built on my prior code which was awful). The new format is still simple CSV, but much more regular.

But it also replaces raw lengths with types in the spec. I think this makes it much clearer, as well as making building on top more accessible.

* `u32`: a 4 byte unsigned integer
* `u64`: an 8 byte unsigned integer

Inside TLV records which contain a single value, leading zeros in
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this is a modification on the existing varint used in the Bitcoin wire protocol? I thought we want to go with something "off the shelf" rather than a custom scheme. If we're open to a custom scheme, then we can use an even more compact varint that just signals "more bytes to follow" using the MSB of the current byte.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, TLV already contains a length. We don't need one.


The following convenience types are also defined:

* `chain_hash`: a 32-byte chain identifier (see [BOLT #0](00-introduction.md#glossary-and-terminology-guide))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this in addition to the sha2 type? To indicate byte reversal on display?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could combine them, though it's technically not required to be a sha2 so I kept them separate.

I thought about adding a txid type, which would be byte-reversed, but decided against. Can revisit if you want?

01-messaging.md Outdated
* `sha256`: a 32-byte SHA2-256 hash
* `signature`: a 64-byte bitcoin Elliptic Curve signature
* `point`: a 33-byte Elliptic Curve point (compressed encoding as per [SEC 1 standard](http://www.secg.org/sec1-v2.pdf#subsubsection.2.3.3))
* `pubkey`: a `point` explicitly for use as a public key
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems redundant with the point field above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But we don't use it as a pubkey, just a basis for generating other keys. So calling it a pubkey is a bit weird. We could call a pubkey a point, but they really have different uses in the spec.

01-messaging.md Outdated
* `signature`: a 64-byte bitcoin Elliptic Curve signature
* `point`: a 33-byte Elliptic Curve point (compressed encoding as per [SEC 1 standard](http://www.secg.org/sec1-v2.pdf#subsubsection.2.3.3))
* `pubkey`: a `point` explicitly for use as a public key
* `preimage`: a 32 byte value used as a preimage for a hash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar comment here as to chain_hash above (also applies to secret below, condensing that comment into here).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but secret is worth differentiating I think since it emphasizes not to casually leak it. preimage is technically a sha256 due to shachain, but that's kind of a detail.

@t-bast
Copy link
Collaborator

t-bast commented Jul 8, 2019

ACK 6015c9c

Editing the previous mess was horrific.  I gave up and rewrote using a
generator.

Changes to output:
1. subtypes and tlvs now handled.
2. The output format now has explicit prefixes, so readers don't have
   to rely on number of fields to interpret data.
3. Each field is split into type and count; count is empty if there's
   no '*x'.
4. TLV stream typenames are repeated; TLV record type names are not
   necessarily unique.
5. The unused offset field is removed.
6. No arguments taken: everything is always printed, and you can grep if you
   only want some.

[ Fixup by <niftynei@gmail.com> ]
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's trivial to make types->lengths, but not so much the other way.

The types I used here are the ones I found useful in implementation, and
I think add some clarity, though we can certainly argue about them.

There's no normative changes to the spec in here.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And remove `secret` and `preimage` types in favor of open-coding.

Agreed-at: http://www.erisian.com.au/meetbot/lightning-dev/2019/lightning-dev.2019-07-08-20.05.html

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the spec-field-formatting-and-parsing branch from 6015c9c to e291a9d Compare July 9, 2019 00:46
@rustyrussell rustyrussell merged commit 6f6ea63 into lightning:master Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants