Have createNewBlock() return a BlockTemplate interface #30440

In commit "Have createNewBlock return BlockTemplate interface" (4af3d9a483e4107301f0f0a42da781dfa79acb82)

Should be possible to avoid getBlockHeader calls by just using the block variable:

--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -941,7 +941,7 @@ static RPCHelpMan getblocktemplate()
     result.pushKV("vbavailable", std::move(vbavailable));
     result.pushKV("vbrequired", int(0));
 
-    result.pushKV("previousblockhash", block_template->getBlockHeader().hashPrevBlock.GetHex());
+    result.pushKV("previousblockhash", block.hashPrevBlock.GetHex());
     result.pushKV("transactions", std::move(transactions));
     result.pushKV("coinbaseaux", std::move(aux));
     result.pushKV("coinbasevalue", (int64_t)block.vtx[0]->vout[0].nValue);
@@ -963,8 +963,8 @@ static RPCHelpMan getblocktemplate()
     if (!fPreSegWit) {
         result.pushKV("weightlimit", (int64_t)MAX_BLOCK_WEIGHT);
     }
-    result.pushKV("curtime", block_template->getBlockHeader().GetBlockTime());
-    result.pushKV("bits", strprintf("%08x", block_template->getBlockHeader().nBits));
+    result.pushKV("curtime", block.GetBlockTime());
+    result.pushKV("bits", strprintf("%08x", block.nBits));
     result.pushKV("height", (int64_t)(pindexPrev->nHeight+1));
 
     if (consensusParams.signet_blocks) {

I only used getBlockHeader() here to test the implementation, but it's indeed not really useful in this context.

In commit "Add getCoinbaseMerklePath() to Mining interface" (1f5f16d166f8944d532aadc382b30f68a06720e2)

Instead of implementing this function in node/interfaces.cpp file it would be better to add a standalone GetCoinBaseMerklePath(const CBlock&block) function or a new CBlock method that the interface method could call. This would keep the interfaces.cpp file simple, and let it just contain glue code instead of more complicated functionality. It would also make the merkle path functionality easier to test, and more accessible to code not using the interface classes.

In commit "Add GetCoinBaseMerklePath helper" (c230cc1a72dc4e4079cb2f7b05586e6f32e844ab)

May want to declare this static to avoid creating an unused link symbol.

In commit "Have createNewBlock return BlockTemplate interface" (b4782696a8ec5e21a41db3702d4b4dcc90ea40dd)

Could you move these member declarations to the bottom of the class? It makes it easier to see what state the interface accesses when state is declared at beginning or end of the class, not in the middle mixed with method definitions.

I'm also wondering if it might be more efficient to avoid the call to CBlock::GetBlockHeader():

--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -843,14 +843,10 @@ class BlockTemplateImpl : public BlockTemplate
 {
 public:
     explicit BlockTemplateImpl(std::unique_ptr<CBlockTemplate> block_template) : m_block_template(std::move(block_template)) {
-        m_block_header = m_block_template->block.GetBlockHeader();
     }
 
-    std::unique_ptr<CBlockTemplate> m_block_template;
-    CBlockHeader m_block_header;
-
     CBlockHeader getBlockHeader() override {
-        return m_block_header;
+        return Assert(m_block_template)->block;
     }
 
     CBlock getBlock() override
@@ -883,6 +879,7 @@ public:
         return GetWitnessCommitmentIndex(m_block_template->block);
     }
 
+    std::unique_ptr<CBlockTemplate> m_block_template;
 };
 
 class MinerImpl : public Mining

Do you mean return Assert(m_block_template)->block.GetBlockHeader()?

The reason I made it a member was so that the interface would return it upon constructing BlockTemplate. This would then save multiple round trips over the interface later when we need a bunch of details from the header.

Do you mean return Assert(m_block_template)->block.GetBlockHeader()?

No I think the diff above should work. There should be no need for calling GetBlockHeader() at all, much less caching the result of the call, because CBlock inherits from CBlockHeader.

I'm not suggesting dropping the getBlockHeader interface method, because that method is useful for avoiding round trips like you suggest.

because CBlock inherits from CBlockHeader

Ah ok

that method is useful for avoiding round trips

The choice is between having the caller be careful to avoid round trips, vs not needing round trips at all because we already got the header at construction time.

That's likely a tiny difference, so whatever is better code is probably best.

I took the patch.

In commit "Have createNewBlock return BlockTemplate interface" (6536b345a346670b8733e5372f988452727a2554)

I don't see any way this condition could be true, now or previously. Would suggest changing this to assert(block_template) or CHECK_NONFATAL(block_template)

Same comment also applies to createNewBlock calls below on lines 374 and 817.

The check here also appears to be dead code. I think it would be good to remove, or replace with an assert, maybe in a separate commit.

