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

Introduce deploymentstatus #19438

Merged
merged 12 commits into from
Jul 1, 2021
Merged
6 changes: 4 additions & 2 deletions src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ BITCOIN_CORE_H = \
core_memusage.h \
cuckoocache.h \
dbwrapper.h \
deploymentinfo.h \
deploymentstatus.h \
external_signer.h \
flatfile.h \
fs.h \
Expand Down Expand Up @@ -272,7 +274,6 @@ BITCOIN_CORE_H = \
validation.h \
validationinterface.h \
versionbits.h \
versionbitsinfo.h \
wallet/bdb.h \
wallet/coincontrol.h \
wallet/coinselection.h \
Expand Down Expand Up @@ -328,6 +329,7 @@ libbitcoin_server_a_SOURCES = \
chain.cpp \
consensus/tx_verify.cpp \
dbwrapper.cpp \
deploymentstatus.cpp \
flatfile.cpp \
httprpc.cpp \
httpserver.cpp \
Expand Down Expand Up @@ -540,6 +542,7 @@ libbitcoin_common_a_SOURCES = \
compressor.cpp \
core_read.cpp \
core_write.cpp \
deploymentinfo.cpp \
external_signer.cpp \
init/common.cpp \
key.cpp \
Expand All @@ -561,7 +564,6 @@ libbitcoin_common_a_SOURCES = \
script/sign.cpp \
script/signingprovider.cpp \
script/standard.cpp \
versionbitsinfo.cpp \
warnings.cpp \
$(BITCOIN_CORE_H)

Expand Down
2 changes: 1 addition & 1 deletion src/chainparams.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@

#include <chainparamsseeds.h>
#include <consensus/merkle.h>
#include <deploymentinfo.h>
#include <hash.h> // for signet block challenge hash
#include <util/system.h>
#include <versionbitsinfo.h>

#include <assert.h>

Expand Down
34 changes: 32 additions & 2 deletions src/consensus/params.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,25 @@

namespace Consensus {

enum DeploymentPos
enum BuriedDeployment : int16_t
{
// buried deployments get negative values to avoid overlap with DeploymentPos
DEPLOYMENT_HEIGHTINCB = std::numeric_limits<int16_t>::min(),
DEPLOYMENT_CLTV,
DEPLOYMENT_DERSIG,
DEPLOYMENT_CSV,
DEPLOYMENT_SEGWIT,
};
constexpr bool ValidDeployment(BuriedDeployment dep) { return DEPLOYMENT_HEIGHTINCB <= dep && dep <= DEPLOYMENT_SEGWIT; }
Copy link
Contributor

Choose a reason for hiding this comment

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

very minor nit: If you add a DEPLOYMENT_BURIED_MAX and then use dep < DEPLOYMENT_BURIED_MAX instead of dep <= DEPLOYMENT_SEGWIT then this line wouldn't need to change when something is buried. But I am not sure if it's an improvement overall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't want to add a DEPLOYMENT_BURIED_MAX since that would then need to be included in any switch statements.


enum DeploymentPos : uint16_t
{
DEPLOYMENT_TESTDUMMY,
DEPLOYMENT_TAPROOT, // Deployment of Schnorr/Taproot (BIPs 340-342)
// NOTE: Also add new deployments to VersionBitsDeploymentInfo in versionbits.cpp
// NOTE: Also add new deployments to VersionBitsDeploymentInfo in deploymentinfo.cpp
MAX_VERSION_BITS_DEPLOYMENTS
};
constexpr bool ValidDeployment(DeploymentPos dep) { return DEPLOYMENT_TESTDUMMY <= dep && dep <= DEPLOYMENT_TAPROOT; }
Copy link
Contributor

Choose a reason for hiding this comment

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

in 8b2aa89

nit: couldn't this be < MAX_VERSION_BITS_DEPLOYMENTS to avoid changing this line when adding/burring a deployment?

Suggested change
constexpr bool ValidDeployment(DeploymentPos dep) { return DEPLOYMENT_TESTDUMMY <= dep && dep <= DEPLOYMENT_TAPROOT; }
constexpr bool ValidDeployment(DeploymentPos dep) { return DEPLOYMENT_TESTDUMMY <= dep && dep < MAX_VERSION_BITS_DEPLOYMENTS; }


/**
* Struct for each individual consensus rule change using BIP9.
Expand Down Expand Up @@ -100,7 +112,25 @@ struct Params {
*/
bool signet_blocks{false};
std::vector<uint8_t> signet_challenge;

int DeploymentHeight(BuriedDeployment dep) const
{
switch (dep) {
case DEPLOYMENT_HEIGHTINCB:
return BIP34Height;
case DEPLOYMENT_CLTV:
return BIP65Height;
case DEPLOYMENT_DERSIG:
return BIP66Height;
case DEPLOYMENT_CSV:
return CSVHeight;
case DEPLOYMENT_SEGWIT:
return SegwitHeight;
} // no default case, so the compiler can warn about missing cases
return std::numeric_limits<int>::max();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could use an optional as return value instead of this to signal if no height was found?

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 would mean checking and dereferencing the optional at every call site, which doesn't seem useful.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe turn this dead code into an assert(false)? Crashing the node should be preferred over a consensus failure.

}
};

} // namespace Consensus

