IPC stability bugs

Bug 53

Node accepts its own invalid block when a mining client omits the witness commitment

Status
Fixed
Symptom
Wrong result; worst effect seen: other
Where seen
Bitcoin Core functional tests
Trigger
Unusual input from a peer or caller
First reported
2025-10-30 (first report)
Fixed
2025-11-12
Fix
bitcoin#33745
In released binaries
Yes (v30.0-v30.3)
Defect ever on master
Yes

Summary

An IPC mining client that called BlockTemplate.submitSolution with a coinbase missing its witness commitment made bitcoin-node accept the invalid block as its own chain tip, while other nodes rejected it, leaving the miner's node stuck on an invalid chain. The node reused the template's CBlock after swapping in the client's coinbase without clearing the cached "witness commitment already checked" flag. This is not a crash or hang; it is a wrong validation result reachable only through IPC. Released v30.0 through v30.3 bitcoin-node binaries are affected, as are v29.0 source builds with IPC enabled; fixed on master by bitcoin#33745 ("mining: check witness commitment in submitBlock").

What happened

In review of bitcoin#33374, a PR adding an applySolution() method to the mining interface, ryanofsky asked (2025-09-19) what happens if a client submits a coinbase without the witness commitment, since submitSolution, unlike the submitblock RPC, uses the client's coinbase as given and does not add one. @Sjors tested it and found (2025-10-30) that the node accepted the invalid block "because of a cached m_checked_witness_commitment", opening bitcoin#33745 with a regression test the same day. The PR description includes a patch that reverts the fix and shows the node accepting the block. No user report exists.

Root cause

submitSolution calls AddMerkleRootAndCoinbase (src/node/miner.cpp), which modifies the CBlock the node kept from createNewBlock in place, replacing the coinbase and header fields, and then passes the same object to ProcessNewBlock. CBlock caches validation results in m_checked_witness_commitment, m_checked_merkle_root and fChecked. m_checked_witness_commitment was already true on the template from the validity check at template creation, and it was not reset after the client's coinbase was swapped in, so validation skipped the witness-commitment check on the modified block and connected it. The other two flags were not set on a template (Sjors's explanation).

The submitblock RPC deserializes a fresh CBlock for each submission, so its cached flags always start false. Keeping the template and patching it, which avoids resending full blocks to Stratum v2 clients, made the IPC path the first to re-validate a CBlock after changing it.

The code

Quoted from 862bd432837e^. submitSolution in src/node/interfaces.cpp patches the block the node kept from createNewBlock and validates a copy of it:

bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) override
{
    AddMerkleRootAndCoinbase(m_block_template->block, std::move(coinbase), version, timestamp, nonce);
    return chainman().ProcessNewBlock(std::make_shared<const CBlock>(m_block_template->block), /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr);
}

AddMerkleRootAndCoinbase in src/node/miner.cpp swaps in the client's coinbase and header fields and leaves the block's cached check results alone:

void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t version, uint32_t timestamp, uint32_t nonce)
{
    // ...
        block.vtx[0] = coinbase;
    // ...
    block.hashMerkleRoot = BlockMerkleRoot(block);
}

Validating the template when it was created left m_checked_witness_commitment set, and CheckWitnessMalleation in src/validation.cpp trusts it:

if (expect_witness_commitment) {
    if (block.m_checked_witness_commitment) return true;

So a coinbase submitted without its witness reserved value, which the check would reject as bad-witness-nonce-size, skips the check, and the node connects a block that its peers reject.

The fix clears the cached results after patching the block (862bd432837e; the src/interfaces/mining.h documentation and the test changes are omitted):

@@ -452,6 +452,11 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t
     block.hashMerkleRoot = BlockMerkleRoot(block);
+
+    // Reset cached checks
+    block.m_checked_witness_commitment = false;
+    block.m_checked_merkle_root = false;
+    block.fChecked = false;
 }

ProcessNewBlock now checks the witness commitment of the block as the client finished it, the same as for a block that arrives through submitblock. Only m_checked_witness_commitment was set on a template; the other two are cleared for safety.

Fix

Commit 862bd432837e ("mining: ensure witness commitment check in submitBlock") resets all three cached flags at the end of AddMerkleRootAndCoinbase (the two that were not stale for simplicity, after @TheCharlatan asked), and documents in src/interfaces/mining.h that IPC clients must supply the full coinbase including the witness commitment, instead of adding it the way the RPC does. interface_ipc.py now submits a coinbase without witness and checks it is rejected, and a second node checks that the valid block propagates.

Timeline

Lessons

What would have prevented this bug, found it sooner, or made it easier to debug. The patterns page groups these lessons across all bugs.

Pattern A contract changed and a caller was left behind: A shortcut that only held for the original caller

  • Prevented by a different API shape, a review checklist item
  • Earliest detector a unit test (did not exist then)
  • Diagnosis easy
  • Fix design easy
  • Debugging aids a reproduction script
  • Rust no

Would Rust have prevented it? No. Public cached-validation flags on a mutable struct go stale through any &mut CBlock in Rust just as in C++; the invariant between contents and cache is not expressible to the compiler. More on Rust

Could it have been prevented?

Yes. CBlock carries public mutable flags (fChecked, m_checked_merkle_root, m_checked_witness_commitment) that record "this check already passed", and any code that changes the block's contents is expected to remember to clear them. The submitblock RPC never had to remember because it deserializes a fresh block per submission; the IPC path was the first code to patch a block that had already been validated and validate it again, and it forgot. Two shapes would have made the bug hard to write: keep the flags private and clear them from every mutator (or make the checked block immutable), or build a new CBlock per submission the way the RPC does. The cheaper checklist item was also available when the interface was designed: submitSolution was deliberately made to take the client's coinbase as given rather than add the commitment as the RPC does, and a review that asks "what does an incomplete coinbase do here" is what eventually found it.

Could it have been found sooner?

Yes, by a negative test: submit a coinbase without the witness commitment and expect rejection. None existed for submitSolution from its merge in bitcoin#30955 (2024-10) until the regression test in the fix, and the applySolution tests being written in bitcoin#33374 only exercised valid solutions. A C++ client could send the same bytes, so this did not need a foreign client; it needed someone to try the invalid input, which ryanofsky's review question prompted six weeks later. An independent security review later flagged the same stale-flag class for in-process callers, so the class is not specific to IPC.

What made it hard to debug?

Nothing. The behaviour is deterministic, Sjors named the cached flag in the comment that reported it, and the fix PR carries a patch that reverts the reset to show the tip advancing on the invalid block. The only design question was whether the node should add the commitment for the client or document that the client must supply it; the fix documents it.

Lesson

A cached "already validated" flag on a mutable object is a promise every mutator has to keep; if a new code path modifies and re-validates an object that used to be built fresh each time, either clear the cache in the mutator or stop sharing the object. Write the negative test for each input a new interface accepts as given.

Classification

The survey's classification of this bug. The explorer charts these fields across all bugs; the method page explains them.

Field Value
Symptom wrong-result
Worst effect observed other
Seen in functional-test
Scope production
Trigger Unusual input (unusual-input)
Phase steady-state
Side server
Component bitcoin-mining
Root cause kind assumption-violated
Fix kind local-fix
Detection new-test
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released binary
Confidence low