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

ZIP 302: Structured memos #638

Closed
wants to merge 1 commit into from
Closed

Conversation

str4d
Copy link
Collaborator

@str4d str4d commented Oct 6, 2022

No description provided.

zip-0302.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

ACK

zip-0302.rst Outdated Show resolved Hide resolved
zip-0302.rst Outdated Show resolved Hide resolved
zip-0302.rst Outdated
+ Interpret the next few bytes (1 to 9 of them) as a 64-bit unsigned variable-length
integer [#Bitcoin-CompactSize]_, and use it as an arbitrary application-defined
"type" field.
+ Interpret the next 1 or 2 bytes as a CompactSize value constrained to the range 0..510, and use
Copy link
Collaborator

Choose a reason for hiding this comment

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

Strictly speaking this can't be more than 507, because the 0xF5 || type || length is encoded as at least 3 bytes for length <= 252 and 5 bytes for lengths > 252, therefore the maximum is 512 - 5. But it's correct as stated because we only need a conservative upper bound.

Copy link
Contributor

@sellout sellout Oct 26, 2022

Choose a reason for hiding this comment

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

It’s probably not worth squeezing out the extra couple bytes, but we could store 512 - length instead of storing length, which gets us a range 0..509, so then payloads over 260 bytes only require one byte for the “length”, and smaller payloads require three (which they aren’t using anyway).

Copy link
Contributor

@sellout sellout Oct 27, 2022

Choose a reason for hiding this comment

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

I think the ZIP mixes up the encodings it mentions – the type is stored as CompactSize (1–9 bytes), and the length is stored as VarInt (which fits a value of 508 in 2 bytes, not 3). I found https://bitcoin.stackexchange.com/a/114585, which explains the encodings better than the Bitcoin docs do.

However, just using a regular 2-byte length

  • also leaves 508 bytes for the memo;
  • is simpler; and
  • is consistent with the nested type || length in [ZIP ???] Multipart Memos #247, which says “Total length of encoded multipart memo (2 bytes, as a 16-bit little-endian integer)”.

The multipart length obviously gets no benefit from a variable-length encoding, so I think we should just use a non-variable length 2-byte uint here as well.

Copy link
Collaborator

@daira daira left a comment

Choose a reason for hiding this comment

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

ACK

@daira daira marked this pull request as ready for review October 26, 2022 21:24
@str4d
Copy link
Collaborator Author

str4d commented Nov 23, 2022

In ZIP Sync today, we decided that:

  • We would specify the multi-TLV format using prefix byte 0xF7.
    • Concatenated TLV tuples.
    • Type code 0x00 is an optional "no more TLV tuples" marker, after which everything must be 0x00 (i.e. fixed padding).
  • We would abandon 0xF5 as "for legacy private agreement", and document that newer applications should prefer 0xFF which is unambiguously for this purpose.
    • We decided that using up another top-level prefix byte here is fine, because it opens up the typecode space significantly (it adds >250 new single-byte typecodes that can encode 509 bytes, which is not a significant reduction from the 511 bytes that a top-level prefix byte can encode).

@str4d
Copy link
Collaborator Author

str4d commented Jul 18, 2024

I've replaced the old contents of this PR (which just reverted the prior content that was removed from the original ZIP 302 PR) with an initial draft following what was decided in ZIP Sync almost 2 years ago.

@str4d str4d mentioned this pull request Jul 18, 2024
@str4d str4d changed the base branch from main to zip302-abandon-f5 July 18, 2024 05:08
@str4d str4d deleted the branch zip302-abandon-f5 July 18, 2024 12:44
@str4d str4d closed this Jul 18, 2024
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.

5 participants