#endif // BITCOIN_CONSENSUS_PARAMS_H
36 changes: 36 additions & 0 deletions src/deploymentinfo.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright (c) 2016-2020 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <deploymentinfo.h>

#include <consensus/params.h>

const struct VBDeploymentInfo VersionBitsDeploymentInfo[Consensus::MAX_VERSION_BITS_DEPLOYMENTS] = {
{
/*.name =*/ "testdummy",
/*.gbt_force =*/ true,
},
{
/*.name =*/ "taproot",
/*.gbt_force =*/ true,
},
};

std::string DeploymentName(Consensus::BuriedDeployment dep)
{
assert(ValidDeployment(dep));
switch (dep) {
case Consensus::DEPLOYMENT_HEIGHTINCB:
return "bip34";
case Consensus::DEPLOYMENT_CLTV:
return "bip65";
case Consensus::DEPLOYMENT_DERSIG:
return "bip66";
case Consensus::DEPLOYMENT_CSV:
return "csv";
case Consensus::DEPLOYMENT_SEGWIT:
return "segwit";
} // no default case, so the compiler can warn about missing cases
return "";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could have given a default that provides more context like "unknown"

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't matter too much, as it is dead code. Empty string might even make it easier to run CHECK_NONFATAL(!name.empty()); in rpc code.

Copy link
Member

Choose a reason for hiding this comment

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

Or just assert(false) for symmetry with the assert at the beginning of the function?

}
29 changes: 29 additions & 0 deletions src/deploymentinfo.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright (c) 2016-2018 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#ifndef BITCOIN_DEPLOYMENTINFO_H
#define BITCOIN_DEPLOYMENTINFO_H

#include <consensus/params.h>

#include <string>

struct VBDeploymentInfo {
/** Deployment name */
const char *name;
/** Whether GBT clients can safely ignore this rule in simplified usage */
bool gbt_force;
};

extern const VBDeploymentInfo VersionBitsDeploymentInfo[Consensus::MAX_VERSION_BITS_DEPLOYMENTS];

std::string DeploymentName(Consensus::BuriedDeployment dep);

inline std::string DeploymentName(Consensus::DeploymentPos pos)
{
assert(Consensus::ValidDeployment(pos));
return VersionBitsDeploymentInfo[pos].name;
jnewbery marked this conversation as resolved.
Show resolved Hide resolved
}

#endif // BITCOIN_DEPLOYMENTINFO_H
17 changes: 17 additions & 0 deletions src/deploymentstatus.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Copyright (c) 2020 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <deploymentstatus.h>

#include <consensus/params.h>
#include <versionbits.h>

VersionBitsCache g_versionbitscache;

/* Basic sanity checking for BuriedDeployment/DeploymentPos enums and
* ValidDeployment check */

static_assert(ValidDeployment(Consensus::DEPLOYMENT_TESTDUMMY), "sanity check of DeploymentPos failed (TESTDUMMY not valid)");
static_assert(!ValidDeployment(Consensus::MAX_VERSION_BITS_DEPLOYMENTS), "sanity check of DeploymentPos failed (MAX value considered valid)");
static_assert(!ValidDeployment(static_cast<Consensus::BuriedDeployment>(Consensus::DEPLOYMENT_TESTDUMMY)), "sanity check of BuriedDeployment failed (overlaps with DeploymentPos)");
55 changes: 55 additions & 0 deletions src/deploymentstatus.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Copyright (c) 2020 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#ifndef BITCOIN_DEPLOYMENTSTATUS_H
#define BITCOIN_DEPLOYMENTSTATUS_H

