highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-03-22 21:50:14 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#33745: mining: check witness commitment in submitBlock

Browse Source 
6eaa00fe20 test: clarify submitBlock() mutates the template (Sjors Provoost)
862bd43283 mining: ensure witness commitment check in submitBlock (Sjors Provoost)
00d1b6ef4b doc: clarify UpdateUncommittedBlockStructures (Sjors Provoost)

Pull request description:

  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.

  As first noticed in #33374 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.

  Patch to produce the original issue:

  ```diff
  diff --git a/src/node/miner.cpp b/src/node/miner.cpp
  index b988e28a3f..28e9048a4d 100644
  --- a/src/node/miner.cpp
  +++ b/src/node/miner.cpp
  @@ -450,15 +450,10 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t
       }
       block.nVersion = version;
       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,
                                                         KernelNotifications& kernel_notifications,
                                                         CTxMemPool* mempool,
  diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
  index cce56e3294..bf1b7048ab 100755
  --- a/test/functional/interface_ipc.py
  +++ b/test/functional/interface_ipc.py
  @@ -216,22 +216,22 @@ class IPCInterfaceTest(BitcoinTestFramework):
               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")
  +            self.log.debug("Submitted coinbase with missing witness is accepted")
               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)
  +            assert_equal(res.result, True)

               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")
  +            self.log.debug("Submit again, with the witness - does not replace the invalid block")
               res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())
               assert_equal(res.result, True)

               self.log.debug("Block should propagate")
               assert_equal(self.nodes[1].getchaintips()[0]["height"], current_block_height + 1)
  ```

ACKs for top commit:
  ryanofsky:
    Code review ACK 6eaa00fe20. Just documentation updates and test clarifications since last review, also splitting up a commit.
  TheCharlatan:
    Re-ACK 6eaa00fe20
  ismaelsadeeq:
    Code review and tested ACK   6eaa00fe20

Tree-SHA512: 3a6280345b0290fe8300ebc63c13ad4058d24ceb35b7d7a784b974d5f04f420860ac03a9bf2fc6a799ef3fc55552ce033e879fa369298f976b9a01d72bd55d9e
This commit is contained in:
merge-script
2025-11-12 10:03:48 -05:00
parent e652b69b8d 6eaa00fe20
commit 3c3c6adb72
4 changed files with 57 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
src/interfaces/mining.h
View File

@@ -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;

5
src/node/miner.cpp
View File

@@ -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;
}
void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wait)

6
src/validation.h
View File

@@ -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). */

36
test/functional/interface_ipc.py
View File

@@ -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).
@@ -228,11 +232,35 @@ 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)
# 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)
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")