mining: add getTotalFees(), protect m_block_template with mutex #36525

This PR catches four birds with one stone:

• fixes #36515
• add a new getTotalFees() getter so IPC clients don't have to sum up individual transaction fees
• storing fees in m_total_fees simplifies the WaitAndCreateNewBlock implementation.
• make it easier to add fee info to (future) external templates, as requested in #35671 (comment)

The last commit contains the fix for #36515, which is made easier by the changes before it. It can be tested with: https://gist.github.com/Sjors/832841d705e62a6addaf8e6d4cf1b155 (the test itself didn't seem worth including, though I could look into simplifying it)

In case this PR doesn't make it, I'll open a fresh one with a direct fix.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

External sites

• Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36525
• Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36525.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36525#author-ralph

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #36451 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36451.svg">} (refactor: Use NotNull for CreateNewBlock and pimpls by maflcko)
• #36357 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36357.svg">} (mining: move BlockAssembler statics into BlockTemplateManager by w0xlt)
• #36356 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36356.svg">} (mining: move getblocktemplate cache into BlockTemplateManager by w0xlt)
• #36192 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36192.svg">} (test: cover unsatisfiable mining timestamp by Sjors)
• #36097 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36097.svg">} (mining: replace interrupt methods with cancellation arguments by xyzconstant)
• #35671 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35671.svg">} (mining: add TxCollection to bandwidth-efficiently validate external block templates by Sjors)
• #33922 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/33922.svg">} (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

ACK 4774cf1de6a4670323686bf93beb93035b3f6dad

add a new getTotalFees() getter so IPC clients don't have to sum up individual transaction fees

I thought the client needed to access the individual fees, so we return a vector. It makes sense to return it so they don't have to sum them, so the vector one is now redundant?

See minimal test that fails on our tsan functional tests.

<details>