#include <chain.h>
#include <versionbits.h>

#include <limits>

/** Global cache for versionbits deployment status */
extern VersionBitsCache g_versionbitscache;

/** Determine if a deployment is active for the next block */
inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::BuriedDeployment dep)
Copy link
Contributor

Choose a reason for hiding this comment

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

This dispatch scares me a bit unless we use enum class -- enums could coerce poorly right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope; int (and the like) won't automatically coerce to enum, so with int x = Consensus::DEPLOYMENT_SEGWIT; return DeploymentActiveAt(pindex, params, x) you get errors:

validation.cpp:1921:12: error: no matching function for call to 'DeploymentActiveAt'
    return DeploymentActiveAt(pindex, consensusparams, x);
           ^~~~~~~~~~~~~~~~~~
./deploymentstatus.h:32:13: note: candidate function not viable: no known conversion from 'int' to 'Consensus::BuriedDeployment' for 3rd argument
inline bool DeploymentActiveAt(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::BuriedDeployment dep)
            ^
./deploymentstatus.h:37:13: note: candidate function not viable: no known conversion from 'int' to 'Consensus::DeploymentPos' for 3rd argument
inline bool DeploymentActiveAt(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::DeploymentPos dep) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
            ^

(even if it did, which enum it coerced to is ambiguous between the two types, resulting in a failure like error: call to 'DeploymentActiveAt' is ambiguous)

{
assert(Consensus::ValidDeployment(dep));
return (pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1) >= params.DeploymentHeight(dep);
Copy link
Contributor

@fjahr fjahr Apr 3, 2021

Choose a reason for hiding this comment

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

nit: You could do !pindexPrev instead of == nullptr I think

}

inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos dep)
ajtowns marked this conversation as resolved.
Show resolved Hide resolved
{
assert(Consensus::ValidDeployment(dep));
return ThresholdState::ACTIVE == g_versionbitscache.State(pindexPrev, params, dep);
}

/** Determine if a deployment is active for this block */
inline bool DeploymentActiveAt(const CBlockIndex& index, const Consensus::Params& params, Consensus::BuriedDeployment dep)
{
assert(Consensus::ValidDeployment(dep));
return index.nHeight >= params.DeploymentHeight(dep);
}

inline bool DeploymentActiveAt(const CBlockIndex& index, const Consensus::Params& params, Consensus::DeploymentPos dep)
{
assert(Consensus::ValidDeployment(dep));
return DeploymentActiveAfter(index.pprev, params, dep);
}

/** Determine if a deployment is enabled (can ever be active) */
inline bool DeploymentEnabled(const Consensus::Params& params, Consensus::BuriedDeployment dep)
{
assert(Consensus::ValidDeployment(dep));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This check is repeated three times only to protect DeploymentHeight() afaict, maybe move it into DeploymentHeight()?

return params.DeploymentHeight(dep) != std::numeric_limits<int>::max();
}

inline bool DeploymentEnabled(const Consensus::Params& params, Consensus::DeploymentPos dep)
{
assert(Consensus::ValidDeployment(dep));
return params.vDeployments[dep].nTimeout != 0;
maflcko marked this conversation as resolved.
Show resolved Hide resolved
}

#endif // BITCOIN_DEPLOYMENTSTATUS_H
3 changes: 2 additions & 1 deletion src/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <chain.h>
#include <chainparams.h>
#include <compat/sanity.h>
#include <deploymentstatus.h>
#include <fs.h>
#include <hash.h>
#include <httprpc.h>
Expand Down Expand Up @@ -1587,7 +1588,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
}
}

