Mining: Avoid copying template CBlocks

[luke-jr]

Refactoring to avoid unnecessary copies/complexity, at least in the mainnet paths.

Also abstracts out a new TemplateToJSON function to keep track of variable scoping better.

[DrahtBot]

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/32547.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #33421 (node: add BlockTemplateCache by ismaelsadeeq)
• #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[DrahtBot]

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/runs/42427675996
LLM reason (✨ experimental): The CI failure is caused by linting errors, including spelling mistakes and duplicate includes.}

Hints

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.

[Sjors]

Concept ACK on adding a TemplateToJSON helper, as well as the new alias for BlockAssembler::Options.

This breaks the multiprocess build somehow, cc @ryanofsky, e.g.

[06:00:12.877] /ci_container_base/src/interfaces/mining.h:54:34: note: unimplemented pure virtual method 'getCoinbaseMerklePath' in 'ProxyClient'
[06:00:12.877]    54 |     virtual std::vector<uint256> getCoinbaseMerklePath() const = 0;

[ryanofsky]

I think goals of this PR should be achievable without too much work, but unfortunately the implementation as of 39a3b5b isn't really compatible with IPC so a different approach needs to be taken. There are two problems currently:

1. First, the pure virtual methods cannot be marked const. In general it doesn't make sense to mark virtual methods const, because parent classes can't know how subclasses will implement the virtual methods and what state they may access. The point of pure virtual methods is usually to declare abstract interfaces without leaking information about implementations or making assumptions about them.

2. Methods called over IPC can't return raw pointers or references by default, without additional custom code. It's fine for IPC methods to return ordinary primitive or struct values, or shared_ptr or unique_ptr values, but generated IPC methods like getBlock and getCoinbaseCommitment can't automatically return raw pointers or references, because after they receive the block data or coinbase commitment data over the socket, then need to return values that can be safely accessed and cleaned up by callers. They can't just return references directly to the received socket data because that data will no longer be available after the methods return. So if they return references they need to copy the data somewhere where it won't be freed right away, but will be freed eventually (to not leak memory), and that requires writing custom code and tracking additional state.

I also don't know motivation for TemplateToJSON and BlockCreateOptions changes in this PR:

• I think the point of making the CBlockTemplate::Options struct distinct from the BlockCreateOptions struct, while inheritiing from it, was that BlockCreateOptions was supposed to contain high-level options that were exposed to IPC clients, while CBlockTemplate::Options was supposed to contain lower-level options, not exposed externally over IPC, but could be used for internal code and testing. Current PR seems to collapse that distinction, and it is unclear what the goal of the change is.

• The TemplateToJSON function seems like a nice way to break up code but seems like it should just be taking input data directly instead of an interface pointer that needs to be actively queried. The could make it simpler internally and also allow it to be easily called from contexts where there is only local data and no IPC.

All these things together suggest to me that the Mining interface should probably be exposing the CBlockTemplate struct directly and not hiding it behind so many accessor methods. This should allow dropping methods, simplifying the interface, and also making it more efficient by avoiding copies when it is called locally and no IPC is involved.

The simplest way to do this would be to add new two accessor methods to interfaces::BlockTemplate like:

    virtual const CBlockTemplate* getTemplate() { return nullptr; }
    virtual CBlockTemplate getTemplateData() = 0;

which could be implemented in node::BlockTemplateImpl as:

    const CBlockTemplate* getTemplate() override { return m_block_template.get(); }
    CBlockTemplate getTemplateData() override { return *m_block_template; }

Then if the mining interface is being called from inside the local process, getTemplate() will return a valid pointer which can be used directly. But if the mining interface is being called from a remote process getTemplate() will return null, and getTemplateData() can be used to request and copy the data.

This should work, but would require clients to take call getTemplate() before getTemplateData() to be as efficient as possible and avoid unnecessary copies.

An alternate approach that would avoid unnecessary copies and be simpler for clients but require a little more complexity in the IPC implementation would be to expose an interfaces::BlockTemplate method like

    virtual const CBlockTemplate& getTemplate() = 0;

with a custom implementation that caches the CBlockTemplate internally in the IPC client class so the references it returns will be valid and the data will be freed when the client is destroyed. There are a few examples of this pattern in #10102, but implementing this would requires using some more complicated c++ features like template specialization so would probably recommend trying the simpler approach first.

[Sjors]

All these things together suggest to me that the Mining interface should probably be exposing the CBlockTemplate struct directly and not hiding it behind so many accessor methods.

The original reason for this was so that a Stratum v2 IPC sidecar can fetch the minimal info it needs for a NewTemplate message: basically the version field, some parts of the coinbase and its merkle path. It can do this without fetching the full block.

