mining: ensure witness commitment check in submitBlock
What the developer wrote
Authored by Sjors Provoost
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.
What changed, and why it matters
Bitcoin Core's new mining IPC interface had a bug where it could accept an invalid block as the new chain tip. When an external mining client submitted a block solution with a new coinbase transaction, the node failed to re-check the block's witness commitment. This cached check could remain marked valid from the original template, allowing a block with a missing or wrong witness commitment to be accepted. The fix resets those cached checks whenever the coinbase is swapped in, and adds a test proving such invalid blocks are now rejected.
Treat this as a security-relevant consensus/validation bug in the IPC mining interface. Ensure the fix is included in any release branch that ships the Mining interface. Operators using external miners via IPC should upgrade. No emergency network-wide action is indicated because the interface is new and the RPC submitblock path was unaffected.
Security signals we found
acceptance of invalid chain tip due to stale cached validation flag
missing witness commitment re-validation after coinbase replacement
IPC mining interface submitSolution path affected
test added for rejection of witness-stripped coinbase
Evidence from the diff
In src/node/miner.cpp, AddMerkleRootAndCoinbase() now resets block.m_checked_witness_commitment, block.m_checked_merkle_root, and block.fChecked after updating the coinbase and recomputing the merkle root. Previously these cached flags persisted from the template block, so submitSolution() via the IPC Mining interface could skip the witness commitment validation for the client-provided coinbase. The header documentation in src/interfaces/mining.h is updated to clarify that IPC clients must supply a complete coinbase including the witness commitment, unlike the submitblock RPC which auto-adds it. A functional test in test/functional/interface_ipc.py verifies that submitSolution() with a coinbase stripped of witness data returns false, and that the valid version propagates to a second node.
Changed components
src/node/miner.cpp AddMerkleRootAndCoinbase()src/interfaces/mining.h Mining::submitSolution()test/functional/interface_ipc.pyInspect captured patch +43 / −6
diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h
index 150295e5..50a7922a 100644
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -54,9 +54,22 @@ public:
virtual std::vector<uint256> 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 28e9048a..b988e28a 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<CBlockTemplate> WaitAndCreateNewBlock(ChainstateManager& chainman,
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index abcc4d6b..e905c775 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")
Why this scored 63/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.