if (chainparams.GetConsensus().SegwitHeight != std::numeric_limits<int>::max()) {
if (DeploymentEnabled(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) {
// Advertise witness capabilities.
// The option to not set NODE_WITNESS is only used in the tests and should be removed.
nLocalServices = ServiceFlags(nLocalServices | NODE_WITNESS);
Expand Down
7 changes: 4 additions & 3 deletions src/miner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <consensus/merkle.h>
#include <consensus/tx_verify.h>
#include <consensus/validation.h>
#include <deploymentstatus.h>
#include <policy/feerate.h>
#include <policy/policy.h>
#include <pow.h>
Expand Down Expand Up @@ -120,7 +121,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
assert(pindexPrev != nullptr);
nHeight = pindexPrev->nHeight + 1;

pblock->nVersion = ComputeBlockVersion(pindexPrev, chainparams.GetConsensus());
pblock->nVersion = g_versionbitscache.ComputeBlockVersion(pindexPrev, chainparams.GetConsensus());
// -regtest only: allow overriding block.nVersion with
// -blockversion=N to test forking scenarios
if (chainparams.MineBlocksOnDemand())
Expand All @@ -137,12 +138,12 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
// This is only needed in case the witness softfork activation is reverted
// (which would require a very deep reorganization).
// Note that the mempool would accept transactions with witness data before
// IsWitnessEnabled, but we would only ever mine blocks after IsWitnessEnabled
// the deployment is active, but we would only ever mine blocks after activation
// unless there is a massive block reorganization with the witness softfork
// not activated.
// TODO: replace this with a call to main to assess validity of a mempool
// transaction (which in most cases can be a no-op).
fIncludeWitness = IsWitnessEnabled(pindexPrev, chainparams.GetConsensus());
fIncludeWitness = DeploymentActiveAfter(pindexPrev, chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT);

int nPackagesSelected = 0;
int nDescendantsUpdated = 0;
Expand Down
9 changes: 5 additions & 4 deletions src/net_processing.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <blockfilter.h>
#include <chainparams.h>
#include <consensus/validation.h>
#include <deploymentstatus.h>
#include <hash.h>
#include <index/blockfilterindex.h>
#include <merkleblock.h>
Expand Down Expand Up @@ -997,7 +998,7 @@ void PeerManagerImpl::FindNextBlocksToDownload(NodeId nodeid, unsigned int count
// We consider the chain that this peer is on invalid.
return;
}
if (!State(nodeid)->fHaveWitness && IsWitnessEnabled(pindex->pprev, consensusParams)) {
if (!State(nodeid)->fHaveWitness && DeploymentActiveAt(*pindex, consensusParams, Consensus::DEPLOYMENT_SEGWIT)) {
// We wouldn't download this block or its descendants from this peer.
return;
}
Expand Down Expand Up @@ -1467,7 +1468,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha
return;
nHighestFastAnnounce = pindex->nHeight;

bool fWitnessEnabled = IsWitnessEnabled(pindex->pprev, m_chainparams.GetConsensus());
bool fWitnessEnabled = DeploymentActiveAt(*pindex, m_chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT);
uint256 hashBlock(pblock->GetHash());

{
Expand Down Expand Up @@ -2082,7 +2083,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const Peer& peer,
while (pindexWalk && !m_chainman.ActiveChain().Contains(pindexWalk) && vToFetch.size() <= MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
if (!(pindexWalk->nStatus & BLOCK_HAVE_DATA) &&
!IsBlockRequested(pindexWalk->GetBlockHash()) &&
(!IsWitnessEnabled(pindexWalk->pprev, m_chainparams.GetConsensus()) || State(pfrom.GetId())->fHaveWitness)) {
(!DeploymentActiveAt(*pindexWalk, m_chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT) || State(pfrom.GetId())->fHaveWitness)) {
// We don't have this block, and it's not yet in flight.
vToFetch.push_back(pindexWalk);
}
Expand Down Expand Up @@ -3397,7 +3398,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
return;
}

if (IsWitnessEnabled(pindex->pprev, m_chainparams.GetConsensus()) && !nodestate->fSupportsDesiredCmpctVersion) {
if (DeploymentActiveAt(*pindex, m_chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT) && !nodestate->fSupportsDesiredCmpctVersion) {
// Don't bother trying to process compact blocks from v1 peers
// after segwit activates.
return;
Expand Down
3 changes: 2 additions & 1 deletion src/node/interfaces.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <banman.h>
#include <chain.h>
#include <chainparams.h>
#include <deploymentstatus.h>
#include <external_signer.h>
#include <init.h>
#include <interfaces/chain.h>
Expand Down Expand Up @@ -692,7 +693,7 @@ class ChainImpl : public Chain
{
LOCK(::cs_main);
const CBlockIndex* tip = Assert(m_node.chainman)->ActiveChain().Tip();
return VersionBitsState(tip, Params().GetConsensus(), Consensus::DEPLOYMENT_TAPROOT, versionbitscache) == ThresholdState::ACTIVE;
return DeploymentActiveAfter(tip, Params().GetConsensus(), Consensus::DEPLOYMENT_TAPROOT);
}
NodeContext& m_node;
};
Expand Down
Loading