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

BOLT 1: Spec field formatting and parsing #622

Conversation

4 participants
@rustyrussell
Copy link
Collaborator

commented Jun 17, 2019

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.

Show resolved Hide resolved tools/extract-formats.py Outdated
* `u32`: a 4 byte unsigned integer
* `u64`: an 8 byte unsigned integer

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

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jul 8, 2019

Member

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.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jul 8, 2019

Author Collaborator

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))

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jul 8, 2019

Member

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

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jul 8, 2019

Author Collaborator

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?

* `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

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jul 8, 2019

Member

Seems redundant with the point field above.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jul 8, 2019

Author Collaborator

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.

* `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

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Jul 8, 2019

Member

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

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jul 8, 2019

Author Collaborator

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

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

ACK 6015c9c

@t-bast t-bast moved this from Scheduled to Accepted in Specification Meeting Agenda Jul 8, 2019

rustyrussell added some commits Jun 17, 2019

tools/extract-formats.py: rewrite, change output.
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>
Spec: use explicit types, not just bytelengths for fields.
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>
BOLT 1: add TLV types.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
BOLT 1,2,4,7: remove `pubkey` fundamental type in favor of `point`.
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 rustyrussell:spec-field-formatting-and-parsing branch from 6015c9c to e291a9d Jul 9, 2019

@rustyrussell rustyrussell merged commit 6f6ea63 into lightningnetwork:master Jul 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.