From bfc4e029d41ec3052d68f174565802016cb05d41 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 31 Oct 2024 11:55:13 -0700 Subject: [PATCH 1/3] Remove testBlockValidity() from mining interface It's very low level and not used by the proposed Template Provider. This method was introduced in d8a3496b5ad27bea4c79ea0344f595cc1b95f0d3 and a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed. --- src/interfaces/mining.h | 12 ------------ src/ipc/capnp/mining.capnp | 1 - src/node/interfaces.cpp | 13 ------------- src/rpc/mining.cpp | 9 +++++---- test/functional/rpc_generate.py | 2 +- 5 files changed, 6 insertions(+), 31 deletions(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index dd5cfe5ccc66b..b8ec3aed846aa 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -106,18 +106,6 @@ class Mining //! used to decide whether to make a new block template. virtual unsigned int getTransactionsUpdated() = 0; - /** - * Check a block is completely valid from start to finish. - * Only works on top of our current best block. - * Does not check proof-of-work. - * - * @param[in] block the block to validate - * @param[in] check_merkle_root call CheckMerkleRoot() - * @param[out] state details of why a block failed to validate - * @returns false if it does not build on the current tip, or any of the checks fail - */ - virtual bool testBlockValidity(const CBlock& block, bool check_merkle_root, BlockValidationState& state) = 0; - //! Get internal node context. Useful for RPC and testing, //! but not accessible across processes. virtual node::NodeContext* context() { return nullptr; } diff --git a/src/ipc/capnp/mining.capnp b/src/ipc/capnp/mining.capnp index 5af37b725ab25..3d9b12e7348ac 100644 --- a/src/ipc/capnp/mining.capnp +++ b/src/ipc/capnp/mining.capnp @@ -20,7 +20,6 @@ interface Mining $Proxy.wrap("interfaces::Mining") { createNewBlock @4 (options: BlockCreateOptions) -> (result: BlockTemplate); processNewBlock @5 (context :Proxy.Context, block: Data) -> (newBlock: Bool, result: Bool); getTransactionsUpdated @6 (context :Proxy.Context) -> (result: UInt32); - testBlockValidity @7 (context :Proxy.Context, block: Data, checkMerkleRoot: Bool) -> (state: BlockValidationState, result: Bool); } interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") { diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 68fc17aa70103..7a98f402b9651 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -989,19 +989,6 @@ class MinerImpl : public Mining return context()->mempool->GetTransactionsUpdated(); } - bool testBlockValidity(const CBlock& block, bool check_merkle_root, BlockValidationState& state) override - { - LOCK(cs_main); - CBlockIndex* tip{chainman().ActiveChain().Tip()}; - // Fail if the tip updated before the lock was taken - if (block.hashPrevBlock != tip->GetBlockHash()) { - state.Error("Block does not connect to current chain tip."); - return false; - } - - return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, tip, /*fCheckPOW=*/false, check_merkle_root); - } - std::unique_ptr createNewBlock(const BlockCreateOptions& options) override { BlockAssembler::Options assemble_options{options}; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 5516e1c0d9991..1870636ab1415 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -384,9 +384,10 @@ static RPCHelpMan generateblock() RegenerateCommitments(block, chainman); { + LOCK(::cs_main); BlockValidationState state; - if (!miner.testBlockValidity(block, /*check_merkle_root=*/false, state)) { - throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("testBlockValidity failed: %s", state.ToString())); + if (!TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) { + throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", state.ToString())); } } @@ -709,12 +710,12 @@ static RPCHelpMan getblocktemplate() return "duplicate-inconclusive"; } - // testBlockValidity only supports blocks built on the current Tip + // TestBlockValidity only supports blocks built on the current Tip if (block.hashPrevBlock != tip) { return "inconclusive-not-best-prevblk"; } BlockValidationState state; - miner.testBlockValidity(block, /*check_merkle_root=*/true, state); + TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/true); return BIP22ValidationResult(state); } diff --git a/test/functional/rpc_generate.py b/test/functional/rpc_generate.py index 68de9006644b5..74f31f45716e0 100755 --- a/test/functional/rpc_generate.py +++ b/test/functional/rpc_generate.py @@ -87,7 +87,7 @@ def test_generateblock(self): txid1 = miniwallet.send_self_transfer(from_node=node)['txid'] utxo1 = miniwallet.get_utxo(txid=txid1) rawtx2 = miniwallet.create_self_transfer(utxo_to_spend=utxo1)['hex'] - assert_raises_rpc_error(-25, 'testBlockValidity failed: bad-txns-inputs-missingorspent', self.generateblock, node, address, [rawtx2, txid1]) + assert_raises_rpc_error(-25, 'TestBlockValidity failed: bad-txns-inputs-missingorspent', self.generateblock, node, address, [rawtx2, txid1]) self.log.info('Fail to generate block with txid not in mempool') missing_txid = '0000000000000000000000000000000000000000000000000000000000000000' From 9a47852d88cf79a94ea2b7884f2dfa319bf37e4d Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 31 Oct 2024 11:58:18 -0700 Subject: [PATCH 2/3] Remove getTransactionsUpdated() from mining interface It's unnecessary to expose it via this interface. --- src/interfaces/mining.h | 4 ---- src/ipc/capnp/mining.capnp | 1 - src/node/interfaces.cpp | 5 ----- src/rpc/mining.cpp | 7 ++++--- 4 files changed, 4 insertions(+), 13 deletions(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index b8ec3aed846aa..2f5f8285aeab6 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -102,10 +102,6 @@ class Mining */ virtual bool processNewBlock(const std::shared_ptr& block, bool* new_block) = 0; - //! Return the number of transaction updates in the mempool, - //! used to decide whether to make a new block template. - virtual unsigned int getTransactionsUpdated() = 0; - //! Get internal node context. Useful for RPC and testing, //! but not accessible across processes. virtual node::NodeContext* context() { return nullptr; } diff --git a/src/ipc/capnp/mining.capnp b/src/ipc/capnp/mining.capnp index 3d9b12e7348ac..6f9f2fe31c118 100644 --- a/src/ipc/capnp/mining.capnp +++ b/src/ipc/capnp/mining.capnp @@ -19,7 +19,6 @@ interface Mining $Proxy.wrap("interfaces::Mining") { waitTipChanged @3 (context :Proxy.Context, currentTip: Data, timeout: Float64) -> (result: Common.BlockRef); createNewBlock @4 (options: BlockCreateOptions) -> (result: BlockTemplate); processNewBlock @5 (context :Proxy.Context, block: Data) -> (newBlock: Bool, result: Bool); - getTransactionsUpdated @6 (context :Proxy.Context) -> (result: UInt32); } interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") { diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 7a98f402b9651..a5a62c2654a9b 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -984,11 +984,6 @@ class MinerImpl : public Mining return chainman().ProcessNewBlock(block, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/new_block); } - unsigned int getTransactionsUpdated() override - { - return context()->mempool->GetTransactionsUpdated(); - } - std::unique_ptr createNewBlock(const BlockCreateOptions& options) override { BlockAssembler::Options assemble_options{options}; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 1870636ab1415..3b6435f3f183e 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -743,6 +743,7 @@ static RPCHelpMan getblocktemplate() } static unsigned int nTransactionsUpdatedLast; + const CTxMemPool& mempool = EnsureMemPool(node); if (!lpval.isNull()) { @@ -773,7 +774,7 @@ static RPCHelpMan getblocktemplate() tip = miner.waitTipChanged(hashWatchedChain, checktxtime).hash; // Timeout: Check transactions for update // without holding the mempool lock to avoid deadlocks - if (miner.getTransactionsUpdated() != nTransactionsUpdatedLastLP) + if (mempool.GetTransactionsUpdated() != nTransactionsUpdatedLastLP) break; checktxtime = std::chrono::seconds(10); } @@ -804,13 +805,13 @@ static RPCHelpMan getblocktemplate() static int64_t time_start; static std::unique_ptr block_template; if (!pindexPrev || pindexPrev->GetBlockHash() != tip || - (miner.getTransactionsUpdated() != nTransactionsUpdatedLast && GetTime() - time_start > 5)) + (mempool.GetTransactionsUpdated() != nTransactionsUpdatedLast && GetTime() - time_start > 5)) { // Clear pindexPrev so future calls make a new block, despite any failures from here on pindexPrev = nullptr; // Store the pindexBest used before createNewBlock, to avoid races - nTransactionsUpdatedLast = miner.getTransactionsUpdated(); + nTransactionsUpdatedLast = mempool.GetTransactionsUpdated(); CBlockIndex* pindexPrevNew = chainman.m_blockman.LookupBlockIndex(tip); time_start = GetTime(); From c991cea1a0c3ea99dc3e3919789ddf32a338a59d Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 31 Oct 2024 11:58:44 -0700 Subject: [PATCH 3/3] Remove processNewBlock() from mining interface processNewBlock was added in 7b4d3249ced93ec5986500e43b324005ed89502f, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d3249ced93ec5986500e43b324005ed89502f. getTransactionsUpdated() is only needed by the implementation of waitFeesChanged() (not yet part of the interface). --- src/interfaces/mining.h | 9 --------- src/ipc/capnp/mining.capnp | 1 - src/node/interfaces.cpp | 5 ----- src/rpc/mining.cpp | 13 +++++-------- 4 files changed, 5 insertions(+), 23 deletions(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index 2f5f8285aeab6..bc5955ded6628 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -93,15 +93,6 @@ class Mining */ virtual std::unique_ptr createNewBlock(const node::BlockCreateOptions& options = {}) = 0; - /** - * Processes new block. A valid new block is automatically relayed to peers. - * - * @param[in] block The block we want to process. - * @param[out] new_block A boolean which is set to indicate if the block was first received via this call - * @returns If the block was processed, independently of block validity - */ - virtual bool processNewBlock(const std::shared_ptr& block, bool* new_block) = 0; - //! Get internal node context. Useful for RPC and testing, //! but not accessible across processes. virtual node::NodeContext* context() { return nullptr; } diff --git a/src/ipc/capnp/mining.capnp b/src/ipc/capnp/mining.capnp index 6f9f2fe31c118..50b07bda708b7 100644 --- a/src/ipc/capnp/mining.capnp +++ b/src/ipc/capnp/mining.capnp @@ -18,7 +18,6 @@ interface Mining $Proxy.wrap("interfaces::Mining") { getTip @2 (context :Proxy.Context) -> (result: Common.BlockRef, hasResult: Bool); waitTipChanged @3 (context :Proxy.Context, currentTip: Data, timeout: Float64) -> (result: Common.BlockRef); createNewBlock @4 (options: BlockCreateOptions) -> (result: BlockTemplate); - processNewBlock @5 (context :Proxy.Context, block: Data) -> (newBlock: Bool, result: Bool); } interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") { diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index a5a62c2654a9b..552df81e3c3af 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -979,11 +979,6 @@ class MinerImpl : public Mining return BlockRef{chainman().ActiveChain().Tip()->GetBlockHash(), chainman().ActiveChain().Tip()->nHeight}; } - bool processNewBlock(const std::shared_ptr& block, bool* new_block) override - { - return chainman().ProcessNewBlock(block, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/new_block); - } - std::unique_ptr createNewBlock(const BlockCreateOptions& options) override { BlockAssembler::Options assemble_options{options}; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 3b6435f3f183e..3b05f84eee4f6 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -131,7 +131,7 @@ static RPCHelpMan getnetworkhashps() }; } -static bool GenerateBlock(ChainstateManager& chainman, Mining& miner, CBlock&& block, uint64_t& max_tries, std::shared_ptr& block_out, bool process_new_block) +static bool GenerateBlock(ChainstateManager& chainman, CBlock&& block, uint64_t& max_tries, std::shared_ptr& block_out, bool process_new_block) { block_out.reset(); block.hashMerkleRoot = BlockMerkleRoot(block); @@ -151,7 +151,7 @@ static bool GenerateBlock(ChainstateManager& chainman, Mining& miner, CBlock&& b if (!process_new_block) return true; - if (!miner.processNewBlock(block_out, nullptr)) { + if (!chainman.ProcessNewBlock(block_out, /*force_processing=*/true, /*min_pow_checked=*/true, nullptr)) { throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock, block not accepted"); } @@ -166,7 +166,7 @@ static UniValue generateBlocks(ChainstateManager& chainman, Mining& miner, const CHECK_NONFATAL(block_template); std::shared_ptr block_out; - if (!GenerateBlock(chainman, miner, block_template->getBlock(), nMaxTries, block_out, /*process_new_block=*/true)) { + if (!GenerateBlock(chainman, block_template->getBlock(), nMaxTries, block_out, /*process_new_block=*/true)) { break; } @@ -394,7 +394,7 @@ static RPCHelpMan generateblock() std::shared_ptr block_out; uint64_t max_tries{DEFAULT_MAX_TRIES}; - if (!GenerateBlock(chainman, miner, std::move(block), max_tries, block_out, process_new_block) || !block_out) { + if (!GenerateBlock(chainman, std::move(block), max_tries, block_out, process_new_block) || !block_out) { throw JSONRPCError(RPC_MISC_ERROR, "Failed to make block."); } @@ -1034,13 +1034,10 @@ static RPCHelpMan submitblock() } } - NodeContext& node = EnsureAnyNodeContext(request.context); - Mining& miner = EnsureMining(node); - bool new_block; auto sc = std::make_shared(block.GetHash()); CHECK_NONFATAL(chainman.m_options.signals)->RegisterSharedValidationInterface(sc); - bool accepted = miner.processNewBlock(blockptr, /*new_block=*/&new_block); + bool accepted = chainman.ProcessNewBlock(blockptr, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/&new_block); CHECK_NONFATAL(chainman.m_options.signals)->UnregisterSharedValidationInterface(sc); if (!new_block && accepted) { return "duplicate";