diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index a30c4afb526..6e0376915d8 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 39f0879ce2c..14e5ce0a7be 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; } void InterruptWait(KernelNotifications& kernel_notifications, bool& interrupt_wait) 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). */ diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py index a4b57ece2d1..23036649529 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). @@ -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")