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
2 changes: 2 additions & 0 deletions src/Makefile.am
Expand Up @@ -138,6 +138,7 @@ BITCOIN_CORE_H = \
compressor.h \
consensus/consensus.h \
consensus/params.h \
consensus/upgrades.h \
consensus/validation.h \
core_io.h \
core_memusage.h \
Expand Down Expand Up @@ -354,6 +355,7 @@ libbitcoin_common_a_SOURCES = \
chainparams.cpp \
coins.cpp \
compressor.cpp \
consensus/upgrades.cpp \
core_read.cpp \
core_write.cpp \
hash.cpp \
Expand Down
1 change: 1 addition & 0 deletions src/Makefile.gtest.include
Expand Up @@ -33,6 +33,7 @@ zcash_gtest_SOURCES += \
gtest/test_random.cpp \
gtest/test_rpc.cpp \
gtest/test_transaction.cpp \
gtest/test_upgrades.cpp \
gtest/test_validation.cpp \
gtest/test_circuit.cpp \
gtest/test_txid.cpp \
Expand Down
24 changes: 24 additions & 0 deletions src/chain.h
Expand Up @@ -94,8 +94,14 @@ enum BlockStatus: uint32_t {
BLOCK_FAILED_VALID = 32, //! stage after last reached validness failed
BLOCK_FAILED_CHILD = 64, //! descends from failed block
BLOCK_FAILED_MASK = BLOCK_FAILED_VALID | BLOCK_FAILED_CHILD,

BLOCK_ACTIVATES_UPGRADE = 128, //! block activates a network upgrade
};

//! Short-hand for the highest consensus validity we implement.
//! Blocks with this validity are assumed to satisfy all consensus rules.
static const BlockStatus BLOCK_VALID_CONSENSUS = BLOCK_VALID_SCRIPTS;

/** The block chain is a tree shaped structure starting with the
* genesis block at the root, with each block potentially having multiple
* candidates to be the next block. A blockindex may have multiple pprev pointing
Expand Down Expand Up @@ -140,6 +146,11 @@ class CBlockIndex
//! Verification status of this block. See enum BlockStatus
unsigned int nStatus;

//! Branch ID corresponding to the consensus rules used to validate this block.
//! Only cached if block validity is BLOCK_VALID_CONSENSUS.
//! Persisted at each activation height, memory-only for intervening blocks.
boost::optional<uint32_t> nCachedBranchId;

//! The anchor for the tree state up to the start of this block
uint256 hashAnchor;

Expand Down Expand Up @@ -180,6 +191,7 @@ class CBlockIndex
nTx = 0;
nChainTx = 0;
nStatus = 0;
nCachedBranchId = boost::none;
hashAnchor = uint256();
hashAnchorEnd = uint256();
nSequenceId = 0;
Expand Down Expand Up @@ -341,6 +353,18 @@ class CDiskBlockIndex : public CBlockIndex
READWRITE(VARINT(nDataPos));
if (nStatus & BLOCK_HAVE_UNDO)
READWRITE(VARINT(nUndoPos));
if (nStatus & BLOCK_ACTIVATES_UPGRADE) {
if (ser_action.ForRead()) {
uint32_t branchId;
READWRITE(branchId);
nCachedBranchId = branchId;
} else {
// nCachedBranchId must always be set if BLOCK_ACTIVATES_UPGRADE is set.
assert(nCachedBranchId);
uint32_t branchId = *nCachedBranchId;
READWRITE(branchId);
}
}
READWRITE(hashAnchor);

// block header
Expand Down
31 changes: 31 additions & 0 deletions src/chainparams.cpp
Expand Up @@ -90,6 +90,13 @@ class CMainParams : public CChainParams {
consensus.nPowMaxAdjustDown = 32; // 32% adjustment down
consensus.nPowMaxAdjustUp = 16; // 16% adjustment up
consensus.nPowTargetSpacing = 2.5 * 60;
consensus.vUpgrades[Consensus::BASE_SPROUT].nActivationHeight =
Consensus::NetworkUpgrade::ALWAYS_ACTIVE;
consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nActivationHeight =
Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight =
Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;

/**
* The message start string should be awesome! ⓩ❤
*/
Expand Down Expand Up @@ -241,6 +248,13 @@ class CTestNetParams : public CChainParams {
consensus.nPowMaxAdjustDown = 32; // 32% adjustment down
consensus.nPowMaxAdjustUp = 16; // 16% adjustment up
consensus.nPowTargetSpacing = 2.5 * 60;
consensus.vUpgrades[Consensus::BASE_SPROUT].nActivationHeight =
Consensus::NetworkUpgrade::ALWAYS_ACTIVE;
consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nActivationHeight =
Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight =
Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;

pchMessageStart[0] = 0xfa;
pchMessageStart[1] = 0x1a;
pchMessageStart[2] = 0xf9;
Expand Down Expand Up @@ -341,6 +355,12 @@ class CRegTestParams : public CChainParams {
consensus.nPowMaxAdjustDown = 0; // Turn off adjustment down
consensus.nPowMaxAdjustUp = 0; // Turn off adjustment up
consensus.nPowTargetSpacing = 2.5 * 60;
consensus.vUpgrades[Consensus::BASE_SPROUT].nActivationHeight =
Consensus::NetworkUpgrade::ALWAYS_ACTIVE;
consensus.vUpgrades[Consensus::UPGRADE_TESTDUMMY].nActivationHeight =
Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
consensus.vUpgrades[Consensus::UPGRADE_OVERWINTER].nActivationHeight =
Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment, this means that all the RPC tests will default to Sprout mode. I think we should at some point change this to ALWAYS_ACTIVE (updating src/gtest/test_upgrades.cpp accordingly), and have a few specific RPC tests that disable Overwinter.


pchMessageStart[0] = 0xaa;
pchMessageStart[1] = 0xe8;
Expand Down Expand Up @@ -394,6 +414,12 @@ class CRegTestParams : public CChainParams {
vFoundersRewardAddress = { "t2FwcEhFdNXuFMv1tcYwaBJtYVtMj8b1uTg" };
assert(vFoundersRewardAddress.size() <= consensus.GetLastFoundersRewardBlockHeight());
}

void UpdateNetworkUpgradeParameters(Consensus::UpgradeIndex idx, int nActivationHeight)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

assert(idx >= 0 && idx < Consensus::MAX_NETWORK_UPGRADES);

assert(idx > Consensus::BASE_SPROUT && idx < Consensus::MAX_NETWORK_UPGRADES);
consensus.vUpgrades[idx].nActivationHeight = nActivationHeight;
}
};
static CRegTestParams regTestParams;