The only way this should fail is coinbase_script is invalid (e.g. too large). But that currently results in a throw std::runtime_error inside BlockAssembler::CreateNewBlock.

Changed the three lines to CHECK_NONFATAL.

Added commit to drop the dead code.

In commit "Have createNewBlock return BlockTemplate interface" (6536b345a346670b8733e5372f988452727a2554)

Would be good to guard against undefined behavior by adding assert(m_block_template); in the constructor body and declaring m_block_template as const unique_ptr to prevent it being reset to null.

If this is done, the Assert in getBlockHeader could also be removed so it is more consistent with the other methods.

Done

In commit "Have createNewBlock return BlockTemplate interface" (f434c8562281dfec1bab37e7232ca35fd759e25f)

While changing this it would make sense to add std::move() on line 148 below so the block data does not need to be copied there.

Changed

In commit "Have createNewBlock return BlockTemplate interface" (977ab92f8040cd139fc4b969baeec39ef58c00af)

Want to note that at some point, depending on how this interface is used, it might be a good idea to consolidate individual accessor methods into more general ones that return structs or tuples to return multiple values at the same time.

For example, if getTxFees() and getTxSigops() are typically called one after another over IPC it could make sense to combine them into a getTxInfo() function to save a round trip.

I'm not sure if we end using getTxFees() and getTxSigops() in stratum v2, or only for the existing stratum v1 RPC calls. Once sv2 code is a bit more mature it's indeed worth going over the interface once more.

Does this need to be public?

private:
    const std::unique_ptr<CBlockTemplate> m_block_template;

Does this need to be public?

Convention in interface.cpp files is to make class members public since the classes are private and meant to be a minimal as possible (ideally just containing accessors and forwarding methods). Comment at top of the file mentions this:

https://github.com/bitcoin/bitcoin/blob/5d280130446d57d653c749005a2e363265d87686/src/node/interfaces.cpp#L82-L83

Likely a dumb question, but is switching from a CBlock pointer to using getBlock which returns a value directly going to have much of a performance impact (lots of copying)?

Likely a dumb question, but is switching from a CBlock pointer to using getBlock which returns a value directly going to have much of a performance impact (lots of copying)?

Definitely not a dumb question, this change results in an extra copy (only a move is happening on this line, but there is a copy inside the getBlock() function, but a copy is also removed later on due to adding std::move in GenerateBlock, so the net number of copies should be the same.

Definitely not a dumb question, this change results in an extra copy (only a move is happening on this line, but there is a copy inside the getBlock() function, but a copy is also removed later on due to adding std::move in GenerateBlock, so the net number of copies should be the same.

I don't understand this comment. This line is part of the getblocktemplate RPC, which is a real RPC (possibly used in production). Whereas the GenerateBlock function is a test-only function for the test-only RPCs generate*.

So it looks like this is adding an extra copy in production code so that a copy can be removed in test-only code.

Generally, production code should not be adjusted to accommodate tests, but rather the other way round. So doing the inverse here would have been better.

Am I missing something obvious?

Edit: I guess the block is later (unconditionally) serialized as UniValue, so adding or removing a copy here shouldn't change the overall performance.

Marking as unresolved so we don't lose track of it.

(I edited my comment, because it seems that the block is fully serialized into json anyway, which should be way more expensive than a copy, so probably fine, but I haven't benchmarked this)

Sorry, I didn't look at where GenerateBlock function was being called, I just saw one copy being added and another being removed and assumed net number of copies was the same. Agree that cost of copying should not be significant relative to other things.

Nit: Alphabetical order for the includes. There are some other minor clang-format issues, maybe run the commit through clang-format-diff? As far as I know we also don't have a tool to check the includes of these interface headers, but it is possible to check them manually:

interfaces/mining.h should add these lines:
#include <stdint.h>                  // for int64_t
#include <vector>                    // for vector
namespace node { struct BlockCreateOptions; }

interfaces/mining.h should remove these lines:
- #include <node/types.h>  // lines 11-11
- class CBlock;  // lines 22-22
- class CBlockHeader;  // lines 23-23

The full include-list for interfaces/mining.h:
#include <consensus/amount.h>        // for CAmount
#include <primitives/block.h>        // for CBlock, CBlockHeader
#include <primitives/transaction.h>  // for CTransactionRef
#include <stdint.h>                  // for int64_t
#include <uint256.h>                 // for uint256
#include <memory>                    // for unique_ptr, shared_ptr
#include <optional>                  // for optional
#include <vector>                    // for vector
class BlockValidationState;  // lines 21-21
class CScript;  // lines 24-24
namespace node { struct BlockCreateOptions; }
namespace node { struct NodeContext; }  // lines 18-18
---