diff --git a/test/functional/interface_ipc_mining.py b/test/functional/interface_ipc_mining.py
index c55a8f7a54..a969691596 100755
--- a/test/functional/interface_ipc_mining.py
+++ b/test/functional/interface_ipc_mining.py
@@ -41,6 +41,7 @@ from test_framework.ipc_util import (
     assert_create_new_block_fails,
     destroying,
     load_capnp_modules,
+    make_capnp_init_ctx,
     make_mining_ctx,
     mining_create_block_template,
     mining_get_block,
@@ -799,6 +800,45 @@ class IPCMiningTest(BitcoinTestFramework):

         asyncio.run(capnp.run(async_routine()))

+    def run_concurrent_template_access_test(self):
+        """Reading and submitting a template at the same time must not race."""
+        self.log.info("Running concurrent template access test")
+
+        async def async_routine():
+            # One context runs on one server thread. Two contexts run on two
+            # threads, so the read and the submit can run at the same time and race.
+            reader_ctx, init = await make_capnp_init_ctx(self)
+            writer_ctx = self.capnp_modules['proxy'].Context()
+            writer_ctx.thread = init.construct().threadMap.makeThread("pythread").result
+            mining = init.makeMining(reader_ctx).result
+
+            async with AsyncExitStack() as stack:
+                template = await mining_create_block_template(
+                    mining, stack, reader_ctx, self.default_block_create_options, cooldown=False)
+                assert template is not None
+                block = await mining_get_block(template, reader_ctx)
+                # No witness means every submit is rejected, so the tip never moves.
+                coinbase = block.vtx[0].serialize_without_witness()
+                height = self.nodes[0].getblockcount()
+
+                done = asyncio.Event()
+
+                async def read_loop():
+                    while not done.is_set():
+                        await template.getBlockHeader(reader_ctx)
+                        await template.getBlock(reader_ctx)
+
+                async def submit_loop():
+                    for _ in range(100):
+                        await template.submitSolution(
+                            writer_ctx, block.nVersion, block.nTime, block.nNonce, coinbase)
+                    done.set()
+
+                await asyncio.gather(read_loop(), submit_loop())
+                assert_equal(self.nodes[0].getblockcount(), height)
+
+        asyncio.run(capnp.run(async_routine()))
+
     def run_test(self):
         self.miniwallet = MiniWallet(self.nodes[0])
         # Amount of time in milliseconds the test is allowed to wait or be idle before it should fail.
@@ -815,6 +855,7 @@ class IPCMiningTest(BitcoinTestFramework):
         self.run_block_max_weight_test()
         self.run_ipc_option_override_test()
         self.run_transaction_lookup_test()
+        self.run_concurrent_template_access_test()

         # Needs to run last because it resets the chain.
         self.run_low_height_test()

</details>

The vector was initially introduced for (internal) compatibility with the getblocktemplate RPC, which returns this fee information.

sv2-tp only uses fees (the total) for logging, the SRI implementation doesn't use it at all.

I haven't done an inventory of software out there that uses the fees from getblocktemplate. One use case I could imagine is a miner dashboard that shows the current template you're working on, and a breakdown of fees.

We definitely need the internal total. Providing the total and individual via IPC is trivial, so I think we should keep it.

See minimal test that fails on our tsan functional tests.

It works and I'm open to taking it, but failure isn't deterministic (just very likely with the 100 iterations).

Concept ACK

It works and I'm open to taking it, but failure isn't deterministic (just very likely with the 100 iterations).

Indeed, it is not deterministic but is sure to fail at some point.

ACK 4774cf1de6a4670323686bf93beb93035b3f6dad

This change fixes a data race between ipc calls running on different worker threads, where submitSolution modifies a template while another call reads it. It also adds the getTotalFees IPC method, allowing external clients to retrieve the total fees without summing individual transaction fees.

Ran @ismaelsadeeq’s ipc test with tsan, repeating each submission setting (100, 150, 200 and 300) 25 times. Without the mutex fix, every run detected the race. With the fix, every run passed with no race reports or timeouts

I think the test is worth including. even though reproduction depends on scheduling, it consistently caught the regression on my machine (macOS arm)

Added the test in a new commit, with some adjustments:

• use timeout_factor to avoid false positive failures
• improve cleanup

reACK cbadeff334f182086538d7483dc1146c559859e6

reACK https://github.com/bitcoin/bitcoin/pull/36525/changes/cbadeff334f182086538d7483dc1146c559859e6

ACK cbadeff334

nit:

diff --git a/src/node/block_template_manager.cpp b/src/node/block_template_manager.cpp
index f4875ff4f6..310d9726b5 100644
--- a/src/node/block_template_manager.cpp
+++ b/src/node/block_template_manager.cpp
@@ -146,9 +146,9 @@ std::unique_ptr<CBlockTemplate> BlockTemplateManager::WaitAndCreateNewBlock(
             m_notifications.m_tip_block_cv.wait_until(lock, std::min(now + tick, deadline), [&]() EXCLUSIVE_LOCKS_REQUIRED(m_notifications.m_tip_block_mutex) {
                 AssertLockHeld(m_notifications.m_tip_block_mutex);
                 const auto tip_block{m_notifications.TipBlock()};
-                // We assume tip_block is set, because this is an instance
-                // method on BlockTemplate and no template could have been
-                // generated before a tip exists.
+                // We assume tip_block is set, because callers pass the parent
+                // hash of an existing template and no template could have
+                // been generated before a tip exists.
                 tip_changed = Assume(tip_block) && tip_block != hash_prev_block;
                 return tip_changed || m_chainman.m_interrupt || interrupt_wait;
             });
diff --git a/src/node/block_template_manager.h b/src/node/block_template_manager.h
index 88bdef4fa4..880af33293 100644
--- a/src/node/block_template_manager.h
+++ b/src/node/block_template_manager.h
@@ -100,6 +100,7 @@ public:
      * (on testnet3, testnet4, and regtest).
      *
      * [@param](/bitcoin-bitcoin/contributor/param/)[in] hash_prev_block Parent block hash of the previous template.
+     *                            The chain tip must already be set.
      * [@param](/bitcoin-bitcoin/contributor/param/)[in] current_fees Total transaction fees in the previous template, in satoshis.
      * [@param](/bitcoin-bitcoin/contributor/param/)[in] wait_options Timeout and minimum fee increase relative to current_fees.
      * [@param](/bitcoin-bitcoin/contributor/param/)[in] create_options Options used to create the next block template.

@w0xlt I prefer the existing wording:

callers pass the parent hash of an existing template

The function doesn't know where the caller got its hash.

Might it be preferable to make m_block_template non-mutating? Do users of the interface even need the mutated template after submitting?

AFAICT no, so we could do that and make the template const, that way no race and we copy when mutating for submission.

Might it be preferable to make m_block_template non-mutating?

I think it's a preferable solution. With no writes after construction there's no race, and since submitSolution() already copies the block, calling AddMerkleRootAndCoinbase() on that copy instead of on the template costs nothing.

The documentation and tests would need to be updated too, since the in-place mutation is documented and tested since #33745 (released in v31).

The sv2-tp client apparently relies on that mutation (SaveBlockAsync()), but it may be easy to update.

Do users of the interface even need the mutated template after submitting?

Yes. Both existing clients call getBlock() after submit to store a copy of the block. This is potentially very useful when debugging a rejected block.

The sv2-tp client apparently relies on that mutation (SaveBlockAsync()), but it may be easy to update.

Clients could download the non-mutated block and re-apply the solution themselves before storing to disk. That's slightly more complicated, so I prefer the existing approach.

Could we just return the block copy after applying the solution?

@ismaelsadeeq in theory yes, but clients intentionally wait 1 second before requesting it. If the solution is correct, we don't want to "distract" the node from relaying it and from producing the next template.

If the solution is correct, we don't want to "distract" the node from relaying it and from producing the next template.

I don't understand what you mean here. Doesn't the new valid header get processed synchronously anyway?

A call to getBlock() locks cs_main (briefly). Since sv2-tp typically runs on the same machine, when it saves to disk it also blocks the OS. Creating the new template blocks cs_main. And IIUC so does our p2p code when it announces the new block and handles compact block missing-transaction requests.

That said, I haven't done a very thorough analysis on whether the 1 second delay before getBlock() actually matters.

A call to getBlock() locks cs_main (briefly). Since sv2-tp typically runs on the same machine, when it saves to disk it also blocks the OS. Creating the new template blocks cs_main. And IIUC so does our p2p code when it announces the new block and handles compact block missing-transaction requests.

That should all be unrelated to copying a block. Or is the concern just that the OS might be something more important in the meantime and this might distract it? I don't think that is a valid concern.

I wrote:

A call to getBlock() locks cs_main (briefly).

This is not the case.

I benchmarked the approach of making the template block immutable and returning the full block in submitSolution(). The good news is that there's no regression in compact block relay, not even when a transaction is missing.

But it does add a 6ms slowdown to when sv2-tp serves the next template. That's because IPC code still bottlenecks a few things on a single thread (even with multiple connections).

I was able to reduce that by having sv2-tp skip block deserialization (including sha256 hashing), and write the serialized bytes straight to disk (after a 500ms delay). That still left 2.5ms.

Things can be sped up further by:

1. having another thread copy the result (1ms)
2. taking advantage of Capnp shared memory support: https://capnproto.org/cxxrpc.html (1ms)
3. more fundamentally making IPC (response) handling parallel

These require changes to libmultiprocess.

So my preference would be to defer making CBlockTemplate const until after these improvements land. This would be a breaking change for clients, but that's straight-forward to work around.

There's another approach, though a bit of a layer violation: we can have submitSolution() defer its response so it comes after waitNext(). But I also prefer doing that as a followup, rather than increase the scope of this PR.