-
Notifications
You must be signed in to change notification settings - Fork 224
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 a backwards-compatible proposal for future-proofing the type-bits #19
Open
Steve132
wants to merge
1
commit into
bitcoincashorg:master
Choose a base branch
from
Steve132:patch-1
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is implied by the reservation of the leftmost bit being 0, but it may be good to make this explicit. @deadalnix
Also we should update the version number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply saying that the leftmost bit is reserved is not the same thing as implying that it is to be interpreted as a 0xFD 0xFE 0xFF Bitcoin VarInt. :) I don't know of any implementations that do that If that's the case, we should make it explicit now (like I've done in the pull request).
Like actually say that if the first byte is 0xFD then there are two more bytes of version, 0xFE for 4 bytes, 0xFF for 8 bytes, like in the rest of the Bitcoin code, would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly confused... are we turning the whole version byte into a VarInt, or just the type part of it? If it's about the full version byte, then we'd also need to define how many of the LSBits will then be the payload-size part for each of the VarInt-sizes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole version byte becomes a varint. That's necessary in order to maintain backcompat with existing spec for MSB=0. It also simplifies the implementation quite a bit.
This doesn't extend the potential sizes of the payload, because that's beyond the scope of this spec, which is designed to be simple. The sizes are still the 8 sizes defined in the spec. Under this change, the sizes are still the 8 sizes defined in the spec.
However, we could later define a region of the larger 64-bit type space for which the sizes are extended if we wanted. However, that's impossible unless we first extend the type space.
Side note: is there a reason that CashAddr doesn't simply do what bech32 does and actually use a serialization of the scriptPubKey? That would allow us to skip a large headache with all of these different address types and stuff completely and then people would be able to automatically see what kind of address it was (if there's any weird non-standard scriptPubKeys) from the serialization.
Maybe a separate discussion should be added for implementing that as a type (like type 04?) and cut off a lot of the need for weird new types off at the pass? Pros: new weird smart contracts with weird scriptPubKeys don't need a new type for each kind of smart contract, because the whole thing is in the address. Cons: It would be possible to construct two valid addresses for a p2sh or p2pkh address (one the 'standard' way and one by encoding the scriptPubKey)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we first extended the type-range, it would otoh be impossible to later define different payload-sizes for certain sub-ranges of types...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not the case. In fact, it's the opposite. What we're really doing here is extending the entire version byte to potentially be larger to be a version int.
For example, later extensions could say that if the version int is between 0x4000 and 0x8000, then in that case the bottom 8 bits define the size instead of the bottom 3. Parsers that don't recognize 0x4000 as a valid type will then simply fail to parse and explain why (unsupported version) versus the status quo.
My proposal allows for backwards compatibility with existing code while still allowing forwards extensions like you're proposing.