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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
124 changes: 124 additions & 0 deletions src/consensus/upgrades.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
// 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(
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 200:
//
// ACTIVATION_HEIGHT
// The non-zero block height at which the network upgrade rules will come
// into effect, and be enforced as part of the blockchain consensus.
//
// For removal of ambiguity, the block at height ACTIVATION_HEIGHT - 1 is
// subject to the pre-upgrade consensus rules, and would be the last common
// block in the event of a persistent pre-upgrade branch.
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.

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
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;
}
Loading