Skip to content

Commit

Permalink
Merge bitcoin/bitcoin#31196: Prune mining interface
Browse files Browse the repository at this point in the history 
c991cea Remove processNewBlock() from mining interface (Sjors Provoost)
9a47852 Remove getTransactionsUpdated() from mining interface (Sjors Provoost)
bfc4e02 Remove testBlockValidity() from mining interface (Sjors Provoost)

Pull request description:

  There are three methods in the mining interface that can be dropped. The Template Provider doesn't need them and other application should probably not use them either.

  1. `processNewBlock()` was added in 7b4d324, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d324.

  Dropping it was suggested in bitcoin/bitcoin#30200 (comment)

  2. `getTransactionsUpdated()`: this is used in the implementation of #31003 `waitFeesChanged`. It's not very useful generically because the mempool updates very frequently.

  3. `testBlockValidity()`: it might be useful for mining application to have a way to check the validity of a block template they modified, but the Stratum v2 Template Provider doesn't do that, and this method is a bit brittle (e.g. the block needs to build on the tip).

ACKs for top commit:
  TheCharlatan:
    Re-ACK c991cea
  ryanofsky:
    Code review ACK c991cea. Since last review, just rebased to avoid conflicts in surrounding code, and edited a commit message
  tdb3:
    code review ACK c991cea

Tree-SHA512: 2138e54f920b26e01c068b24498c6a210c5c4358138dce0702ab58185d9ae148a18f04c97ac9f043646d40f8031618d80a718a176b1ce4779c237de6fb9c4a67
  • Loading branch information
@ryanofsky
ryanofsky committed Dec 18, 2024
2 parents ea53568 + c991cea commit fa0c473
Show file tree
Hide file tree
Showing 5 changed files with 15 additions and 67 deletions.
25 changes: 0 additions & 25 deletions src/interfaces/mining.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,31 +93,6 @@ class Mining
*/
virtual std::unique_ptr<BlockTemplate> 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<const CBlock>& 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;

/**
* 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; }
Expand Down
3 changes: 0 additions & 3 deletions src/ipc/capnp/mining.capnp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +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);
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") {
Expand Down
23 changes: 0 additions & 23 deletions src/node/interfaces.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -979,29 +979,6 @@ class MinerImpl : public Mining
return BlockRef{chainman().ActiveChain().Tip()->GetBlockHash(), chainman().ActiveChain().Tip()->nHeight};
}

bool processNewBlock(const std::shared_ptr<const CBlock>& block, bool* new_block) override
{
return chainman().ProcessNewBlock(block, /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/new_block);
}

unsigned int getTransactionsUpdated() override
{
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<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override
{
BlockAssembler::Options assemble_options{options};
Expand Down
29 changes: 14 additions & 15 deletions src/rpc/mining.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ static RPCHelpMan getnetworkhashps()
};
}

static bool GenerateBlock(ChainstateManager& chainman, Mining& miner, CBlock&& block, uint64_t& max_tries, std::shared_ptr<const CBlock>& block_out, bool process_new_block)
static bool GenerateBlock(ChainstateManager& chainman, CBlock&& block, uint64_t& max_tries, std::shared_ptr<const CBlock>& block_out, bool process_new_block)
{
block_out.reset();
block.hashMerkleRoot = BlockMerkleRoot(block);
Expand All @@ -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");
}

Expand All @@ -166,7 +166,7 @@ static UniValue generateBlocks(ChainstateManager& chainman, Mining& miner, const
CHECK_NONFATAL(block_template);

std::shared_ptr<const CBlock> 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;
}

Expand Down Expand Up @@ -384,16 +384,17 @@ 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()));
}
}

std::shared_ptr<const CBlock> 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.");
}

Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -742,6 +743,7 @@ static RPCHelpMan getblocktemplate()
}

static unsigned int nTransactionsUpdatedLast;
const CTxMemPool& mempool = EnsureMemPool(node);

if (!lpval.isNull())
{
Expand Down Expand Up @@ -772,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);
}
Expand Down Expand Up @@ -803,13 +805,13 @@ static RPCHelpMan getblocktemplate()
static int64_t time_start;
static std::unique_ptr<BlockTemplate> 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();

Expand Down Expand Up @@ -1032,13 +1034,10 @@ static RPCHelpMan submitblock()
}
}

NodeContext& node = EnsureAnyNodeContext(request.context);
Mining& miner = EnsureMining(node);

bool new_block;
auto sc = std::make_shared<submitblock_StateCatcher>(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";
Expand Down
2 changes: 1 addition & 1 deletion test/functional/rpc_generate.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down

0 comments on commit fa0c473

Please sign in to comment.