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

Add CompactSize fundamental type #1062

Closed
wants to merge 1 commit into from

Conversation

dunxen
Copy link
Contributor

@dunxen dunxen commented Mar 28, 2023

The CompactSize (compactsize) fundamental type is added to allow the use of Bitcoin consensus serialization where desired, for example, a list of witnesses.

The definition is reproduced in BOLT 1 for convenience and necessarily matches that of CompactSize (VarInt), defined for Bitcoin at: https://en.bitcoin.it/wiki/Protocol_documentation#Variable_length_integer.


Note: Kept the test vectors and example test code the same as for BigSize, but just inverted the endianness. Checked against consensus serialization using rust-bitcoin locally. Thought it was better to keep things as explicit as possible, but I can remove something if it's a bit too redundant.

Motivation: This is especially useful for cases such as for a list of witnesses mentioned above as seen in the tx_signatures message in #851. See here for more context.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Thanks!

01-messaging.md Outdated Show resolved Hide resolved
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.

ACK 5b66a55, I verified the test vectors against eclair 👍

@dunxen
Copy link
Contributor Author

dunxen commented Mar 29, 2023

Will squash the fix up quick :)

The CompactSize (compactsize) fundamental type is added to allow the use
of Bitcoin consensus serialization where desired, for example, a list of
witnesses.

The definition is reproduced in BOLT 1 for convenience and necessarily
matches that of CompactSize (VarInt), defined for Bitcoin at:
https://en.bitcoin.it/wiki/Protocol_documentation#Variable_length_integer.
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.

ACK 349353b

@Roasbeef
Copy link
Collaborator

Roasbeef commented Apr 10, 2023

Why not just use BigSize? We added that since we wanted to keep it 100% big-endian, and wanted to avoid mixing endian like Bitcoin does.

I think it would make sense only if people are actually taking the raw bytes, and stuffing it as raw bytes directly into some optimized sighash function. Otherwise, I'm assuming everyone would take the encoded witness, put the raw byte slice/vector into some struct, then use that to serialize the sighash, etc.

@dunxen
Copy link
Contributor Author

dunxen commented Apr 10, 2023

Why not just use BigSize?

The point is to be able to explicitly specify the bitcoin consensus encoding for structures such as witnesses in BOLTs (instead of just saying "encoding used in bitcoin").

It's not used for anything other than bitcoin types that need it. We want to avoid re-implementing (efficient) encoders/decoders for bitcoin types with BigSize if we already have an existing consensus definition for them using CompactSize.

@rustyrussell
Copy link
Collaborator

No, this is wrong. If lightning has to deal with it, we'll use the same endian everywhere please.

But, if we really just want a "witness blob" here which we don't deal with, we should use a simpler encoding:

1. type: 71 (`tx_signatures`)
2. data:
    * [`channel_id`:`channel_id`]
    * [`sha256`:`txid`]
    * [`u16`:`num_witnesses`]
    * [`num_witnesses*witness`:`witnesses`]

1. subtype: `witness`
2. data:
    * [`u16`:`len`]
    * [`len*byte`:`witness`]

You might use simple referential language like so:

The witness is encoded as per bitcoin's wire protocol (a CompactSize number of elements, with each element a CompactSize length and that many bytes following).

@dunxen
Copy link
Contributor Author

dunxen commented Apr 11, 2023

You might use simple referential language like so:


The witness is encoded as per bitcoin's wire protocol (a CompactSize number of elements, with each element a CompactSize length and that many bytes following).

This also works. If less explicit is fine then that's ok.

@t-bast
Copy link
Collaborator

t-bast commented Apr 11, 2023

@rustyrussell's suggestion works for me, this also makes the tx_signatures tlvs simpler!

@dunxen
Copy link
Contributor Author

dunxen commented Apr 11, 2023

Great, closing this as we have no issues with it being defined with a reference.

@dunxen dunxen closed this Apr 11, 2023
@t-bast
Copy link
Collaborator

t-bast commented Apr 11, 2023

@dunxen can you open a PR on https://github.com/niftynei/lightning-rfc/tree/nifty/dual-fund to update the tx_signatures message to use a blob of bytes as witness data as suggested by Rusty? This way we'll directly integrate it in the dual funding PR.

@dunxen
Copy link
Contributor Author

dunxen commented Apr 11, 2023

can you open a PR on https://github.com/niftynei/lightning-rfc/tree/nifty/dual-fund to update the tx_signatures message

Modified the draft one I had there and made it ready for review at niftynei#14 :)

@t-bast
Copy link
Collaborator

t-bast commented Apr 11, 2023

Nice, I missed that you already had one open ;)

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.

None yet

5 participants