Expand Down Expand Up @@ -467,3 +493,8 @@ std::string CChainParams::GetFoundersRewardAddressAtIndex(int i) const {
assert(i >= 0 && i < vFoundersRewardAddress.size());
return vFoundersRewardAddress[i];
}

void UpdateNetworkUpgradeParameters(Consensus::UpgradeIndex idx, int nActivationHeight)
{
regTestParams.UpdateNetworkUpgradeParameters(idx, nActivationHeight);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

}
5 changes: 5 additions & 0 deletions src/chainparams.h
Expand Up @@ -134,4 +134,9 @@ void SelectParams(CBaseChainParams::Network network);
*/
bool SelectParamsFromCommandLine();

/**
* Allows modifying the network upgrade regtest parameters.
*/
void UpdateNetworkUpgradeParameters(Consensus::UpgradeIndex idx, int nActivationHeight);

#endif // BITCOIN_CHAINPARAMS_H
44 changes: 44 additions & 0 deletions src/consensus/params.h
Expand Up @@ -9,6 +9,49 @@
#include "uint256.h"

namespace Consensus {

/**
* Index into Params.vUpgrades and NetworkUpgradeInfo
*
* Being array indices, these MUST be numbered consecutively.
*
* The order of these indices MUST match the order of the upgrades on-chain, as
* several functions depends on the enum being sorted.
*/
enum UpgradeIndex {
// Sprout must be first
BASE_SPROUT,
UPGRADE_TESTDUMMY,
UPGRADE_OVERWINTER,
// NOTE: Also add new upgrades to NetworkUpgradeInfo in upgrades.cpp
MAX_NETWORK_UPGRADES
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add static asserts to ensure UPGRADE_SPROUT==1 and UPGRADE_SPROUT < UPGRADE_OVERWINTER etc? Just in case a developer changes thing around? Although a stronger comment to the effect of "Do not reorder these enum constants!" could also work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to add the latter. Not adding the former, per above comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to strongly-word the comment instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also add a comment that they must be consecutively numbered.

struct NetworkUpgrade {
/**
* Height of the first block for which the new consensus rules will be active
*/
int nActivationHeight;

/**
* Special value for nActivationHeight indicating that the upgrade is always active.
* This is useful for testing, as it means tests don't need to deal with the activation
* process (namely, faking a chain of somewhat-arbitrary length).
*
* New blockchains that want to enable upgrade rules from the beginning can also use
* this value. However, additional care must be taken to ensure the genesis block
* satisfies the enabled rules.
*/
static constexpr int ALWAYS_ACTIVE = 0;

/**
* Special value for nActivationHeight indicating that the upgrade will never activate.
* This is useful when adding upgrade code that has a testnet activation height, but
* should remain disabled on mainnet.
*/
static constexpr int NO_ACTIVATION_HEIGHT = -1;
};

/**
* Parameters that influence chain consensus.
*/
Expand Down Expand Up @@ -39,6 +82,7 @@ struct Params {
int nMajorityEnforceBlockUpgrade;
int nMajorityRejectBlockOutdated;
int nMajorityWindow;
NetworkUpgrade vUpgrades[MAX_NETWORK_UPGRADES];
/** Proof of work parameters */
uint256 powLimit;
int64_t nPowAveragingWindow;
Expand Down
122 changes: 122 additions & 0 deletions src/consensus/upgrades.cpp
@@ -0,0 +1,122 @@
// Copyright (c) 2018 The Zcash developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include "consensus/upgrades.h"

/**
* General information about each network upgrade.
* Ordered by Consensus::UpgradeIndex.
*/
const struct NUInfo NetworkUpgradeInfo[Consensus::MAX_NETWORK_UPGRADES] = {
{
/*.nBranchId =*/ 0,
/*.strName =*/ "Sprout",
/*.strInfo =*/ "The Zcash network at launch",
},
{
/*.nBranchId =*/ 0x74736554,
/*.strName =*/ "Test dummy",
/*.strInfo =*/ "Test dummy info",
},
{
/*.nBranchId =*/ 0x5ba81b19,
/*.strName =*/ "Overwinter",
/*.strInfo =*/ "TBD",
}
};

UpgradeState NetworkUpgradeState(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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 nHeight,
const Consensus::Params& params,
Consensus::UpgradeIndex idx)
{
assert(nHeight >= 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

assert(idx >= 0 && idx < Consensus::MAX_NETWORK_UPGRADES);

assert(idx >= Consensus::BASE_SPROUT && idx < Consensus::MAX_NETWORK_UPGRADES);
auto nActivationHeight = params.vUpgrades[idx].nActivationHeight;

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

//
// ACTIVATION_HEIGHT
// The block height at which the network upgrade rules will come into effect.
//
// For removal of ambiguity, the block at height ACTIVATION_HEIGHT - 1 is
Copy link
Contributor

Choose a reason for hiding this comment

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

So there will be no tx embargo period?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at the consensus layer, no. @zookozcash was only willing to consider it for Overwinter itself if necessary, not for the general mechanism (which is what this PR implements).

// subject to the pre-upgrade consensus rules.
return UPGRADE_ACTIVE;
} else {
return UPGRADE_PENDING;
}
}

bool NetworkUpgradeActive(
int nHeight,
const Consensus::Params& params,
Consensus::UpgradeIndex idx)
{
return NetworkUpgradeState(nHeight, params, idx) == UPGRADE_ACTIVE;
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

assert(nHeight >= 0);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't bother here because nHeight is only used as a parameter to function calls that make the same assumption, so the assertion in the underlying NetworkUpgradeState() call will trigger.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible, but per above comment, nHeight is only passed to functions that will eventually have the above assert. I can add it here as well if it is a sticking point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I really wish we were writing in a language with integer range types. (cf #627)

for (auto idxInt = Consensus::MAX_NETWORK_UPGRADES - 1; idxInt >= Consensus::BASE_SPROUT; idxInt--) {
if (NetworkUpgradeActive(nHeight, params, Consensus::UpgradeIndex(idxInt))) {
return idxInt;
}
}
}

uint32_t CurrentEpochBranchId(int nHeight, const Consensus::Params& params) {
return NetworkUpgradeInfo[CurrentEpoch(nHeight, params)].nBranchId;
}

bool IsActivationHeight(
int nHeight,
const Consensus::Params& params,
Consensus::UpgradeIndex idx)
{
assert(idx >= Consensus::BASE_SPROUT && idx < Consensus::MAX_NETWORK_UPGRADES);

// Don't count Sprout as an activation height
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just assert(idx > Consensus::UPGRADE_SPROUT && idx < Consensus::MAX_NETWORK_UPGRADES); if not counting Sprout as activation height?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That requires the caller to handle that case, and we know better here how that case should be handled (specifically, it's because Sprout wasn't actually an upgrade but is being included as one for code niceness, and this case is needed to prevent the entire chain being reindexed on first start due to missing, and unnecessary, index data).

Copy link
Contributor

Choose a reason for hiding this comment

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

ACK, makes sense.

if (idx == Consensus::BASE_SPROUT) {
return false;
}

return nHeight >= 0 && nHeight == params.vUpgrades[idx].nActivationHeight;
}

bool IsActivationHeightForAnyUpgrade(
int nHeight,
const Consensus::Params& params)
{
if (nHeight < 0) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't actually necessary, since none of the nActivationHeights will be negative.

Oh, yes it is because NO_ACTIVATION_HEIGHT could match a negative nHeight. Never mind.


// Don't count Sprout as an activation height
for (int idx = Consensus::BASE_SPROUT + 1; idx < Consensus::MAX_NETWORK_UPGRADES; idx++) {
if (nHeight == params.vUpgrades[idx].nActivationHeight)
return true;
}

return false;
}

boost::optional<int> NextActivationHeight(
int nHeight,
const Consensus::Params& params)
{
if (nHeight < 0) {
return boost::none;
}

// Don't count Sprout as an activation height
for (auto idx = Consensus::BASE_SPROUT + 1; idx < Consensus::MAX_NETWORK_UPGRADES; idx++) {
if (NetworkUpgradeState(nHeight, params, Consensus::UpgradeIndex(idx)) == UPGRADE_PENDING) {
return params.vUpgrades[idx].nActivationHeight;
}
}

return boost::none;
}