From 00d1b6ef4b1203e80271c16c0d5b179525de1913 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 31 Oct 2025 09:45:33 +0100 Subject: [PATCH 1/3] doc: clarify UpdateUncommittedBlockStructures --- src/validation.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/validation.h b/src/validation.h index 291c1021dc9..cd448f3ca9e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1276,7 +1276,11 @@ public: //! ResizeCoinsCaches() as needed. void MaybeRebalanceCaches() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - /** Update uncommitted block structures (currently: only the witness reserved value). This is safe for submitted blocks. */ + /** + * Update uncommitted block structures (currently: only the witness reserved + * value). This is safe for submitted blocks as long as they honor + * default_witness_commitment from the template. + */ void UpdateUncommittedBlockStructures(CBlock& block, const CBlockIndex* pindexPrev) const; /** Produce the necessary coinbase commitment for a block (modifies the hash, don't call for mined blocks). */ From 862bd432837efeb6ab1435f75493501618ab3190 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 31 Oct 2025 11:53:12 +0100 Subject: [PATCH 2/3] mining: ensure witness commitment check in submitBlock When an IPC client requests a new block template via the Mining interface, we hold on to its CBlock. That way when they call submitSolution() we can modify it in place, rather than having to reconstruct the full block like the submitblock RPC does. Before this commit however we forgot to invalidate m_checked_witness_commitment, which we should since the client brings a new coinbase. This would cause us to accept an invalid chaintip. Fix this and add a test to confirm that we now reject such a block. As a sanity check, we add a second node to the test and confirm that will accept our mined block. Note that the IPC code takes the coinbase as provided, unlike the submitblock RPC which calls UpdateUncommittedBlockStructures() and adds witness commitment to the coinbase if it was missing. Although that could have been an alternative fix, we instead document that IPC clients are expected to provide the full coinbase including witness commitment. --- src/interfaces/mining.h | 17 +++++++++++++++-- src/node/miner.cpp | 5 +++++ test/functional/interface_ipc.py | 27 +++++++++++++++++++++++---- 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index 150295e5b7b..50a7922a8d5 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -54,9 +54,22 @@ public: virtual std::vector getCoinbaseMerklePath() = 0; /** - * Construct and broadcast the block. + * Construct and broadcast the block. Modifies the template in place, + * updating the fields listed below as well as the merkle root. * - * @returns if the block was processed, independent of block validity + * @param[in] version version block header field + * @param[in] timestamp time block header field (unix timestamp) + * @param[in] nonce nonce block header field + * @param[in] coinbase complete coinbase transaction (including witness) + * + * @note unlike the submitblock RPC, this method does NOT add the + * coinbase witness automatically. + * + * @returns if the block was processed, does not necessarily indicate validity. + * + * @note Returns true if the block is already known, which can happen if + * the solved block is constructed and broadcast by multiple nodes + * (e.g. both the miner who constructed the template and the pool). */ virtual bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) = 0; diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 28e9048a4dd..b988e28a3ff 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -452,6 +452,11 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t block.nTime = timestamp; block.nNonce = nonce; block.hashMerkleRoot = BlockMerkleRoot(block); + + // Reset cached checks + block.m_checked_witness_commitment = false; + block.m_checked_merkle_root = false; + block.fChecked = false; } std::unique_ptr WaitAndCreateNewBlock(ChainstateManager& chainman, diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py index abcc4d6b5d1..e905c7753be 100755 --- a/test/functional/interface_ipc.py +++ b/test/functional/interface_ipc.py @@ -8,7 +8,11 @@ from io import BytesIO from pathlib import Path import shutil from test_framework.messages import (CBlock, CTransaction, ser_uint256, COIN) -from test_framework.test_framework import (BitcoinTestFramework, assert_equal) +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + assert_not_equal +) from test_framework.wallet import MiniWallet # Test may be skipped and not have capnp installed @@ -49,10 +53,10 @@ class IPCInterfaceTest(BitcoinTestFramework): } def set_test_params(self): - self.num_nodes = 1 + self.num_nodes = 2 def setup_nodes(self): - self.extra_init = [{"ipcbind": True}] + self.extra_init = [{"ipcbind": True}, {}] super().setup_nodes() # Use this function to also load the capnp modules (we cannot use set_test_params for this, # as it is being called before knowing whether capnp is available). @@ -206,11 +210,26 @@ class IPCInterfaceTest(BitcoinTestFramework): self.log.debug("Submit a valid block") block.nVersion = original_version block.solve() + + self.log.debug("First call checkBlock()") res = await mining.result.checkBlock(block.serialize(), check_opts) assert_equal(res.result, True) + + self.log.debug("Submitted coinbase must include witness") + assert_not_equal(coinbase.serialize_without_witness().hex(), coinbase.serialize().hex()) + res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize_without_witness()) + assert_equal(res.result, False) + + self.log.debug("Submit again, with the witness") res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize()) assert_equal(res.result, True) - assert_equal(self.nodes[0].getchaintips()[0]["height"], current_block_height + 1) + + self.log.debug("Block should propagate") + assert_equal(self.nodes[1].getchaintips()[0]["height"], current_block_height + 1) + # Stalls if a regression causes submitBlock() to accept an invalid block: + self.sync_all() + assert_equal(self.nodes[0].getchaintips()[0], self.nodes[1].getchaintips()[0]) + miniwallet.rescan_utxos() assert_equal(miniwallet.get_balance(), balance + 1) self.log.debug("Check block should fail now, since it is a duplicate") From 6eaa00fe20206baedc0d8ade5bb8d066ea615704 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 31 Oct 2025 12:01:04 +0100 Subject: [PATCH 3/3] test: clarify submitBlock() mutates the template PR #33374 proposed a new Mining IPC method applySolution() which could be used by clients to obtain the reconstructed block for inspection, especially in the case of a rejected block. However it was pointed out during review that submitBlock() modified the template CBlock in place, so the client can just call getBlock() and no new method is needed. This commit adds a test to document that (now intentional) behavior. --- test/functional/interface_ipc.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py index e905c7753be..cce56e3294b 100755 --- a/test/functional/interface_ipc.py +++ b/test/functional/interface_ipc.py @@ -215,11 +215,20 @@ class IPCInterfaceTest(BitcoinTestFramework): res = await mining.result.checkBlock(block.serialize(), check_opts) assert_equal(res.result, True) + # The remote template block will be mutated, capture the original: + remote_block_before = await self.parse_and_deserialize_block(template, ctx) + self.log.debug("Submitted coinbase must include witness") assert_not_equal(coinbase.serialize_without_witness().hex(), coinbase.serialize().hex()) res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize_without_witness()) assert_equal(res.result, False) + self.log.debug("Even a rejected submitBlock() mutates the template's block") + # Can be used by clients to download and inspect the (rejected) + # reconstructed block. + remote_block_after = await self.parse_and_deserialize_block(template, ctx) + assert_not_equal(remote_block_before.serialize().hex(), remote_block_after.serialize().hex()) + self.log.debug("Submit again, with the witness") res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize()) assert_equal(res.result, True)