Later on it turned out that this interface is also very useful for submitting a solution without (again) having to send a full block over the wire. Ditto for an ergonomic way to wait for fees to rise or a new tip to arrive (with multiple clients).

If we assume that Stratum v2 sidecar is running on the same machine as the node, which seems pretty likely, the original optimisation might be overkill and it might as well get the full template. However it's also easier for the sidecar implementation if it doesn't have to hold on to these templates.

An approach where the sidecar calls getTemplateData(), and we drop getBlockHeader(), getBlock(), getCoinbaseTx() and getCoinbaseCommitment() is fine by me. I'm not worried about making the non-sidecar version Sjors#68 less efficient since that approach is deprecated.

The only tricky bits are getWitnessCommitmentIndex() and getCoinbaseMerklePath(), which I'd rather not implement on the sidecar side. We could cache these values in the CBlockTemplate struct (as we do with the coinbase SegWit commitment).

(since #31283 the sidecar doesn't use getTxFees() and getTxSigops() anymore so those were already on the nomination list for deletion)

cc @ismaelsadeeq

[ryanofsky]

re: #32547 (comment)

All these things together suggest to me that the Mining interface should probably be exposing the CBlockTemplate struct directly and not hiding it behind so many accessor methods.

Sorry, I did not write that very clearly. I am not suggesting dropping the interfaces::BlockTemplate class and only exposing the CBlockTemplate struct. I am just suggesting adding a const CBlockTemplate* accessor to the interface which could return a pointer to the internal struct and avoid copying. The new accessor could replace some of the other accessors, or just augment them.

This would just be a minor change to avoid unnecessary copies, not a broader rethinking of the interface.

[Sjors]

I agree it could just be an addition to the interface, though it also fits well if we want to prune it a bit.

[luke-jr]

In general it doesn't make sense to mark virtual methods const, because parent classes can't know how subclasses will implement the virtual methods and what state they may access. The point of pure virtual methods is usually to declare abstract interfaces without leaking information about implementations or making assumptions about them.

The constness of methods is a property of the interface. It is a way to say calling it will not change the object, and thereby allows using const references.

[ryanofsky]

The constness of methods is a property of the interface. It is a way to say calling it will not change the object

Yes, I understand the intent of marking interfaces::BlockTemplate methods const is to say calling those methods will not change the object. And I agree using const to prevent changing the object would be a nice goal, but unfortunately that's not what const keyword does here.

Marking the methods const does not mark the block template data const or do anything that would prevent modifying it. It only makes the interface itself, which is stateless, const, and prevents subclasses implementing the interface from modifying their internal state while not preventing them from modifying CBlockTemplate data. Specifically it makes it impossible for the IPC subclass to update its IPC state, and does nothing to prevent the non-IPC subclass from modifying the block template because it is accessed through a non-const pointer. (It does look like that pointer could be const but that's orthogonal to whether these methods are const.

[Dorex45]

Consistent use of const correctness and references //
nice improvement for clarity and performance!

[DrahtBot]

🐙 This pull request conflicts with the target branch and needs rebase.

[Sjors]

NACK

If going through the Mining interface in getblocktemplate RPC code is actually a problem, i.e. the extra copy takes significant time compared to JSON serialisation), then someone can open a PR to undo that.

The checkBlock() and getBlock() (template) IPC methods are currently indirectly tested through the getblocktemplate (template) RPC. We don't want to lose that coverage, but since #33201 we have direct test coverage for the IPC interface in test/functional/interface_ipc.py. So if that's expanded to (minimally) cover checkBlock() and better cover getBlock(), I have no objection to bypassing the interface in RPC code.

I'm less excited about adding CBlockTemplate* getTemplate() since that feels like polluting the interface.

[achow101]

This PR does not seem to have attracted much attention from reviewers. As such, it does not seem important enough right now to keep it sitting idle in the list of open PRs.

Closing due to lack of interest.

[Sjors]

Since this is still being brought up, I created a regtest mempool with 800 transactions (500 kilobyte block) and had my agent analyze where bitcoind (master @ 1a2523e, release build) spends time:

Bucket	Share
Block assembly	24.52%
Final JSON write, UniValue::write*	52.26%
Tx hex serialization, EncodeHexTx	7.27%
UniValue object building, pushKV	4.83%
Template/block/vector copies	0.05%

Note that transactions themselves are not copied, just pointers. I also disabled block template caching.

I still think it's fine to open a PR that makes getblocktemplate bypass the interface for the miner.createNewBlock call and use the BlockAssembler directly. It's perhaps easier to understand. But for real performance gains, IPC seems the way forward.

Or maybe a REST version of this call. I quickly whipped up a GET /rest/blocktemplate.bin, which is about 3x faster and the response is 3x smaller.

[Sjors]

After #35675 the RPC code stops going through the mining interface. With an additional small change it can get rid of the copies, see #35675 (comment).