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

Network upgrade activation mechanism #2898

Merged
merged 10 commits into from Feb 7, 2018
Merged

Conversation

@str4d
Copy link
Contributor

str4d commented Jan 23, 2018

Implements ZIP 200.

Integration with CChainParams inspired by bitcoin/bitcoin#7575.

Includes block index rewinding logic cherry-picked from bitcoin/bitcoin#8149.

Closes #2286. Part of #2905.

@str4d str4d added this to Work Queue in Network Upgrade 0 via automation Jan 23, 2018
@str4d str4d force-pushed the str4d:2286-nu-activation-mechanism branch from f26a878 to 4d7227c Jan 25, 2018
* Caller must check that the height is >= 0 (and handle unknown heights).
*/
UpgradeState NetworkUpgradeState(
int nHeight,

This comment has been minimized.

Copy link
@arielgabizon

arielgabizon Jan 26, 2018

Contributor

isn't an unsigned int better if it's supposed to be positive?

This comment has been minimized.

Copy link
@str4d

str4d Jan 26, 2018

Author Contributor

Height is represented as an int throughout the codebase, and there are places where a negative height has a specific (non-height) meaning. Keeping this as an int ensures we can correctly assert, and that the caller can't accidentally pass in a negative value that gets coerced to positive.

if (nActivationHeight == Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT) {
return UPGRADE_DISABLED;
} else if (nHeight >= nActivationHeight) {
// From ZIP ???:

This comment has been minimized.

Copy link
@arielgabizon

arielgabizon Jan 26, 2018

Contributor

Is there a ZIP for this? This comment needs to be updated.

This comment has been minimized.

Copy link
@str4d

str4d Jan 26, 2018

Author Contributor

Yes, I will update this comment once @daira assigns a ZIP number to the corresponding draft.

This comment has been minimized.

Copy link
@bitcartel

bitcartel Feb 3, 2018

Contributor

It's possible that the caller passes in -1 for nHeight.

This comment has been minimized.

Copy link
@str4d

str4d Feb 3, 2018

Author Contributor

No it isn't, because of the assertion at the start of the function.

}
};

UpgradeState NetworkUpgradeState(

This comment has been minimized.

Copy link
@arielgabizon

arielgabizon Jan 26, 2018

Contributor

Just to see I understand: there is no way to make an Upgrade for a limited range of blocks. Once it's active, it is active for ever - unless you change it's activation height to NO_ACTIVATION_HEIGHT which would require a hard fork

This comment has been minimized.

Copy link
@str4d

str4d Jan 26, 2018

Author Contributor

There's a confusion between upgrades and epochs here, which maybe needs clarification in the ZIP:

  • An upgrade is a single event, with blocks before and after it. It doesn't have a duration.
  • An epoch is the period between two consecutive upgrades. It does have a duration, either explicit (between two upgrades) or implicit (from last upgrade until now).

So if you want some rule to be only applied to a specific epoch, you could use CurrentEpoch() API.

This comment has been minimized.

Copy link
@str4d

str4d Jan 29, 2018

Author Contributor

The ZIP now defines a "branch" vs a "hard fork", which captures the above distinction (epochs are sub-regions of branches, and upgrades are subsets of hard forks).

This comment has been minimized.

Copy link
@arielgabizon

arielgabizon Jan 30, 2018

Contributor

OK, so if you want a change to be for a limited range of blocks, you define in advance two network upgrades that are the endpoints of this range.

}
}

