From faadc606c7644f2934de390e261d9d65a81a7592 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 25 Nov 2021 10:59:26 +0100 Subject: [PATCH 1/3] refactor: Pass const reference instead of pointer to GetBlockScriptFlags The function dereferences the pointer and can not accept nullptr. Change the arg to a const reference to clarify this for the caller. --- src/validation.cpp | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index b0e6152b09b..1959fa2c695 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -302,7 +302,7 @@ bool CheckSequenceLocks(CBlockIndex* tip, } // Returns the script flags which should be checked for a given block -static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consensus::Params& chainparams); +static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Consensus::Params& chainparams); static void LimitMempoolSize(CTxMemPool& pool, CCoinsViewCache& coins_cache, size_t limit, std::chrono::seconds age) EXCLUSIVE_LOCKS_REQUIRED(pool.cs, ::cs_main) @@ -939,7 +939,7 @@ bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws) // There is a similar check in CreateNewBlock() to prevent creating // invalid blocks (using TestBlockValidity), however allowing such // transactions into the mempool can be exploited as a DoS attack. - unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(m_active_chainstate.m_chain.Tip(), chainparams.GetConsensus()); + unsigned int currentBlockScriptVerifyFlags{GetBlockScriptFlags(*m_active_chainstate.m_chain.Tip(), chainparams.GetConsensus())}; if (!CheckInputsFromMempoolAndCache(tx, state, m_view, m_pool, currentBlockScriptVerifyFlags, ws.m_precomputed_txdata, m_active_chainstate.CoinsTip())) { LogPrintf("BUG! PLEASE REPORT THIS! CheckInputScripts failed against latest-block but not STANDARD flags %s, %s\n", hash.ToString(), state.ToString()); @@ -1579,7 +1579,7 @@ public: static ThresholdConditionCache warningcache[VERSIONBITS_NUM_BITS] GUARDED_BY(cs_main); -static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consensus::Params& consensusparams) +static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Consensus::Params& consensusparams) { unsigned int flags = SCRIPT_VERIFY_NONE; @@ -1589,35 +1589,35 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens // mainnet and testnet), so for simplicity, always leave P2SH // on except for the one violating block. if (consensusparams.BIP16Exception.IsNull() || // no bip16 exception on this chain - pindex->phashBlock == nullptr || // this is a new candidate block, eg from TestBlockValidity() - *pindex->phashBlock != consensusparams.BIP16Exception) // this block isn't the historical exception + block_index.phashBlock == nullptr || // this is a new candidate block, eg from TestBlockValidity() + *block_index.phashBlock != consensusparams.BIP16Exception) // this block isn't the historical exception { // Enforce WITNESS rules whenever P2SH is in effect flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS; } // Enforce the DERSIG (BIP66) rule - if (DeploymentActiveAt(*pindex, consensusparams, Consensus::DEPLOYMENT_DERSIG)) { + if (DeploymentActiveAt(block_index, consensusparams, Consensus::DEPLOYMENT_DERSIG)) { flags |= SCRIPT_VERIFY_DERSIG; } // Enforce CHECKLOCKTIMEVERIFY (BIP65) - if (DeploymentActiveAt(*pindex, consensusparams, Consensus::DEPLOYMENT_CLTV)) { + if (DeploymentActiveAt(block_index, consensusparams, Consensus::DEPLOYMENT_CLTV)) { flags |= SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY; } // Enforce CHECKSEQUENCEVERIFY (BIP112) - if (DeploymentActiveAt(*pindex, consensusparams, Consensus::DEPLOYMENT_CSV)) { + if (DeploymentActiveAt(block_index, consensusparams, Consensus::DEPLOYMENT_CSV)) { flags |= SCRIPT_VERIFY_CHECKSEQUENCEVERIFY; } // Enforce Taproot (BIP340-BIP342) - if (DeploymentActiveAt(*pindex, consensusparams, Consensus::DEPLOYMENT_TAPROOT)) { + if (DeploymentActiveAt(block_index, consensusparams, Consensus::DEPLOYMENT_TAPROOT)) { flags |= SCRIPT_VERIFY_TAPROOT; } // Enforce BIP147 NULLDUMMY (activated simultaneously with segwit) - if (DeploymentActiveAt(*pindex, consensusparams, Consensus::DEPLOYMENT_SEGWIT)) { + if (DeploymentActiveAt(block_index, consensusparams, Consensus::DEPLOYMENT_SEGWIT)) { flags |= SCRIPT_VERIFY_NULLDUMMY; } @@ -1625,7 +1625,6 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens } - static int64_t nTimeCheck = 0; static int64_t nTimeForks = 0; static int64_t nTimeVerify = 0; @@ -1811,7 +1810,7 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, } // Get the script flags for this block - unsigned int flags = GetBlockScriptFlags(pindex, m_params.GetConsensus()); + unsigned int flags{GetBlockScriptFlags(*pindex, m_params.GetConsensus())}; int64_t nTime2 = GetTimeMicros(); nTimeForks += nTime2 - nTime1; LogPrint(BCLog::BENCH, " - Fork checks: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime2 - nTime1), nTimeForks * MICRO, nTimeForks * MILLI / nBlocksTotal); From fa422994116a7a053789304d56159760081479eb Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 25 Nov 2021 11:18:33 +0100 Subject: [PATCH 2/3] Remove nullptr check in GetBlockScriptFlags Commit d59b8d6aa1102ffac980c89e96105ddec9cfb579 removed the need for this check and it was never needed. --- src/validation.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 1959fa2c695..f21d6cdbf10 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1589,8 +1589,7 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Co // mainnet and testnet), so for simplicity, always leave P2SH // on except for the one violating block. if (consensusparams.BIP16Exception.IsNull() || // no bip16 exception on this chain - block_index.phashBlock == nullptr || // this is a new candidate block, eg from TestBlockValidity() - *block_index.phashBlock != consensusparams.BIP16Exception) // this block isn't the historical exception + *Assert(block_index.phashBlock) != consensusparams.BIP16Exception) // this block isn't the historical exception { // Enforce WITNESS rules whenever P2SH is in effect flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS; From cccc1e70b8a14430cc94143da97936a60d6c83d3 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 16 Nov 2021 14:40:19 +0100 Subject: [PATCH 3/3] Enforce Taproot script flags whenever WITNESS is set --- src/chainparams.cpp | 11 +++++++---- src/consensus/params.h | 11 +++++++++-- src/validation.cpp | 23 +++++++++-------------- test/functional/feature_taproot.py | 10 +++------- 4 files changed, 28 insertions(+), 27 deletions(-) diff --git a/src/chainparams.cpp b/src/chainparams.cpp index 2e823c12112..c35d265f3cf 100644 --- a/src/chainparams.cpp +++ b/src/chainparams.cpp @@ -9,6 +9,7 @@ #include #include #include // for signet block challenge hash +#include