From 4da2347e969df0414ff90ca62a9b2b52ba5c90ee Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 15 Jun 2021 00:50:17 +0100 Subject: [PATCH 1/8] Rename hashLightClientRoot to hashBlockCommitments in block header --- src/chain.h | 18 +++++++++--------- src/main.cpp | 22 +++++++++++----------- src/miner.cpp | 6 +++--- src/primitives/block.cpp | 4 ++-- src/primitives/block.h | 10 +++++----- src/rpc/mining.cpp | 4 ++-- src/test/miner_tests.cpp | 4 ++-- src/txdb.cpp | 14 +++++++------- 8 files changed, 41 insertions(+), 41 deletions(-) diff --git a/src/chain.h b/src/chain.h index 01204fdc8..4daabddee 100644 --- a/src/chain.h +++ b/src/chain.h @@ -258,7 +258,7 @@ public: //! Root of the Sapling commitment tree as of the end of this block. //! //! - For blocks prior to (not including) the Heartwood activation block, this is - //! always equal to hashLightClientRoot. + //! always equal to hashBlockCommitments. //! - For blocks including and after the Heartwood activation block, this is only set //! once a block has been connected to the main chain, and will be null otherwise. uint256 hashFinalSaplingRoot; @@ -268,13 +268,13 @@ public: //! - For blocks prior to and including the Heartwood activation block, this is //! always null. //! - For blocks after (not including) the Heartwood activation block, this is - //! always equal to hashLightClientRoot. + //! always equal to hashBlockCommitments. uint256 hashChainHistoryRoot; //! block header int nVersion; uint256 hashMerkleRoot; - uint256 hashLightClientRoot; + uint256 hashBlockCommitments; unsigned int nTime; unsigned int nBits; uint256 nNonce; @@ -307,7 +307,7 @@ public: nVersion = 0; hashMerkleRoot = uint256(); - hashLightClientRoot = uint256(); + hashBlockCommitments = uint256(); nTime = 0; nBits = 0; nNonce = uint256(); @@ -325,7 +325,7 @@ public: nVersion = block.nVersion; hashMerkleRoot = block.hashMerkleRoot; - hashLightClientRoot = block.hashLightClientRoot; + hashBlockCommitments = block.hashBlockCommitments; nTime = block.nTime; nBits = block.nBits; nNonce = block.nNonce; @@ -357,7 +357,7 @@ public: if (pprev) block.hashPrevBlock = pprev->GetBlockHash(); block.hashMerkleRoot = hashMerkleRoot; - block.hashLightClientRoot = hashLightClientRoot; + block.hashBlockCommitments = hashBlockCommitments; block.nTime = nTime; block.nBits = nBits; block.nNonce = nNonce; @@ -486,7 +486,7 @@ public: READWRITE(this->nVersion); READWRITE(hashPrev); READWRITE(hashMerkleRoot); - READWRITE(hashLightClientRoot); + READWRITE(hashBlockCommitments); READWRITE(nTime); READWRITE(nBits); READWRITE(nNonce); @@ -512,7 +512,7 @@ public: } else if (ser_action.ForRead()) { // For block indices written before the client was Heartwood-aware, // these are always identical. - hashFinalSaplingRoot = hashLightClientRoot; + hashFinalSaplingRoot = hashBlockCommitments; } // If you have just added new serialized fields above, remember to add @@ -525,7 +525,7 @@ public: block.nVersion = nVersion; block.hashPrevBlock = hashPrev; block.hashMerkleRoot = hashMerkleRoot; - block.hashLightClientRoot = hashLightClientRoot; + block.hashBlockCommitments = hashBlockCommitments; block.nTime = nTime; block.nBits = nBits; block.nNonce = nNonce; diff --git a/src/main.cpp b/src/main.cpp index d0f995086..0613f19be 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3016,15 +3016,15 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin blockundo.old_sprout_tree_root = old_sprout_tree_root; if (IsActivationHeight(pindex->nHeight, chainparams.GetConsensus(), Consensus::UPGRADE_HEARTWOOD)) { - // In the block that activates ZIP 221, block.hashLightClientRoot MUST + // In the block that activates ZIP 221, block.hashBlockCommitments MUST // be set to all zero bytes. - if (!block.hashLightClientRoot.IsNull()) { + if (!block.hashBlockCommitments.IsNull()) { return state.DoS(100, - error("ConnectBlock(): block's hashLightClientRoot is incorrect (should be null)"), + error("ConnectBlock(): block's hashBlockCommitments is incorrect (should be null)"), REJECT_INVALID, "bad-heartwood-root-in-block"); } } else if (chainparams.GetConsensus().NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_HEARTWOOD)) { - // If Heartwood is active, block.hashLightClientRoot must be the same as + // If Heartwood is active, block.hashBlockCommitments must be the same as // the root of the history tree for the previous block. We only store // one tree per epoch, so we have two possible cases: // - If the previous block is in the previous epoch, this block won't @@ -3032,17 +3032,17 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin // - If the previous block is in this epoch, this block would affect // this epoch's tree root, but as we haven't updated the tree for this // block yet, view.GetHistoryRoot() returns the root we need. - if (block.hashLightClientRoot != view.GetHistoryRoot(prevConsensusBranchId)) { + if (block.hashBlockCommitments != view.GetHistoryRoot(prevConsensusBranchId)) { return state.DoS(100, - error("ConnectBlock(): block's hashLightClientRoot is incorrect (should be history tree root)"), + error("ConnectBlock(): block's hashBlockCommitments is incorrect (should be history tree root)"), REJECT_INVALID, "bad-heartwood-root-in-block"); } } else if (chainparams.GetConsensus().NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_SAPLING)) { - // If Sapling is active, block.hashLightClientRoot must be the + // If Sapling is active, block.hashBlockCommitments must be the // same as the root of the Sapling tree - if (block.hashLightClientRoot != sapling_tree.root()) { + if (block.hashBlockCommitments != sapling_tree.root()) { return state.DoS(100, - error("ConnectBlock(): block's hashLightClientRoot is incorrect (should be Sapling tree root)"), + error("ConnectBlock(): block's hashBlockCommitments is incorrect (should be Sapling tree root)"), REJECT_INVALID, "bad-sapling-root-in-block"); } } @@ -3915,10 +3915,10 @@ CBlockIndex* AddToBlockIndex(const CBlockHeader& block, const Consensus::Params& // hashChainHistoryRoot is null. } else if (consensusParams.NetworkUpgradeActive(pindexNew->nHeight, Consensus::UPGRADE_HEARTWOOD)) { // hashFinalSaplingRoot is currently null, and will be set correctly in ConnectBlock. - pindexNew->hashChainHistoryRoot = pindexNew->hashLightClientRoot; + pindexNew->hashChainHistoryRoot = pindexNew->hashBlockCommitments; } else { // hashChainHistoryRoot is null. - pindexNew->hashFinalSaplingRoot = pindexNew->hashLightClientRoot; + pindexNew->hashFinalSaplingRoot = pindexNew->hashBlockCommitments; } pindexNew->BuildSkip(); diff --git a/src/miner.cpp b/src/miner.cpp index f0ef4ba75..af7964f0d 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -622,11 +622,11 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre // Fill in header pblock->hashPrevBlock = pindexPrev->GetBlockHash(); if (IsActivationHeight(nHeight, chainparams.GetConsensus(), Consensus::UPGRADE_HEARTWOOD)) { - pblock->hashLightClientRoot.SetNull(); + pblock->hashBlockCommitments.SetNull(); } else if (chainparams.GetConsensus().NetworkUpgradeActive(nHeight, Consensus::UPGRADE_HEARTWOOD)) { - pblock->hashLightClientRoot = view.GetHistoryRoot(prevConsensusBranchId); + pblock->hashBlockCommitments = view.GetHistoryRoot(prevConsensusBranchId); } else { - pblock->hashLightClientRoot = sapling_tree.root(); + pblock->hashBlockCommitments = sapling_tree.root(); } UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev); pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, chainparams.GetConsensus()); diff --git a/src/primitives/block.cpp b/src/primitives/block.cpp index 592d116b0..01e5ed74e 100644 --- a/src/primitives/block.cpp +++ b/src/primitives/block.cpp @@ -164,12 +164,12 @@ uint256 CBlock::BuildAuthDataMerkleTree() const std::string CBlock::ToString() const { std::stringstream s; - s << strprintf("CBlock(hash=%s, ver=%d, hashPrevBlock=%s, hashMerkleRoot=%s, hashLightClientRoot=%s, nTime=%u, nBits=%08x, nNonce=%s, vtx=%u)\n", + s << strprintf("CBlock(hash=%s, ver=%d, hashPrevBlock=%s, hashMerkleRoot=%s, hashBlockCommitments=%s, nTime=%u, nBits=%08x, nNonce=%s, vtx=%u)\n", GetHash().ToString(), nVersion, hashPrevBlock.ToString(), hashMerkleRoot.ToString(), - hashLightClientRoot.ToString(), + hashBlockCommitments.ToString(), nTime, nBits, nNonce.ToString(), vtx.size()); for (unsigned int i = 0; i < vtx.size(); i++) diff --git a/src/primitives/block.h b/src/primitives/block.h index dfba56833..8dec3f66a 100644 --- a/src/primitives/block.h +++ b/src/primitives/block.h @@ -26,7 +26,7 @@ public: int32_t nVersion; uint256 hashPrevBlock; uint256 hashMerkleRoot; - uint256 hashLightClientRoot; + uint256 hashBlockCommitments; uint32_t nTime; uint32_t nBits; uint256 nNonce; @@ -44,7 +44,7 @@ public: READWRITE(this->nVersion); READWRITE(hashPrevBlock); READWRITE(hashMerkleRoot); - READWRITE(hashLightClientRoot); + READWRITE(hashBlockCommitments); READWRITE(nTime); READWRITE(nBits); READWRITE(nNonce); @@ -56,7 +56,7 @@ public: nVersion = CBlockHeader::CURRENT_VERSION; hashPrevBlock.SetNull(); hashMerkleRoot.SetNull(); - hashLightClientRoot.SetNull(); + hashBlockCommitments.SetNull(); nTime = 0; nBits = 0; nNonce = uint256(); @@ -118,7 +118,7 @@ public: block.nVersion = nVersion; block.hashPrevBlock = hashPrevBlock; block.hashMerkleRoot = hashMerkleRoot; - block.hashLightClientRoot = hashLightClientRoot; + block.hashBlockCommitments = hashBlockCommitments; block.nTime = nTime; block.nBits = nBits; block.nNonce = nNonce; @@ -163,7 +163,7 @@ public: READWRITE(this->nVersion); READWRITE(hashPrevBlock); READWRITE(hashMerkleRoot); - READWRITE(hashLightClientRoot); + READWRITE(hashBlockCommitments); READWRITE(nTime); READWRITE(nBits); } diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 5c83af562..4818f847a 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -756,9 +756,9 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp) result.pushKV("capabilities", aCaps); result.pushKV("version", pblock->nVersion); result.pushKV("previousblockhash", pblock->hashPrevBlock.GetHex()); - result.pushKV("lightclientroothash", pblock->hashLightClientRoot.GetHex()); + result.pushKV("lightclientroothash", pblock->hashBlockCommitments.GetHex()); // Deprecated; remove in a future release. - result.pushKV("finalsaplingroothash", pblock->hashLightClientRoot.GetHex()); + result.pushKV("finalsaplingroothash", pblock->hashBlockCommitments.GetHex()); result.pushKV("transactions", transactions); if (coinbasetxn) { assert(txCoinbase.isObject()); diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index a5573d18e..82f104fb4 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -272,8 +272,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) } */ - // These tests assume null hashLightClientRoot (before Sapling) - pblock->hashLightClientRoot = uint256(); + // These tests assume null hashBlockCommitments (before Sapling) + pblock->hashBlockCommitments = uint256(); CValidationState state; BOOST_CHECK(ProcessNewBlock(state, chainparams, NULL, pblock, true, NULL)); diff --git a/src/txdb.cpp b/src/txdb.cpp index 1abdc56ca..b8396f6a3 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -549,7 +549,7 @@ bool CBlockTreeDB::LoadBlockIndexGuts( pindexNew->hashSproutAnchor = diskindex.hashSproutAnchor; pindexNew->nVersion = diskindex.nVersion; pindexNew->hashMerkleRoot = diskindex.hashMerkleRoot; - pindexNew->hashLightClientRoot = diskindex.hashLightClientRoot; + pindexNew->hashBlockCommitments = diskindex.hashBlockCommitments; pindexNew->nTime = diskindex.nTime; pindexNew->nBits = diskindex.nBits; pindexNew->nNonce = diskindex.nNonce; @@ -590,16 +590,16 @@ bool CBlockTreeDB::LoadBlockIndexGuts( // if (diskindex.nClientVersion >= CHAIN_HISTORY_ROOT_VERSION && chainParams.GetConsensus().NetworkUpgradeActive(pindexNew->nHeight, Consensus::UPGRADE_HEARTWOOD)) { - if (pindexNew->hashLightClientRoot != pindexNew->hashChainHistoryRoot) { + if (pindexNew->hashBlockCommitments != pindexNew->hashChainHistoryRoot) { return error( - "LoadBlockIndex(): block index inconsistency detected (post-Heartwood; hashLightClientRoot %s != hashChainHistoryRoot %s): %s", - pindexNew->hashLightClientRoot.ToString(), pindexNew->hashChainHistoryRoot.ToString(), pindexNew->ToString()); + "LoadBlockIndex(): block index inconsistency detected (post-Heartwood; hashBlockCommitments %s != hashChainHistoryRoot %s): %s", + pindexNew->hashBlockCommitments.ToString(), pindexNew->hashChainHistoryRoot.ToString(), pindexNew->ToString()); } } else { - if (pindexNew->hashLightClientRoot != pindexNew->hashFinalSaplingRoot) { + if (pindexNew->hashBlockCommitments != pindexNew->hashFinalSaplingRoot) { return error( - "LoadBlockIndex(): block index inconsistency detected (pre-Heartwood; hashLightClientRoot %s != hashFinalSaplingRoot %s): %s", - pindexNew->hashLightClientRoot.ToString(), pindexNew->hashFinalSaplingRoot.ToString(), pindexNew->ToString()); + "LoadBlockIndex(): block index inconsistency detected (pre-Heartwood; hashBlockCommitments %s != hashFinalSaplingRoot %s): %s", + pindexNew->hashBlockCommitments.ToString(), pindexNew->hashFinalSaplingRoot.ToString(), pindexNew->ToString()); } } } From bd4c0a8515b3220a00e9fcdd345d37728e5d1a7f Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 15 Jun 2021 13:21:09 +0100 Subject: [PATCH 2/8] ZIP 244 hashBlockCommitments implementation We will start storing two new hashes in the block index from v4.5.0: - hashAuthDataRoot - hashFinalOrchardRoot --- src/chain.h | 36 +++++++++++++++++++++++++-- src/main.cpp | 51 ++++++++++++++++++++++++++++++++++----- src/miner.cpp | 21 +++++++++++++--- src/miner.h | 9 ++++++- src/primitives/block.cpp | 14 +++++++++++ src/primitives/block.h | 5 ++++ src/rpc/mining.cpp | 7 ++++-- src/test/test_bitcoin.cpp | 2 +- src/txdb.cpp | 9 ++++++- 9 files changed, 138 insertions(+), 16 deletions(-) diff --git a/src/chain.h b/src/chain.h index 4daabddee..08b04c4f4 100644 --- a/src/chain.h +++ b/src/chain.h @@ -18,6 +18,7 @@ static const int SPROUT_VALUE_VERSION = 1001400; static const int SAPLING_VALUE_VERSION = 1010100; static const int CHAIN_HISTORY_ROOT_VERSION = 2010200; +static const int NU5_DATA_VERSION = 4050000; /** * Maximum amount of time that a block timestamp is allowed to be ahead of the @@ -231,6 +232,14 @@ public: //! Persisted at each activation height, memory-only for intervening blocks. std::optional nCachedBranchId; + //! Root of the ZIP 244 authorizing data commitment tree for this block. + //! + //! - For blocks prior to (not including) the NU5 activation block, this is always + //! null. + //! - For blocks including and after the NU5 activation block, this is only set once + //! a block has been connected to the main chain, and will be null otherwise. + uint256 hashAuthDataRoot; + //! The anchor for the tree state up to the start of this block uint256 hashSproutAnchor; @@ -263,12 +272,23 @@ public: //! once a block has been connected to the main chain, and will be null otherwise. uint256 hashFinalSaplingRoot; + //! Root of the Orchard commitment tree as of the end of this block. + //! + //! - For blocks prior to (not including) the NU5 activation block, this is always + //! null. + //! - For blocks including and after the NU5 activation block, this is only set + //! once a block has been connected to the main chain, and will be null otherwise. + uint256 hashFinalOrchardRoot; + //! Root of the ZIP 221 history tree as of the end of the previous block. //! //! - For blocks prior to and including the Heartwood activation block, this is //! always null. - //! - For blocks after (not including) the Heartwood activation block, this is - //! always equal to hashBlockCommitments. + //! - For blocks after (not including) the Heartwood activation block, and prior to + //! (not including) the NU5 activation block, this is always equal to + //! hashBlockCommitments. + //! - For blocks including and after the NU5 activation block, this is only set + //! once a block has been connected to the main chain, and will be null otherwise. uint256 hashChainHistoryRoot; //! block header @@ -297,8 +317,12 @@ public: nChainTx = 0; nStatus = 0; nCachedBranchId = std::nullopt; + hashAuthDataRoot = uint256(); hashSproutAnchor = uint256(); hashFinalSproutRoot = uint256(); + hashFinalSaplingRoot = uint256(); + hashFinalOrchardRoot = uint256(); + hashChainHistoryRoot = uint256(); nSequenceId = 0; nSproutValue = std::nullopt; nChainSproutValue = std::nullopt; @@ -515,6 +539,14 @@ public: hashFinalSaplingRoot = hashBlockCommitments; } + // Only read/write NU5 data if the client version used to create this + // index was storing them. For block indices written before the client + // was NU5-aware, these are always null / zero. + if ((s.GetType() & SER_DISK) && (nVersion >= NU5_DATA_VERSION)) { + READWRITE(hashAuthDataRoot); + READWRITE(hashFinalOrchardRoot); + } + // If you have just added new serialized fields above, remember to add // them to CBlockTreeDB::LoadBlockIndexGuts() in txdb.cpp :) } diff --git a/src/main.cpp b/src/main.cpp index 0613f19be..0c2b1a12f 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2998,6 +2998,10 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin pos.nTxOffset += ::GetSerializeSize(tx, SER_DISK, CLIENT_VERSION); } + // Derive the various block commitments. + auto hashAuthDataRoot = block.BuildAuthDataMerkleTree(); + auto hashChainHistoryRoot = view.GetHistoryRoot(prevConsensusBranchId); + view.PushAnchor(sprout_tree); view.PushAnchor(sapling_tree); if (!fJustCheck) { @@ -3012,10 +3016,37 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin if (chainparams.GetConsensus().NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_HEARTWOOD)) { pindex->hashFinalSaplingRoot = sapling_tree.root(); } + + // - If this block is before NU5 activation: + // - hashAuthDataRoot and hashFinalOrchardRoot are always null. + // - We don't set hashChainHistoryRoot here to maintain the invariant + // documented in CBlockIndex (which was ensured in AddToBlockIndex). + // - If this block is on or after NU5 activation, this is where we set + // the correct values of hashAuthDataRoot, hashFinalOrchardRoot, and + // hashChainHistoryRoot; in particular, blocks that are never passed + // to ConnectBlock() (and thus never on the main chain) will stay with + // these set to null. + if (chainparams.GetConsensus().NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_NU5)) { + pindex->hashAuthDataRoot = hashAuthDataRoot; + pindex->hashFinalOrchardRoot = uint256(); // TODO: replace with Orchard tree root + pindex->hashChainHistoryRoot = hashChainHistoryRoot; + } } blockundo.old_sprout_tree_root = old_sprout_tree_root; - if (IsActivationHeight(pindex->nHeight, chainparams.GetConsensus(), Consensus::UPGRADE_HEARTWOOD)) { + if (chainparams.GetConsensus().NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_NU5)) { + // If NU5 is active, block.hashBlockCommitments must be the top digest + // of the ZIP 244 block commitments linked list. + // https://zips.z.cash/zip-0244#block-header-changes + uint256 hashBlockCommitments = DeriveBlockCommitmentsHash( + hashChainHistoryRoot, + hashAuthDataRoot); + if (block.hashBlockCommitments != hashBlockCommitments) { + return state.DoS(100, + error("ConnectBlock(): block's hashBlockCommitments is incorrect (should be ZIP 244 block commitment)"), + REJECT_INVALID, "bad-block-commitments-hash"); + } + } else if (IsActivationHeight(pindex->nHeight, chainparams.GetConsensus(), Consensus::UPGRADE_HEARTWOOD)) { // In the block that activates ZIP 221, block.hashBlockCommitments MUST // be set to all zero bytes. if (!block.hashBlockCommitments.IsNull()) { @@ -3032,7 +3063,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin // - If the previous block is in this epoch, this block would affect // this epoch's tree root, but as we haven't updated the tree for this // block yet, view.GetHistoryRoot() returns the root we need. - if (block.hashBlockCommitments != view.GetHistoryRoot(prevConsensusBranchId)) { + if (block.hashBlockCommitments != hashChainHistoryRoot) { return state.DoS(100, error("ConnectBlock(): block's hashBlockCommitments is incorrect (should be history tree root)"), REJECT_INVALID, "bad-heartwood-root-in-block"); @@ -3910,14 +3941,22 @@ CBlockIndex* AddToBlockIndex(const CBlockHeader& block, const Consensus::Params& pindexNew->pprev = (*miPrev).second; pindexNew->nHeight = pindexNew->pprev->nHeight + 1; - if (IsActivationHeight(pindexNew->nHeight, consensusParams, Consensus::UPGRADE_HEARTWOOD)) { - // hashFinalSaplingRoot is currently null, and will be set correctly in ConnectBlock. + if (consensusParams.NetworkUpgradeActive(pindexNew->nHeight, Consensus::UPGRADE_NU5)) { + // The following hashes are currently null, and will be set correctly in + // ConnectBlock: + // - hashFinalSaplingRoot + // - hashFinalOrchardRoot + // - hashChainHistoryRoot + } else if (IsActivationHeight(pindexNew->nHeight, consensusParams, Consensus::UPGRADE_HEARTWOOD)) { + // hashFinalSaplingRoot and hashFinalOrchardRoot are currently null, and will + // be set correctly in ConnectBlock. // hashChainHistoryRoot is null. } else if (consensusParams.NetworkUpgradeActive(pindexNew->nHeight, Consensus::UPGRADE_HEARTWOOD)) { - // hashFinalSaplingRoot is currently null, and will be set correctly in ConnectBlock. + // hashFinalSaplingRoot and hashFinalOrchardRoot are currently null, and will + // be set correctly in ConnectBlock. pindexNew->hashChainHistoryRoot = pindexNew->hashBlockCommitments; } else { - // hashChainHistoryRoot is null. + // hashFinalOrchardRoot and hashChainHistoryRoot are null. pindexNew->hashFinalSaplingRoot = pindexNew->hashBlockCommitments; } diff --git a/src/miner.cpp b/src/miner.cpp index af7964f0d..65b93afa7 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -621,7 +621,12 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre // Fill in header pblock->hashPrevBlock = pindexPrev->GetBlockHash(); - if (IsActivationHeight(nHeight, chainparams.GetConsensus(), Consensus::UPGRADE_HEARTWOOD)) { + if (chainparams.GetConsensus().NetworkUpgradeActive(nHeight, Consensus::UPGRADE_NU5)) { + // hashBlockCommitments depends on the block transactions, so we have to + // update it whenever the coinbase transaction changes. Leave it unset here, + // like hashMerkleRoot, and instead cache what we will need to calculate it. + pblocktemplate->hashChainHistoryRoot = view.GetHistoryRoot(prevConsensusBranchId); + } else if (IsActivationHeight(nHeight, chainparams.GetConsensus(), Consensus::UPGRADE_HEARTWOOD)) { pblock->hashBlockCommitments.SetNull(); } else if (chainparams.GetConsensus().NetworkUpgradeActive(nHeight, Consensus::UPGRADE_HEARTWOOD)) { pblock->hashBlockCommitments = view.GetHistoryRoot(prevConsensusBranchId); @@ -682,8 +687,13 @@ void GetMinerAddress(MinerAddress &minerAddress) } } -void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned int& nExtraNonce) +void IncrementExtraNonce( + CBlockTemplate* pblocktemplate, + const CBlockIndex* pindexPrev, + unsigned int& nExtraNonce, + const Consensus::Params& consensusParams) { + CBlock *pblock = &pblocktemplate->block; // Update nExtraNonce static uint256 hashPrevBlock; if (hashPrevBlock != pblock->hashPrevBlock) @@ -699,6 +709,11 @@ void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned pblock->vtx[0] = txCoinbase; pblock->hashMerkleRoot = pblock->BuildMerkleTree(); + if (consensusParams.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_NU5)) { + pblock->hashBlockCommitments = DeriveBlockCommitmentsHash( + pblocktemplate->hashChainHistoryRoot, + pblock->BuildAuthDataMerkleTree()); + } } static bool ProcessBlockFound(const CBlock* pblock, const CChainParams& chainparams) @@ -797,7 +812,7 @@ void static BitcoinMiner(const CChainParams& chainparams) return; } CBlock *pblock = &pblocktemplate->block; - IncrementExtraNonce(pblock, pindexPrev, nExtraNonce); + IncrementExtraNonce(pblocktemplate.get(), pindexPrev, nExtraNonce, chainparams.GetConsensus()); LogPrintf("Running ZcashMiner with %u transactions in block (%u bytes)\n", pblock->vtx.size(), ::GetSerializeSize(*pblock, SER_NETWORK, PROTOCOL_VERSION)); diff --git a/src/miner.h b/src/miner.h index 4f2e87659..b54207fab 100644 --- a/src/miner.h +++ b/src/miner.h @@ -67,6 +67,9 @@ public: struct CBlockTemplate { CBlock block; + // Cached whenever we update `block`, so we can update hashBlockCommitments + // when we change the coinbase transaction. + uint256 hashChainHistoryRoot; std::vector vTxFees; std::vector vTxSigOps; }; @@ -80,7 +83,11 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre /** Get -mineraddress */ void GetMinerAddress(MinerAddress &minerAddress); /** Modify the extranonce in a block */ -void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned int& nExtraNonce); +void IncrementExtraNonce( + CBlockTemplate* pblocktemplate, + const CBlockIndex* pindexPrev, + unsigned int& nExtraNonce, + const Consensus::Params& consensusParams); /** Run the miner threads */ void GenerateBitcoins(bool fGenerate, int nThreads, const CChainParams& chainparams); #endif diff --git a/src/primitives/block.cpp b/src/primitives/block.cpp index 01e5ed74e..fe3f7590f 100644 --- a/src/primitives/block.cpp +++ b/src/primitives/block.cpp @@ -14,6 +14,20 @@ const unsigned char ZCASH_AUTH_DATA_HASH_PERSONALIZATION[BLAKE2bPersonalBytes] = {'Z','c','a','s','h','A','u','t','h','D','a','t','H','a','s','h'}; +const unsigned char ZCASH_BLOCK_COMMITMENTS_HASH_PERSONALIZATION[BLAKE2bPersonalBytes] = + {'Z','c','a','s','h','B','l','o','c','k','C','o','m','m','i','t'}; + +uint256 DeriveBlockCommitmentsHash( + uint256 hashChainHistoryRoot, + uint256 hashAuthDataRoot) +{ + // https://zips.z.cash/zip-0244#block-header-changes + CBLAKE2bWriter ss(SER_GETHASH, 0, ZCASH_BLOCK_COMMITMENTS_HASH_PERSONALIZATION); + ss << hashChainHistoryRoot; + ss << hashAuthDataRoot; + ss << uint256(); // terminator + return ss.GetHash(); +} uint256 CBlockHeader::GetHash() const { diff --git a/src/primitives/block.h b/src/primitives/block.h index 8dec3f66a..da3bd8ce1 100644 --- a/src/primitives/block.h +++ b/src/primitives/block.h @@ -10,6 +10,11 @@ #include "serialize.h" #include "uint256.h" +// Derives the ZIP 244 block commitments hash. +uint256 DeriveBlockCommitmentsHash( + uint256 hashChainHistoryRoot, + uint256 hashAuthDataRoot); + /** Nodes collect new transactions into a block, hash them into a hash tree, * and scan through nonce values to make the block's hash satisfy proof-of-work * requirements. When they solve the proof-of-work, they broadcast the block diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 4818f847a..76b6d35f8 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -218,7 +218,7 @@ UniValue generate(const UniValue& params, bool fHelp) CBlock *pblock = &pblocktemplate->block; { LOCK(cs_main); - IncrementExtraNonce(pblock, chainActive.Tip(), nExtraNonce); + IncrementExtraNonce(pblocktemplate.get(), chainActive.Tip(), nExtraNonce, Params().GetConsensus()); } // Hash state @@ -444,7 +444,8 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp) "{\n" " \"version\" : n, (numeric) The block version\n" " \"previousblockhash\" : \"xxxx\", (string) The hash of current highest block\n" - " \"lightclientroothash\" : \"xxxx\", (string) The hash of the light client root field in the block header\n" + " \"blockcommitmentshash\" : \"xxxx\", (string) The hash of the block commitments field in the block header\n" + " \"lightclientroothash\" : \"xxxx\", (string) (DEPRECATED) The hash of the light client root field in the block header\n" " \"finalsaplingroothash\" : \"xxxx\", (string) (DEPRECATED) The hash of the light client root field in the block header\n" " \"transactions\" : [ (array) contents of non-coinbase transactions that should be included in the next block\n" " {\n" @@ -756,6 +757,8 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp) result.pushKV("capabilities", aCaps); result.pushKV("version", pblock->nVersion); result.pushKV("previousblockhash", pblock->hashPrevBlock.GetHex()); + result.pushKV("blockcommitmentshash", pblock->hashBlockCommitments.GetHex()); + // Deprecated; remove in a future release. result.pushKV("lightclientroothash", pblock->hashBlockCommitments.GetHex()); // Deprecated; remove in a future release. result.pushKV("finalsaplingroothash", pblock->hashBlockCommitments.GetHex()); diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index 05eb96110..088e730cf 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -162,7 +162,7 @@ TestChain100Setup::CreateAndProcessBlock(const std::vector& block.vtx.push_back(tx); // IncrementExtraNonce creates a valid coinbase and merkleRoot unsigned int extraNonce = 0; - IncrementExtraNonce(&block, chainActive.Tip(), extraNonce); + IncrementExtraNonce(pblocktemplate, chainActive.Tip(), extraNonce, chainparams.GetConsensus()); // Hash state eh_HashState eh_state; diff --git a/src/txdb.cpp b/src/txdb.cpp index b8396f6a3..33ee323f1 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -560,7 +560,9 @@ bool CBlockTreeDB::LoadBlockIndexGuts( pindexNew->nSproutValue = diskindex.nSproutValue; pindexNew->nSaplingValue = diskindex.nSaplingValue; pindexNew->hashFinalSaplingRoot = diskindex.hashFinalSaplingRoot; + pindexNew->hashFinalOrchardRoot = diskindex.hashFinalOrchardRoot; pindexNew->hashChainHistoryRoot = diskindex.hashChainHistoryRoot; + pindexNew->hashAuthDataRoot = diskindex.hashAuthDataRoot; // Consistency checks auto header = pindexNew->GetBlockHeader(); @@ -588,7 +590,12 @@ bool CBlockTreeDB::LoadBlockIndexGuts( // a non-upgraded peer. However that case the entry will be // marked as consensus-invalid. // - if (diskindex.nClientVersion >= CHAIN_HISTORY_ROOT_VERSION && + if (diskindex.nClientVersion >= NU5_DATA_VERSION && + chainParams.GetConsensus().NetworkUpgradeActive(pindexNew->nHeight, Consensus::UPGRADE_NU5)) { + // From NU5 onwards we don't enforce a consistency check, because + // after ZIP 244, hashBlockCommitments will not match any stored + // commitment. + } else if (diskindex.nClientVersion >= CHAIN_HISTORY_ROOT_VERSION && chainParams.GetConsensus().NetworkUpgradeActive(pindexNew->nHeight, Consensus::UPGRADE_HEARTWOOD)) { if (pindexNew->hashBlockCommitments != pindexNew->hashChainHistoryRoot) { return error( From 2d0a8f1f3ac86369bf1ae879b39cdffb83247d92 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 15 Jun 2021 18:52:39 +0100 Subject: [PATCH 3/8] test: Check for valid hashBlockCommitments construction post-NU5 --- qa/pull-tester/rpc-tests.py | 1 + .../feature_zip244_blockcommitments.py | 61 +++++++++++++++++++ qa/rpc-tests/test_framework/blocktools.py | 11 ++++ src/rpc/blockchain.cpp | 2 + 4 files changed, 75 insertions(+) create mode 100755 qa/rpc-tests/feature_zip244_blockcommitments.py diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py index 67cf1268d..d451eca11 100755 --- a/qa/pull-tester/rpc-tests.py +++ b/qa/pull-tester/rpc-tests.py @@ -116,6 +116,7 @@ BASE_SCRIPTS= [ 'framework.py', 'sapling_rewind_check.py', 'feature_zip221.py', + 'feature_zip244_blockcommitments.py', 'upgrade_golden.py', 'post_heartwood_rollback.py', 'feature_logging.py', diff --git a/qa/rpc-tests/feature_zip244_blockcommitments.py b/qa/rpc-tests/feature_zip244_blockcommitments.py new file mode 100755 index 000000000..785dbf1a0 --- /dev/null +++ b/qa/rpc-tests/feature_zip244_blockcommitments.py @@ -0,0 +1,61 @@ +#!/usr/bin/env python3 +# Copyright (c) 2020 The Zcash developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or https://www.opensource.org/licenses/mit-license.php . + + +from test_framework.blocktools import derive_block_commitments_hash +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + bytes_to_hex_str, + hex_str_to_bytes, + start_nodes, +) + +TERMINATOR = b'\x00' * 32 + +# Verify block header field 'hashLightClientRoot' is set correctly for NU5 blocks. +class AuthDataRootTest(BitcoinTestFramework): + + def __init__(self): + super().__init__() + self.num_nodes = 4 + + def setup_nodes(self): + return start_nodes(self.num_nodes, self.options.tmpdir, extra_args=[[ + '-nuparams=5ba81b19:1', # Overwinter + '-nuparams=76b809bb:1', # Sapling + '-nuparams=2bb40e60:201', # Blossom + '-nuparams=f5b9230b:201', # Heartwood + '-nuparams=e9ff75a6:201', # Canopy + '-nuparams=f919a198:205', # NU5 + '-nurejectoldversions=false', + ]] * self.num_nodes) + + def run_test(self): + # Generate a block so we are on Canopy rules. + self.nodes[0].generate(2) + self.sync_all() + + # Before NU5 activation, the hashBlockCommitments field of the block + # header contains the root of the ZIP 221 history tree. + block_before = self.nodes[0].getblock('202') + assert_equal(block_before['blockcommitments'], block_before['chainhistoryroot']) + + # Generate blocks until we are on NU5 rules. + self.nodes[0].generate(3) + self.sync_all() + + # From NU5 activation, the hashBlockCommitments field of the block + # header contains a hash of various block commitments (per ZIP 244). + block_after = self.nodes[0].getblock('205') + block_commitments = bytes_to_hex_str(derive_block_commitments_hash( + hex_str_to_bytes(block_after['chainhistoryroot'])[::-1], + hex_str_to_bytes(block_after['authdataroot'])[::-1], + )[::-1]) + assert_equal(block_after['blockcommitments'], block_commitments) + + +if __name__ == '__main__': + AuthDataRootTest().main() diff --git a/qa/rpc-tests/test_framework/blocktools.py b/qa/rpc-tests/test_framework/blocktools.py index fba86848a..6cf7d8d2b 100644 --- a/qa/rpc-tests/test_framework/blocktools.py +++ b/qa/rpc-tests/test_framework/blocktools.py @@ -4,6 +4,8 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or https://www.opensource.org/licenses/mit-license.php . +from pyblake2 import blake2b + from .mininode import CBlock, CTransaction, CTxIn, CTxOut, COutPoint from .script import CScript, OP_0, OP_EQUAL, OP_HASH160, OP_TRUE, OP_CHECKSIG @@ -29,6 +31,15 @@ def create_block(hashprev, coinbase, nTime=None, nBits=None, hashFinalSaplingRoo block.calc_sha256() return block +def derive_block_commitments_hash(chain_history_root, auth_data_root): + digest = blake2b( + digest_size=32, + person=b'ZcashBlockCommit') + digest.update(chain_history_root) + digest.update(auth_data_root) + digest.update(b'\x00' * 32) + return digest.digest() + def serialize_script_num(value): r = bytearray(0) if value == 0: diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 5930b7a7e..834de7462 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -233,6 +233,8 @@ UniValue blockToJSON(const CBlock& block, const CBlockIndex* blockindex, bool tx result.pushKV("height", blockindex->nHeight); result.pushKV("version", block.nVersion); result.pushKV("merkleroot", block.hashMerkleRoot.GetHex()); + result.pushKV("blockcommitments", blockindex->hashBlockCommitments.GetHex()); + result.pushKV("authdataroot", blockindex->hashAuthDataRoot.GetHex()); result.pushKV("finalsaplingroot", blockindex->hashFinalSaplingRoot.GetHex()); result.pushKV("chainhistoryroot", blockindex->hashChainHistoryRoot.GetHex()); UniValue txs(UniValue::VARR); From d7a517465b5c055c5aadd6c4d1943365a87d15e8 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 15 Jun 2021 21:53:04 +0100 Subject: [PATCH 4/8] Skip hashBlockCommitments check when testing block templates Both hashMerkleRoot and hashAuthDataRoot are not set by the block template, and hashAuthDataRoot is an input to hashBlockCommitments. --- src/main.cpp | 27 +++++++++++++++------------ src/main.h | 3 ++- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 0c2b1a12f..a98001082 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2732,7 +2732,8 @@ static bool ShouldCheckTransactions(const CChainParams& chainparams, const CBloc } bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, - CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck) + CCoinsViewCache& view, const CChainParams& chainparams, + bool fJustCheck, bool fCheckAuthDataRoot) { AssertLockHeld(cs_main); @@ -3035,16 +3036,18 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin blockundo.old_sprout_tree_root = old_sprout_tree_root; if (chainparams.GetConsensus().NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_NU5)) { - // If NU5 is active, block.hashBlockCommitments must be the top digest - // of the ZIP 244 block commitments linked list. - // https://zips.z.cash/zip-0244#block-header-changes - uint256 hashBlockCommitments = DeriveBlockCommitmentsHash( - hashChainHistoryRoot, - hashAuthDataRoot); - if (block.hashBlockCommitments != hashBlockCommitments) { - return state.DoS(100, - error("ConnectBlock(): block's hashBlockCommitments is incorrect (should be ZIP 244 block commitment)"), - REJECT_INVALID, "bad-block-commitments-hash"); + if (fCheckAuthDataRoot) { + // If NU5 is active, block.hashBlockCommitments must be the top digest + // of the ZIP 244 block commitments linked list. + // https://zips.z.cash/zip-0244#block-header-changes + uint256 hashBlockCommitments = DeriveBlockCommitmentsHash( + hashChainHistoryRoot, + hashAuthDataRoot); + if (block.hashBlockCommitments != hashBlockCommitments) { + return state.DoS(100, + error("ConnectBlock(): block's hashBlockCommitments is incorrect (should be ZIP 244 block commitment)"), + REJECT_INVALID, "bad-block-commitments-hash"); + } } } else if (IsActivationHeight(pindex->nHeight, chainparams.GetConsensus(), Consensus::UPGRADE_HEARTWOOD)) { // In the block that activates ZIP 221, block.hashBlockCommitments MUST @@ -4604,7 +4607,7 @@ bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams, return false; if (!ContextualCheckBlock(block, state, chainparams, pindexPrev, true)) return false; - if (!ConnectBlock(block, state, &indexDummy, viewNew, chainparams, true)) + if (!ConnectBlock(block, state, &indexDummy, viewNew, chainparams, true, fCheckMerkleRoot)) return false; assert(state.IsValid()); diff --git a/src/main.h b/src/main.h index ff0f0ddd2..e45cf1ecd 100644 --- a/src/main.h +++ b/src/main.h @@ -486,7 +486,8 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, * Validity checks that depend on the UTXO set are also done; ConnectBlock() * can fail if those validity checks fail (among other reasons). */ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& coins, - const CChainParams& chainparams, bool fJustCheck = false); + const CChainParams& chainparams, + bool fJustCheck = false, bool fCheckAuthDataRoot = true); /** * Check a block is completely valid from start to finish (only works on top From da8fcedd4db7e988db785f22faa793828f943c92 Mon Sep 17 00:00:00 2001 From: str4d Date: Thu, 17 Jun 2021 16:53:44 +0100 Subject: [PATCH 5/8] Improve docs about setting CBlockIndex hash fields These fields have documented invariants, but comments in AddToBlockIndex are useful for review. --- src/main.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index a98001082..103bc6571 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3945,18 +3945,21 @@ CBlockIndex* AddToBlockIndex(const CBlockHeader& block, const Consensus::Params& pindexNew->nHeight = pindexNew->pprev->nHeight + 1; if (consensusParams.NetworkUpgradeActive(pindexNew->nHeight, Consensus::UPGRADE_NU5)) { - // The following hashes are currently null, and will be set correctly in + // The following hashes will be null if this block has never been + // connected to a main chain; they will be (re)set correctly in // ConnectBlock: // - hashFinalSaplingRoot // - hashFinalOrchardRoot // - hashChainHistoryRoot } else if (IsActivationHeight(pindexNew->nHeight, consensusParams, Consensus::UPGRADE_HEARTWOOD)) { - // hashFinalSaplingRoot and hashFinalOrchardRoot are currently null, and will - // be set correctly in ConnectBlock. + // hashFinalSaplingRoot and hashFinalOrchardRoot will be null if this block has + // never been connected to a main chain; they will be (re)set correctly in + // ConnectBlock. // hashChainHistoryRoot is null. } else if (consensusParams.NetworkUpgradeActive(pindexNew->nHeight, Consensus::UPGRADE_HEARTWOOD)) { - // hashFinalSaplingRoot and hashFinalOrchardRoot are currently null, and will - // be set correctly in ConnectBlock. + // hashFinalSaplingRoot and hashFinalOrchardRoot will be null if this block has + // never been connected to a main chain; they will be (re)set correctly in + // ConnectBlock. pindexNew->hashChainHistoryRoot = pindexNew->hashBlockCommitments; } else { // hashFinalOrchardRoot and hashChainHistoryRoot are null. From 9930a2799276fd2b56da044305e33d48c2840e18 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 17 Jun 2021 17:33:04 +0100 Subject: [PATCH 6/8] test: Check hashBlockCommitments before, at, and after NU5 activation --- .../feature_zip244_blockcommitments.py | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/qa/rpc-tests/feature_zip244_blockcommitments.py b/qa/rpc-tests/feature_zip244_blockcommitments.py index 785dbf1a0..6113f856a 100755 --- a/qa/rpc-tests/feature_zip244_blockcommitments.py +++ b/qa/rpc-tests/feature_zip244_blockcommitments.py @@ -38,23 +38,27 @@ class AuthDataRootTest(BitcoinTestFramework): self.nodes[0].generate(2) self.sync_all() - # Before NU5 activation, the hashBlockCommitments field of the block - # header contains the root of the ZIP 221 history tree. - block_before = self.nodes[0].getblock('202') - assert_equal(block_before['blockcommitments'], block_before['chainhistoryroot']) + # For blocks prior to NU5 activation, the hashBlockCommitments field of + # the block header contains the root of the ZIP 221 history tree. + for i in range(3): + block_before = self.nodes[0].getblock('%d' % (202 + i)) + assert_equal(block_before['blockcommitments'], block_before['chainhistoryroot']) - # Generate blocks until we are on NU5 rules. - self.nodes[0].generate(3) - self.sync_all() + self.nodes[0].generate(1) + self.sync_all() # From NU5 activation, the hashBlockCommitments field of the block # header contains a hash of various block commitments (per ZIP 244). - block_after = self.nodes[0].getblock('205') - block_commitments = bytes_to_hex_str(derive_block_commitments_hash( - hex_str_to_bytes(block_after['chainhistoryroot'])[::-1], - hex_str_to_bytes(block_after['authdataroot'])[::-1], - )[::-1]) - assert_equal(block_after['blockcommitments'], block_commitments) + for i in range(2): + block_after = self.nodes[0].getblock('%d' % (205 + i)) + block_commitments = bytes_to_hex_str(derive_block_commitments_hash( + hex_str_to_bytes(block_after['chainhistoryroot'])[::-1], + hex_str_to_bytes(block_after['authdataroot'])[::-1], + )[::-1]) + assert_equal(block_after['blockcommitments'], block_commitments) + + self.nodes[0].generate(1) + self.sync_all() if __name__ == '__main__': From 982c391871c872b434cfa4d5a61774c23dc6b2dc Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 17 Jun 2021 17:36:02 +0100 Subject: [PATCH 7/8] ConnectBlock: Check NU activation when deriving block commitments We compute block commitments ahead of their usage to avoid deriving them multiple times. However, we only want to derive them for blocks if they are needed; in particular, deriving hashChainHistoryRoot prior to Heartwood activation can result in an invalid empty tree being generated. --- src/main.cpp | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 103bc6571..a2ad1b2b3 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3000,8 +3000,15 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin } // Derive the various block commitments. - auto hashAuthDataRoot = block.BuildAuthDataMerkleTree(); - auto hashChainHistoryRoot = view.GetHistoryRoot(prevConsensusBranchId); + // We only derive them if they will be used for this block. + std::optional hashAuthDataRoot; + std::optional hashChainHistoryRoot; + if (chainparams.GetConsensus().NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_NU5)) { + hashAuthDataRoot = block.BuildAuthDataMerkleTree(); + } + if (chainparams.GetConsensus().NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_HEARTWOOD)) { + hashChainHistoryRoot = view.GetHistoryRoot(prevConsensusBranchId); + } view.PushAnchor(sprout_tree); view.PushAnchor(sapling_tree); @@ -3028,9 +3035,9 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin // to ConnectBlock() (and thus never on the main chain) will stay with // these set to null. if (chainparams.GetConsensus().NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_NU5)) { - pindex->hashAuthDataRoot = hashAuthDataRoot; + pindex->hashAuthDataRoot = hashAuthDataRoot.value(); pindex->hashFinalOrchardRoot = uint256(); // TODO: replace with Orchard tree root - pindex->hashChainHistoryRoot = hashChainHistoryRoot; + pindex->hashChainHistoryRoot = hashChainHistoryRoot.value(); } } blockundo.old_sprout_tree_root = old_sprout_tree_root; @@ -3041,8 +3048,8 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin // of the ZIP 244 block commitments linked list. // https://zips.z.cash/zip-0244#block-header-changes uint256 hashBlockCommitments = DeriveBlockCommitmentsHash( - hashChainHistoryRoot, - hashAuthDataRoot); + hashChainHistoryRoot.value(), + hashAuthDataRoot.value()); if (block.hashBlockCommitments != hashBlockCommitments) { return state.DoS(100, error("ConnectBlock(): block's hashBlockCommitments is incorrect (should be ZIP 244 block commitment)"), @@ -3066,7 +3073,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin // - If the previous block is in this epoch, this block would affect // this epoch's tree root, but as we haven't updated the tree for this // block yet, view.GetHistoryRoot() returns the root we need. - if (block.hashBlockCommitments != hashChainHistoryRoot) { + if (block.hashBlockCommitments != hashChainHistoryRoot.value()) { return state.DoS(100, error("ConnectBlock(): block's hashBlockCommitments is incorrect (should be history tree root)"), REJECT_INVALID, "bad-heartwood-root-in-block"); From 2d0b8b0da416a936def94b47c1eed42e7dd0431e Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 18 Jun 2021 11:20:29 +0100 Subject: [PATCH 8/8] Copy authDigest in CTransaction::operator=(const CTransaction &tx) This missing was causing `hashBlockCommitments` to be incorrectly computed in mined blocks, due to the specific way the coinbase transaction gets constructed. This went unnoticed when the default `authDigest` for legacy transactions was the null hash, but was exposed when that changed to `[0xFF; 32]`. --- src/primitives/transaction.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp index ec6e9a759..41c138ab0 100644 --- a/src/primitives/transaction.cpp +++ b/src/primitives/transaction.cpp @@ -252,6 +252,7 @@ CTransaction& CTransaction::operator=(const CTransaction &tx) { *const_cast(&joinSplitSig) = tx.joinSplitSig; *const_cast(&bindingSig) = tx.bindingSig; *const_cast(&hash) = tx.hash; + *const_cast(&authDigest) = tx.authDigest; return *this; }