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

BOLT01: swap CompactSize for BigSize in TLV format #640

Merged
merged 2 commits into from Jul 22, 2019

Conversation

cfromknecht
Copy link
Collaborator

This commit modifies the varint encoding used for TLV types and lengths
to use a custom format called BigSize. The format is identical to
bitcoin's CompactSize, except it replaces the use of little-endian
encodings for multi-byte values with big-endian. This is done to prevent
mixing endianness within the protocol, since otherwise CompactSize would
be the first introduction of little-endian encodings.

@cfromknecht cfromknecht mentioned this pull request Jul 12, 2019
@cfromknecht cfromknecht force-pushed the tlv-varint-testvectors branch 4 times, most recently from 5eaa876 to 41a50d9 Compare July 12, 2019 02:32
Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

I honestly don't have a strong opinion on the endianness we should use.
Keeping Bitcoin's little-endian encoding or switching to big-endian as you propose both seem ok.
I'll let @rustyrussell chime in to decide.

01-messaging.md Outdated Show resolved Hide resolved
.aspell.en.pws Outdated
tlv
ExpErr
Fatalf
fc
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit weird to have to add all of those in the spell-checker...
I think that if we always wrap them in back-ticks (e.g. 0xfc) the spell-checker doesn't look at them, which might be a better solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, i gave a shot at modifying the spell checker to ignore code blocks, but not sure that's the best approach

Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm +1 for ignoring code blocks fwiw

@cfromknecht cfromknecht force-pushed the tlv-varint-testvectors branch 2 times, most recently from 096953b to 770e92c Compare July 12, 2019 23:34
This commit modifies the varint encoding used for TLV types and lengths
to use a custom format called BigSize. The format is identical to
bitcoin's CompactSize, except it replaces the use of little-endian
encodings for multi-byte values with big-endian. This is done to prevent
mixing endianness within the protocol, since otherwise CompactSize would
be the first introduction of little-endian encodings.
@rustyrussell
Copy link
Collaborator

Ack.

We ended up not reusing the Bitcoin routines and reimplementing them for this, so using Bitcoin endianness here was a lose. Wasn't sure if other implementations were the same.

JSON for testcases is a bit useless without a programmatic extractor IMHO, but I'm just happy to have test vectors at all. We can have a Grand Testcase Unification later...

@rustyrussell
Copy link
Collaborator

PS. I've lost count of how many beers I owe @cfromknecht now, but I suspect it may be more than we can drink in Berlin...

@t-bast
Copy link
Collaborator

t-bast commented Jul 15, 2019

ACK. Let's decide on this once and for all, big-endian everywhere it is!

@rustyrussell
Copy link
Collaborator

Tested-Ack: 8038471

rustyrussell pushed a commit to rustyrussell/lightning that referenced this pull request Jul 16, 2019
	lightning/bolts#640

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Jul 18, 2019
Based on:
	lightning/bolts#640

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell added a commit to ElementsProject/lightning that referenced this pull request Jul 18, 2019
Based on:
	lightning/bolts#640

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@niftynei niftynei added the Meeting Discussion Raise at next meeting label Jul 22, 2019
@t-bast t-bast moved this from Scheduled to Accepted in Specification Meeting Agenda Jul 22, 2019
@niftynei niftynei merged commit 65784f7 into lightning:master Jul 22, 2019
@niftynei niftynei removed the Meeting Discussion Raise at next meeting label Jul 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants