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

Change protocol versioning to allow independent upstream and zcash changes. #114

Closed
nathan-at-least opened this issue Feb 21, 2015 · 11 comments
Labels
A-consensus Area: Consensus rules A-documentation Area: Documentation C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase. dev policy special to Daira Zcash codebase

Comments

@nathan-at-least
Copy link
Contributor

Tracking upstream while also having flexibility to extend or customize the protocol is going to be tricky, and one indicator of the complexity is deciding how to do protocol versioning.

For example, consider if we have a policy for the block CURRENT_VERSION that says "whenever upstream increments the version, we also increment it. Whenever we alter the protocol, we also increment the version."

Now we need to remember a mapping from each ZC block-scoped protocol version to each upstream block-scoped protocol version. This also means we must alter every code site which does versioning checks appropriately.

Another option might be to have separate ZC and upstream version number fields in blocks. The advantage here is that the mapping from the upstream version is direct, and code which reacts to it doesn't need to be modified by ZC. The disadvantage is that the (block-scoped) protocol version is now a 2-dimensional tuple rather than a fully ordered int, so if we need to introduce a total ordering, there will be some weird complex logic.

@daira
Copy link
Contributor

daira commented Sep 23, 2015

I think that a pair of version numbers with lexicographic ordering is not significantly more complex than a single version number.

@daira
Copy link
Contributor

daira commented Sep 23, 2015

We can even potentially encode that ordering into a single version number (e.g. UPSTREAM_VERSION*10 + ZEROCASH_MINOR_VERSION) if that makes things simpler.

@nathan-at-least nathan-at-least changed the title Decide on a protocol versioning strategy. Change protocol versioning to be allow independent upstream and zcoin changes. Oct 12, 2015
@nathan-at-least
Copy link
Contributor Author

Note: figure out what upstream's "version bits" plan is (there is at least one BIP and people actively working on implementation right now), as it's probably directly relevant.

@nathan-at-least nathan-at-least changed the title Change protocol versioning to be allow independent upstream and zcoin changes. Change protocol versioning to allow independent upstream and zcoin changes. Nov 9, 2015
@ebfull ebfull modified the milestone: Calgary Design Nov 10, 2015
@nathan-at-least nathan-at-least modified the milestones: Calgary Design Snowflake, Dec 1 Timebox Nov 18, 2015
@nathan-at-least
Copy link
Contributor Author

The Calgary design uses the CTransaction nVersion field and defines the Calgary protocol to be version 4. This means if/when Bitcoin upgrades we must deal the protocol upgrade at that time.

@nathan-at-least
Copy link
Contributor Author

We're reconsidering this.

@defuse
Copy link
Contributor

defuse commented Dec 1, 2015

Here are my thoughts.

No matter what syntax we decide on for versioning, there are always going to be a finite number N of valid versions. What I mean by a version, here, is the entire contents of all transaction version fields.

The number N is, in effect, the number of cases transaction-handling code has to deal with. Therefore, we want N to be small. I personally prefer a scheme where each of the N combinations are written down explicitly, and each of those cases have separate code paths. Adding bit fields to the version number violates that, since N grows exponentially in the number of fields you add.

The exception to my desire not to allow exponential N-growth is if it's upstream that makes the bad decision to allow it. For example, if upstream adds bit fields. If they do, it's better to re-use upstream's code with as few changes as possible than it is to try to re-implement their changes in a way that constrains N.

I guess my preference is for version numbers to be opaque symbols (as opaque as a random 128-bit number, say), with no internal meaning, and not even an ordering between them. Anything else should be implemented explicitly as flags/fields which have a meaning within the opaque version they're a part of.

@nathan-at-least nathan-at-least removed this from the Dec 7 Timebox milestone Dec 7, 2015
@daira daira added the A-documentation Area: Documentation label Dec 22, 2015
@daira
Copy link
Contributor

daira commented Dec 29, 2015

The Bitcoin Version bits BIP proposes multiple soft forks going on at the same time, which I think would be awfully complicated to reason about. This just makes me more confident we should have our own separate version field, so that we can allow upstream to do whatever it wants with its field.

daira added a commit that referenced this issue Dec 29, 2015
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@ebfull
Copy link
Contributor

ebfull commented Dec 30, 2015

We need to change the nVersion for our transactions. It's annoying to reason about serialization any other way. (You can't just add a field "to the end" without changing nVersion, or everything will expect to be able to deserialize another version field that potentially isn't there.) It breaks countless tests as well, and would break any future tests or invariants that upstream introduces. This means an additional version field would express nothing we can't already express with the current one -- after all, regardless of what "semantics" our version expresses, upstream would break us anyway. We'd have to edit all their tests and code.

We should treat nVersion as our version field and integrate what we want. It is typically not difficult to do: upstream has made abstractions over these things we can modify. Zooko has also expressed his opposition to the softforking policies of upstream. Do we really care if upstream wants to implement some feature for multiple simultanious softforks? Anything they're softforking (or have softforked) we're forking in on launch anyway.

I think this is somewhere we can afford, for simplicity purposes, to diverge with upstream on.

@daira
Copy link
Contributor

daira commented Jan 5, 2016

Okay, I'm sold by @ebfull 's arguments here. Let's do the simplest thing and stick with a single nVersion that we own the semantics of.

@daira
Copy link
Contributor

daira commented Jan 5, 2016

The "needs prioritization" is just to remind us to look at this ticket (and close it if we have consensus) at the next engineering meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rules A-documentation Area: Documentation C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase. dev policy special to Daira Zcash codebase
Projects
None yet
Development

No branches or pull requests

4 participants