int CurrentEpoch(int nHeight, const Consensus::Params& params) {

This comment has been minimized.

Copy link
@arielgabizon

arielgabizon Jan 26, 2018

Contributor

This assumes the latest upgrade is the last one in the vector. I guess that's fine but should be clearly documented.

This comment has been minimized.

Copy link
@str4d

str4d Jan 26, 2018

Author Contributor

Correct; it assumes that chainparams.cpp is edited to always have the upgrades in-order. I'll add a comment in consensus/params.h to that effect.

@str4d str4d self-assigned this Jan 26, 2018
@str4d str4d force-pushed the str4d:2286-nu-activation-mechanism branch from 4d7227c to 19ecc7d Jan 27, 2018
@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Jan 29, 2018

Over the weekend I've been working on the block reorganization logic for #2905. I'm going to add it to this PR tomorrow, as it both ties into branch ID handling/caching, and exercises the API this PR implements.

@str4d str4d force-pushed the str4d:2286-nu-activation-mechanism branch from 19ecc7d to 11f3aee Jan 29, 2018
@@ -241,6 +243,8 @@ class CTestNetParams : public CChainParams {
consensus.nPowMaxAdjustDown = 32; // 32% adjustment down
consensus.nPowMaxAdjustUp = 16; // 16% adjustment up
consensus.nPowTargetSpacing = 2.5 * 60;
consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nActivationHeight = 100;

This comment has been minimized.

Copy link
@str4d

str4d Jan 29, 2018

Author Contributor

I'm not sure about leaving this here... With the rewind logic, this will cause everyone's testnet chains to be unnecessarily re-orged (trivially - the old chain will be re-connected). But I also want to have a test case with an activation height... I'll look into adding a testnet-only (or maybe regtest-only) internal API for setting an activation height.

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Jan 29, 2018

I'm now debating whether we represent nBranchId as a uint32_t or unsigned char[4]. The former is easier to set and clear; the latter is easier to serialize. Thoughts?

@str4d str4d force-pushed the str4d:2286-nu-activation-mechanism branch 2 times, most recently from 8cda17f to e74c414 Jan 29, 2018
@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Jan 30, 2018

READWRITE(someint) and READWRITE(FLATDATA(somechars)) seem equally easy to serialize. Will a branch id most likely to be documented as 0x02FFAB03 or its equivalent decimal value?

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Jan 30, 2018

I wanted to specify it in hex, for two reasons:

  • It matches existing conventions for specifying "magic bytes"
  • It encourages people to think of them as random bytestrings instead of numbers (that could be sortable or chosen to be small), even if under the hood we used numbers to represent them.

But my issue is how we handle that hex encoding. E.g. 0x54657374 specified in code is AFAIR interpreted as big-endian, but most of the serializers (in particular, the uint32_t one) use little-endian internally. What I ultimately want is for how you see the branch ID bytes on-screen or in-RPC to exactly map to how they are represented in serialized formats.

[EDIT: Fixed example - mistype]

src/chain.h Outdated
@@ -140,6 +142,10 @@ class CBlockIndex
//! Verification status of this block. See enum BlockStatus
unsigned int nStatus;

//! Branch ID of this block. Only accurate if block validity is at least SCRIPTS.
//! Persisted at each activation height, memory-only for intervening blocks.
uint32_t nBranchId;

This comment has been minimized.

Copy link
@str4d

str4d Jan 30, 2018

Author Contributor

I will probably change this to nBlockBranchId (even though it sounds redundant), to disambiguate from nFormatBranchId in transactions.

This comment has been minimized.

Copy link
@daira

daira Jan 30, 2018

Contributor

Good idea, in general I dislike the same field names being used in different types (that are not deliberately polymorphic).

This comment has been minimized.

Copy link
@str4d

str4d Jan 31, 2018

Author Contributor

Elsewhere we decided to use "consensus branch ID" for this "type", so I'll use nConsensusBranchId here.


void UpdateNetworkUpgradeParameters(Consensus::UpgradeIndex idx, int nActivationHeight)
{
regTestParams.UpdateNetworkUpgradeParameters(idx, nActivationHeight);

This comment has been minimized.

Copy link
@str4d

str4d Jan 30, 2018

Author Contributor

Upstream has a similar function for their BIP9 tests, which they later changed to be usable on any params for testing flexibility (while still only allowing regtest-level configuration from outside the binary). I will end up doing the same thing when I pull in bitcoin/bitcoin#8855 (which as a reminder to myself requires bitcoin/bitcoin#6235).

nHeight++;
}

// nHeight is now the height of the first insufficiently-validated block, or tipheight + 1

This comment has been minimized.

Copy link
@str4d

str4d Jan 30, 2018

Author Contributor

I will rebase #2463 on top of this PR, and add a restriction here to prevent rollbacks past the limit.

src/chain.h Outdated
@@ -182,6 +186,7 @@ class CBlockIndex
nTx = 0;
nChainTx = 0;
nStatus = 0;
nBranchId = 0;

This comment has been minimized.

Copy link
@arielgabizon

arielgabizon Jan 30, 2018

Contributor

add to the ZIP that you are including the branch_id in the block

This comment has been minimized.

Copy link
@str4d

str4d Jan 30, 2018

Author Contributor

This is the block index, not the block itself. The inclusion here is just an implementation optimisation.

if (!fReindex) {
uiInterface.InitMessage(_("Rewinding blocks..."));
if (!RewindBlockIndex(chainparams)) {
strLoadError = _("Unable to rewind the database to a pre-upgrade state. You will need to redownload the blockchain");

This comment has been minimized.

Copy link
@arielgabizon

arielgabizon Jan 30, 2018

Contributor

Everytime we are running the client we try to rewind to a pre-upgrade state? What for?

This comment has been minimized.

Copy link
@daira

daira Jan 30, 2018

Contributor

It looks like RewindBlockIndex is supposed to do nothing if no rewind is needed, but its name (and the init message "Rewinding blocks...") does not reflect that.

This comment has been minimized.

Copy link
@str4d

str4d Jan 31, 2018

Author Contributor

I'll see if I can think of a better name (RewindBlockIndexIfNecessary?) and UI message, but FWIW upstream also uses this message. I expect in most cases no one notices it, because it is immediately followed by Verifying blocks....

This comment has been minimized.

Copy link
@arielgabizon

arielgabizon Jan 31, 2018

Contributor

The main thing that confused me is the error message, it implies you are always trying to rewind till a pre-upgrade state, whereas iiuc, e.g.,
if you are missing data like branch id from your last 5 block indices, but the -6 block index is after upgrade and isn't missing data, you'll stop rewinding there.

@@ -1386,6 +1422,14 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
break;
}

if (!fReindex) {
uiInterface.InitMessage(_("Rewinding blocks..."));

This comment has been minimized.

Copy link
@daira

daira Feb 6, 2018

Contributor

Maybe just change this to "Rewinding blocks if needed..."?

// since older blocks can't be served anyway, there's
// no need to walk further, and trying to DisconnectTip()
// will fail (and require a needless reindex/redownload
// of the blockchain).

This comment has been minimized.

Copy link
@daira

daira Feb 6, 2018

Contributor

So what happens if we're pruning and we would otherwise rewind past the HAVE_DATA point? Can we recover from that?

This comment has been minimized.

Copy link
@str4d

str4d Feb 7, 2018

Author Contributor

I don't believe we can. However, we haven't accounted for pruning in any of our shielded transaction changes, so if this needs tweaking, it should be done as part of a concerted effort to ensure pruning works properly.

@daira
daira approved these changes Feb 6, 2018
Copy link
Contributor

daira left a comment

ut-re-ACK, modulo the question about pruning.

Copy link
Contributor

bitcartel left a comment

NACK. I rebased the protocol handshake PR on top of the latest change (which I haven't reviewed yet). Previous rebases, zcashd ran fine. On this rebase, in launching zcashd on mainnet, it launched into rewind mode, so now I am stuck with all RPC calls returning "Rewinding blocks..." and I can see in debug.log output like this:

2018-02-06 17:20:39 Opened LevelDB successfully
2018-02-06 17:20:42 LoadBlockIndexDB: last block file = 74
2018-02-06 17:20:42 LoadBlockIndexDB: last block file info: CBlockFileInfo(blocks=211, size=12553074, heights=265808...266612, time=2018-02-03...2018-02-04)
2018-02-06 17:20:42 Checking all blk files are present...
2018-02-06 17:20:42 LoadBlockIndexDB: transaction index enabled
2018-02-06 17:20:43 LoadBlockIndexDB: hashBestChain=0000000004733f08015733b2f9cd0240f44bbcdbd3cc4b3c0a69dd7aa2e887d1 height=266612 date=2018-02-04 21:06:59 progress=0.997570
2018-02-06 17:20:43 init message: Rewinding blocks...
2018-02-06 17:20:43 - Disconnect block: 4.36ms
2018-02-06 17:20:43 UpdateTip: new best=0000000011243509ab23fe80b3b808061a383d152bdbb8f6731d4e7901167cf1  height=266611  log2_work=52.691153  tx=2361539  date=2018-02-04 21:05:51 progress=0.997569  cache=1.0MiB(38tx)
2018-02-06 17:20:43 - Disconnect block: 2.21ms
2018-02-06 17:20:43 UpdateTip: new best=0000000013b9282703a3eb7de2c5f1b97830d7ef8b6501d429352d9ff877aee8  height=266610  log2_work=52.691143  tx=2361532  date=2018-02-04 21:05:01 progress=0.997569  cache=1.2MiB(53tx)
...
2018-02-06 17:45:13 UpdateTip: new best=000000000273e32ac2c8e9a0b22d52fe628056e3b61d001721dbe27172ca6746  height=237146  log2_work=52.329446  tx=1810681  date=2017-12-15 10:59:58 progress=0.912876  cache=109.5MiB(2888tx)
2018-02-06 17:45:13 - Disconnect block: 25.96ms
@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Feb 6, 2018

Ugh, related to above, even zcash-cli stop results in Rewinding blocks so the process can't be stopped now...

This enables us to distinguish between it being unset vs. being set to zero.
@str4d str4d force-pushed the str4d:2286-nu-activation-mechanism branch from 8b0b78f to 828940b Feb 6, 2018
@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Feb 6, 2018

There was a bug in the boost::optional commit: the genesis block is side-loaded in when a new chain is created, so it never gets validity status set, which meant its branch ID wasn't getting correctly cached in-memory. Force-pushed to fix the commit.

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Feb 6, 2018

The bug in the boost::optional commit would have been detected, because it broke the RPC tests. I confirmed via sampling that the RPC tests were unbroken by the fix.

@daira daira changed the title Network upgrade activation mechanism Network upgrade activation mechanis Feb 6, 2018
@daira
daira approved these changes Feb 6, 2018
Copy link
Contributor

daira left a comment

utACK changes.

@str4d str4d changed the title Network upgrade activation mechanis Network upgrade activation mechanism Feb 6, 2018
@str4d str4d requested a review from bitcartel Feb 7, 2018
@ebfull
ebfull approved these changes Feb 7, 2018
@str4d str4d dismissed bitcartel’s stale review Feb 7, 2018

Confirmed in Slack that the problem is resolved.

@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Feb 7, 2018

Thanks everyone!

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 7, 2018

📌 Commit cad27eb has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 7, 2018

⌛️ Testing commit cad27eb with merge e685057...

zkbot added a commit that referenced this pull request Feb 7, 2018
Network upgrade activation mechanism

Implements ZIP 200.

Integration with `CChainParams` inspired by bitcoin/bitcoin#7575.

Includes block index rewinding logic cherry-picked from bitcoin/bitcoin#8149.

Closes #2286. Part of #2905.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Feb 7, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing e685057 to master...

@zkbot zkbot merged commit cad27eb into zcash:master Feb 7, 2018
1 check passed
1 check passed
homu Test successful
Details
Network Upgrade 0 automation moved this from Work Queue to Complete Feb 7, 2018
@str4d str4d deleted the str4d:2286-nu-activation-mechanism branch Feb 7, 2018
zkbot added a commit that referenced this pull request Feb 12, 2018
[WIP] Overwinter transaction format

Early feedback on this PR is needed, specifically commit 0e003a7. This PR depends on #2898
Related ZIP for Overwinter transaction format is zcash/zips#133

Closes #2906.
@studyjavapython

This comment has been minimized.

Copy link

studyjavapython commented Nov 11, 2018

Who knowns which branchId is?Someone just tell me it can get from the api of 'getblock'. Thanks.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.