interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner

ismaelsadeeq commented at 1:48 pm on April 29, 2025: member

Motivation

It’s stated that

Interface method definitions should wrap existing functionality instead of implementing new functionality. Any substantial new node or wallet functionality should be implemented in src/node/ or src/wallet/ and just exposed in src/interfaces/ instead of being implemented there, so it can be more modular and accessible to unit tests.

However the some methods in the newly added BlockTemplateImpl and MinerImpl classes partially enforces this guideline, as the implementations of the submitSolution, waitNext, and waitTipChanged methods reside within the class itself.

What the PR Does

This PR introduces a simple refactor by moving certain method implementations from BlockTemplateImpl into the miner module. It introduces three new functions:

AddMerkleRootAndCoinbase: Computes the block’s Merkle root, inserts the coinbase transaction, and sets the Merkle root in the block. This function is called by submitSolution before the block is submitted for processing.
WaitNext: Returns a new block template either when transaction fees reach a certain threshold or when a new tip is detected. If a timeout is reached, it returns nullopt. The waitNext method in BlockTemplateImpl now simply wraps this function.
ChainTipChanged: Returns true when the chain tip changes, or false if a timeout or interrupt occurs. The waitTipChanged method in MinerImpl now calls GetTip after invoking ChainTipChanged, and returns true.

Behavior Change

We now only Assert for a valid chainman and notifications pointer once at the top of the functions.

interfaces: refactor: move `submitSolution` implementation to miner

- Create a new function `AddMerkleRootAndCoinbase` that compute the
  block's merkle root, insert the coinbase transaction and the merkle
  root into the block.

d3f4e64d41

DrahtBot commented at 1:48 pm on April 29, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32378.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

different then → different than (in comment “different then current_tip provided by caller”)

callers convenience → callers’ convenience (in comment “For callers convenience, …”)

DrahtBot added the label interfaces on Apr 29, 2025

ismaelsadeeq renamed this:
~~interfaces: move `Mining` and `BlockTemplate` implementation to miner~~
interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner
on Apr 29, 2025

in src/node/miner.cpp:453 in a5145ba0fc outdated

448@@ -446,4 +449,94 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t
449     block.nNonce = nonce;
450     block.hashMerkleRoot = BlockMerkleRoot(block);
451 }
452+
453+std::optional<std::unique_ptr<CBlockTemplate>> WaitNext(NodeContext& node, const uint256& prev_block_hash, const std::vector<CAmount>& tx_fees, const BlockWaitOptions& options, const BlockAssembler::Options& assemble_options)

ryanofsky commented at 3:09 pm on April 29, 2025:

In commit “interfaces: refactor: move waitNext implementation to miner” (a5145ba0fc3339392ff71bf20613812e0d8c8efc)

This change generally looks good, but I would suggest tweaking this commit and the next one to pass Chainman& and KernelNotification& parameters directly instead of NodeContext& because NodeContext exposes a lot of node state that this code shouldn’t need access to and also brings in dependencies this code shouldn’t need to depend on.

ismaelsadeeq commented at 4:45 pm on April 29, 2025:

Thanks for taking a look I’ve taken your suggestion with the addition of CTxMemPool* as a parameter to WaitNext. I think it’s best to pass it as well, to make the method self contained and allow for testing with a custom mempool.

Also, if I use the mempool pointer in the active chainstate not the node, a miner test fails. This happens because in TestPackageSelection, we construct a new mempool and update the node interfaces’ mempool pointer, but we don’t update the raw pointer stored in the active chainstate.

DrahtBot added the label CI failed on Apr 29, 2025

DrahtBot commented at 3:22 pm on April 29, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/41351512028 LLM reason (✨ experimental): The CI failure was caused by clang-tidy generating errors due to an unused using-declaration in testnet4_miner_tests.cpp.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

interfaces: refactor: move `waitNext` implementation to miner

- We now assert for a valid chainman and notifications once at the
  beginning of the WaitNext function.

d11db291d4

interfaces: refactor: move `waitTipChanged` implementation to miner

- This commit creates a function `ChainTipChanged` that waits for the connected
  tip to change until timeout elapsed.

- This function is now used by `waitTipChanged`

c8a3fabe40

ismaelsadeeq force-pushed on Apr 29, 2025

ismaelsadeeq commented at 4:47 pm on April 29, 2025: member

Updated to fix recent comment by @ryanofsky see 5fa54e2..c8a3fab
Also fixed tidy issues

DrahtBot removed the label CI failed on Apr 29, 2025