This PR adds a submitBlock method to the IPC Mining interface, equivalent to the submitblock RPC. It accepts a serialized block over IPC, validates/processes it via the normal block-processing path.
The method uses the same result shape as checkBlock: bool + reason/debug out-params. It reports duplicate, inconclusive, and invalid-block rejection details, and initializes reason/debug on every call.
Closes #34626
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34644.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
BlockTemplateCache by ismaelsadeeq)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Possible places where comparison-specific test macros should replace generic comparisons:
assert len(block.vtx) >= 2, "Block should include at least the coinbase and the mempool tx" -> use assert_greater_than_or_equal(len(block.vtx), 2)<sup>2026-05-18 21:18:35</sup>
153 | + * it, and if accepted as new, processes it into chainstate. Accepted 154 | + * blocks may then be announced to peers through normal validation signals. 155 | + * 156 | + * Like the submitblock RPC, this may add the coinbase witness nonce if 157 | + * missing, if the block already has a witness commitment and segwit is 158 | + * active for the previous block (via UpdateUncommittedBlockStructures).
7a2d88c: I would prefer to mimic the behavior of submitSolution() here, and not magically modify the block.
It was useful for backwards compatibility when SegWit was newly introduced, but since IPC is new and SegWit has been active for a decade, there's no need for it. And I find the behavior confusing.
Removed the UpdateUncommittedBlockStructures call - the block is now passed as std::make_shared<const CBlock>(block_in) directly (the const enforces this at the type level).
The doc comment now notes: "unlike the submitblock RPC, this method does NOT add the coinbase witness automatically."
Added a test that submits a block with a witness commitment but no coinbase witness nonce and confirms it's rejected with bad-witness-nonce-size.
927 | @@ -928,6 +928,24 @@ class BlockTemplateImpl : public BlockTemplate 928 | NodeContext& m_node; 929 | }; 930 | 931 | +class SubmitBlockStateCatcher final : public CValidationInterface
7a2d88c6592efc9b5b55a652dc9d5a39ecf44242: I'm surprised this is needed, given that submitSolution can do without.
submitSolution returns just a bool (and passes nullptr for new_block), so it doesn't need to distinguish failure reasons.
submitBlock returns (reason, debug, result) and to populate reason and debug with the specific rejection string (e.g. bad-version(...), bad-witness-nonce-size), we need the BlockValidationState, which is only available through the BlockChecked validation interface callback.
Ah, I confused checkBlock with submitSolution.
Yes, checkBlock calls TestBlockValidity() which returns BlockValidationState directly as its return value.
submitBlock calls ProcessNewBlock() which only returns a bool. The BlockValidationState with the specific rejection reason is only available asynchronously through the CValidationInterface::BlockChecked callback.
146 | @@ -147,10 +147,11 @@ class Mining 147 | virtual bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason, std::string& debug) = 0; 148 | 149 | /** 150 | - * Process and relay a fully assembled block. 151 | + * Process a fully assembled block.
b72108fac82c38ced57aeded8725aec59ec52304 nit: changes in the wrong commit?
Done. Thanks.
346 | + async with destroying((await mining.createNewBlock(ctx, self.default_block_create_options)).result, ctx) as template: 347 | + block = await mining_get_block(template, ctx) 348 | + balance = self.miniwallet.get_balance() 349 | + coinbase = await self.build_coinbase_test(template, ctx, self.miniwallet) 350 | + coinbase.vout[0].nValue = COIN 351 | + block.vtx[0] = coinbase
b72108fac82c38ced57aeded8725aec59ec52304: let's add at least one real transaction to the block.
It's also useful, see my earlier comment, to test:
Done. Thanks.
373 | + assert_equal(self.nodes[0].getchaintips()[0], self.nodes[1].getchaintips()[0]) 374 | + 375 | + self.miniwallet.rescan_utxos() 376 | + assert_equal(self.miniwallet.get_balance(), balance + 1) 377 | + 378 | + self.log.debug("submitBlock duplicate should fail")
b72108fac82c38ced57aeded8725aec59ec52304: it might be useful to add a test for the interaction between submitSolution and submitBlock(). In either order this should result in a duplicate.
Calling submitSolution after submitBlock() shouldn't result in a crash or missing object error, because we hold on to BlockTemplate as long as the client has a reference to it. The sv2-tp implementation waits 10 seconds after a new block before releasing stale templates.
Added two interaction tests. Thanks.
Concept ACK
But let's make it more similar to the submitSolution() IPC rather than the getblocktemplate RPC, in terms of behavior and implementation (which can be much smaller).
999 | @@ -982,6 +1000,44 @@ class MinerImpl : public Mining 1000 | return state.IsValid(); 1001 | } 1002 | 1003 | + bool submitBlock(const CBlock& block_in, std::string& reason, std::string& debug) override
Once submitSolution reconstructs the CBlock it should be able to use the same code path as submitBlock.
That's true in principle, but submitSolution currently returns a plain bool (and true for duplicates), while submitBlock returns (reason, debug, result) and false for duplicates.
Sharing the code path would change submitSolution's return semantics, which would be a breaking change for existing IPC clients. Might be worth doing as a follow-up if the IPC schema for submitSolution is updated to also return reason/debug strings.
Adding fields to .capnp is not a breaking change for clients. It makes sense to relay the failure reason.
I'm not saying that submitSolution should call submitBlock directly, it can be a common helper. So it shouldn't be necessary to change the submitSolution signature, that can be left to a followup.
Added the commit 9e12fb4989bbac68d2ec4d13fcace56fd0b0c445 with the common helper.
Thanks, it makes sense to leave submitBlock changes to that followup.
reACK 35c74132b2cb0095ab5116376264fb921261d6e4
Minor rebase changes.
Previous:
ACK 6cc83536daf043dd11626a91b767b9f81116cf84
New method added to the mining IPC interface. New method, so no behavior change or compatibility issues. The implementation builds on submitSolution method and "submitblock" RPC, so it is rather small. Tests are added, including for typical error cases. Follow-up in #34672.
utACK 9e12fb4989bbac68d2ec4d13fcace56fd0b0c445
857 | @@ -858,6 +858,35 @@ class ChainImpl : public Chain 858 | NodeContext& m_node; 859 | }; 860 | 861 | +class SubmitBlockStateCatcher final : public CValidationInterface
In commit "refactor: extract ProcessBlock helper for submitSolution and submitBlock" (https://github.com/bitcoin/bitcoin/pull/34644/changes/9e12fb4989bbac68d2ec4d13fcace56fd0b0c445):
SubmitBlockStateCatcher is essentially a 1:1 copy of submitblock_StateCatcher in src/rpc/mining.cpp. Since commit this commit already refactored how this is used locally, does it make sense to deduplicate it entirely and move it to a shared location so that it can be used in src/rpc/mining.cpp as well?
This can be done in a follow-up PR. It's a refactor that increases scope. Moving the class to a shared header would touch src/rpc/mining.cpp which is unrelated to this PR's purpose (adding IPC submitBlock).
And also it is only 10 lines of boilerplate. Deduplicating it would require a new shared header, an include in both files, and coordination between the RPC and IPC layers that currently don't depend on each other. The "abstraction cost" exceeds the "duplication cost" for something this small.
493 | + assert_equal(submitted, True) 494 | + 495 | + self.sync_all() 496 | + 497 | + asyncio.run(capnp.run(async_routine())) 498 | +
nit suggestion:
In commit "test: add IPC submitBlock functional test" (https://github.com/bitcoin/bitcoin/pull/34644/changes/354538db36dc8f04a547b6cbb9159d0a94834901):
The current tests check what happens when you submit the same valid block twice (returning "duplicate"). It would be great to also test what happens when you submit the same invalid block twice.
The IPC test should return "duplicate-invalid" the second time, proving that the node correctly remembered the block was bad and rejected it.
Here is a quick test snippet you can drop in to cover this
index 032a696353..f5cc9c3d56 100755
--- a/test/functional/interface_ipc_mining.py
+++ b/test/functional/interface_ipc_mining.py
@@ -530,6 +530,43 @@ class IPCMiningTest(BitcoinTestFramework):
asyncio.run(capnp.run(async_routine()))
+ def run_submit_block_duplicate_invalid_test(self):
+ """Test submitting the exact same invalid block twice to check StateCatcher behavior."""
+ self.log.info("Running submitBlock duplicate-invalid test")
+
+ async def async_routine():
+ ctx, mining = await self.make_mining_ctx()
+
+ async with destroying((await mining.createNewBlock(ctx, self.default_block_create_options)).result, ctx) as
template:
+ block = await mining_get_block(template, ctx)
+ coinbase = await self.build_coinbase_test(template, ctx, self.miniwallet)
+ coinbase.vout[0].nValue = COIN
+ block.vtx[0] = coinbase
+ block.hashMerkleRoot = block.calc_merkle_root()
+
+ # Make the block definitively invalid (bad locktime)
+ block.vtx[0].nLockTime = 2**32 - 1
+ block.hashMerkleRoot = block.calc_merkle_root()
+ block.solve()
+ serialized_block = block.serialize()
+
+ self.log.debug("Submit invalid block first time")
+ result1 = await mining.submitBlock(ctx, serialized_block)
+ assert_equal(result1.result, False)
+ assert_equal(result1.reason, "bad-txns-nonfinal")
+
+
+ self.log.info(f"First submission reason was: '{result1.reason}'")
+
+ self.log.debug("Submit the exact same invalid block a second time")
+ result2 = await mining.submitBlock(ctx, serialized_block)
+ assert_equal(result2.result, False)
+
+ self.log.info(f"Second submission reason was: '{result2.reason}'")
+ assert_equal(result2.reason, "duplicate-invalid")
+
+ asyncio.run(capnp.run(async_routine()))
+
def run_test(self):
self.miniwallet = MiniWallet(self.nodes[0])
self.default_block_create_options = self.capnp_modules['mining'].BlockCreateOptions()
@@ -540,6 +577,7 @@ class IPCMiningTest(BitcoinTestFramework):
self.run_submit_block_witness_test()
self.run_submit_block_then_solution_test()
self.run_submit_solution_then_block_test()
+ self.run_submit_block_duplicate_invalid_test()
self.run_ipc_option_override_test()
Good catch. Included the test. Thanks.
527 | + assert_equal(result.reason, "duplicate") 528 | + 529 | + self.sync_all() 530 | + 531 | + asyncio.run(capnp.run(async_routine())) 532 | +
In commit "test: add IPC submitBlock functional test" (https://github.com/bitcoin/bitcoin/pull/34644/changes/354538db36dc8f04a547b6cbb9159d0a94834901):
I was testing the boundary conditions of the new IPC interface and noticed that sending malformed or truncated block data causes the entire node to crash.
Unlike the standard submitblock RPC which safely catches deserialization errors and returns a -22 Block decode failed error. The IPC implementation seems to let the deserialization exception bubble up, resulting in a SIGABRT (exit code -6).
Since a sv2 client could potentially send bad wire data, this looks like a DoS vector that should be caught at the boundary.
You can reproduce the crash by adding this test to test/functional/interface_ipc_mining.py:
def run_submit_block_malformed_test(self):
"""Test that submitting truncated/garbage bytes does not crash the node."""
self.log.info("Running submitBlock malformed data test")
async def async_routine():
ctx, mining = await self.make_mining_ctx()
async with destroying((await mining.createNewBlock(ctx, self.default_block_create_options)).result, ctx) as template:
block = await mining_get_block(template, ctx)
# Truncate the last 15 bytes to create an invalid serialization
bad_bytes = block.serialize()[:-15]
self.log.debug("Submitting truncated block bytes via IPC")
try:
result = await mining.submitBlock(ctx, bad_bytes)
assert_equal(result.result, False)
except Exception as e:
self.log.info(f"IPC exception caught in test: {e}")
# The node should handle the bad stream gracefully and remain alive
assert_equal(self.nodes[0].is_node_stopped(), False)
asyncio.run(capnp.run(async_routine()))
Running this results in the peer disconnecting and the node crashing:
AssertionError: [node 0] Node returned unexpected exit code (-6) vs ((0,)) when stopping
capnp.lib.capnp.KjException: capnp/rpc.c++:2778: disconnected: Peer disconnected.
I suspect the C++ handler unpacking the Data blob into a CBlock needs to be wrapped in a try...catch block to handle the std::ios_base::failure and return a safe rejection reason (e.g., "inconclusive" or a specific decode error), similar to how src/rpc/mining.cpp handles it.
I ran the test but I wasn't able to reproduce the error you mentioned.
As I understand it, Proxy.Context already handles it (via kj::runCatchingExceptions()), catching std::exception and sends it back to the client as kj::Exception rather than crashing the node.
I added the suggested functional test for this (truncated block bytes via submitBlock, asserting the node stays alive) so we can verify this on CI. Could you share which platform/configuration you tested on and which capnp version you were using? Is it possible there's a platform-specific edge case ?
Here is my environment
The kj::runCatchingExceptions() safety net might be failing locally because Fedora's capnproto package could be compiled with flags that alter standard exception handling. This could cause it to bypass the catch block and crash.
I couldn't find any information about flags that alter standard exception handling. Standard C++ try/catch for std::exception is fundamental. Distro packages don't break this.
Even if Cap'n Proto were built with -fno-exceptions, it would use a completely different error path.
The information provided so far are vague and insufficient to reproduce the error.
Anyway, what you are reporting - assuming it is reproducible - affects the IPC as overall (it would affect checkBlock and any other IPC method that deserializes Data → C++ types, not just submitBlock) and should be handled in a specific PR.
You make a fair point. If this crash also happens when sending bad data to checkBlock or other IPC methods, then it is a broader issue with the IPC layer itself. It definitely shouldn't hold up this specific PR.
re: #34644 (review)
You make a fair point. If this crash also happens when sending bad data to
checkBlockor other IPC methods, then it is a broader issue with the IPC layer itself. It definitely shouldn't hold up this specific PR.
This sounds very similar to the issue reported #33341 and fixed by https://github.com/bitcoin-core/libmultiprocess/commit/c4762c7b513abf66e3275b2bd3f2cf0cb980a8f7 in https://github.com/bitcoin-core/libmultiprocess/pull/240 and #34422.
The fix is not present in commit 354538db36dc8f04a547b6cbb9159d0a94834901 referenced above.
Rebased to address it. Thanks.
Concept ACK
I reviewed the code and ran some boundary tests. Left some comments below along with a couple of suggestions for test coverage and code cleanup
I opened https://github.com/2140-dev/bitcoin-capnp-types/pull/12 for easier testing from Rust, cc @plebhash.
It would be good to rebase this now that #34422 landed, to prevent confusion while testing from Rust.
It would be good to rebase this
Done. Thanks.
488 | + assert_equal(result.reason, "bad-witness-nonce-size") 489 | + 490 | + asyncio.run(capnp.run(async_routine())) 491 | + 492 | + def run_submit_block_then_solution_test(self): 493 | + """Test that submitSolution returns False for a duplicate after submitBlock succeeds."""
"""Test that submitSolution returns True for a duplicate after submitBlock succeeds."""
It should be true, since it's testing "submitSolution on same template should still return True (duplicate is accepted)".
I just noticed that the draft PR #34672 changes the submitSolution behavior and addresses this. If this current PR is intended to be merged first, it may still be worth to change docstring here to True to reflect the current state of the code.
Good catch. Thanks.
882 | + } 883 | +}; 884 | + 885 | +//! Process a block and capture the validation state via the BlockChecked callback. 886 | +//! Returns whether ProcessNewBlock accepted the block. 887 | +static bool ProcessBlock(ChainstateManager& chainman, const std::shared_ptr<const CBlock>& blockptr, bool* new_block, std::shared_ptr<SubmitBlockStateCatcher>& sc)
In 35c74132b2cb0095ab5116376264fb921261d6e4 refactor: extract ProcessBlock helper for submitSolution and submitBlock nit: blockptr should just be block, per naming conventions.
I would prefer to avoid passing SubmitBlockStateCatcher around. Why not pull in the checks from submitBlock and add the following arguments:
std::string& reasonstd::string& debugsubmitSolution() would ignore these arguments (until #34672), while submitBlock() can return accepted && new_block.
Done. Thanks.
832 | @@ -833,7 +833,19 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) 833 | // Alternate calls between Chainman's ProcessNewBlock and submitSolution 834 | // via the Mining interface. The former is used by net_processing as well 835 | // as the submitblock RPC. 836 | - if (current_height % 2 == 0) { 837 | + if (current_height == 0) {
In 448874d59de8e6a588ffedc6d209c3c6132b6886 mining: add submitBlock IPC method to Mining interface: don't forget to update the comment
You could also use % 3 == 0 for submitSolution() and % 3 == 1 for submitBlock() for a little more thorough coverage.
Style nit: maybe move this case below submitSolution()
Done. Thanks.
Code review 35c74132b2cb0095ab5116376264fb921261d6e4
It might be good to de-duplicate the test block creation between submitSolution(), submitBlock() and checkBlock(), here's a sketch: sjors/2026/05/ipc-submitblock-tests
Instead of adding a new test, after each submitSolution() / checkBlock() it also does a submitBlock() on an isolated node.
Maybe add a release note?
ACK 7049b0ad8734708e8cf40832c9cd31e4d9a64f43
933 | @@ -934,6 +934,24 @@ class BlockTemplateImpl : public BlockTemplate 934 | NodeContext& m_node; 935 | }; 936 | 937 | +class SubmitBlockStateCatcher final : public CValidationInterface 938 | +{ 939 | +public: 940 | + uint256 hash; 941 | + bool found{false}; 942 | + BlockValidationState state;
Would be good to prefixes these with m_ so these do not look like local variables when accessed.
Done. Thanks.
800 | @@ -801,12 +801,13 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) 801 | for (const auto& bi : BLOCKINFO) { 802 | const int current_height{mining->getTip()->height}; 803 | 804 | + const int height_mod{current_height % 3};
In commit "mining: add submitBlock IPC method to Mining interface" (1e9eb375c1056c1853f9b46b78c0345031591fb3)
IMO it would be better not to introduce this height_mod test complexity. Instead, the ProcessNewBlock call below could just be replaced with a submitBlock call. There should be no loss of test coverage because submitBlock is a thin wrapper around ProcessNewBlock. And actually it would increase coverage because reason and debug strings would be checked.
Done. Thanks.
942 | + BlockValidationState state; 943 | + 944 | + explicit SubmitBlockStateCatcher(const uint256& hashIn) : hash(hashIn), state() {} 945 | + 946 | +protected: 947 | + void BlockChecked(const std::shared_ptr<const CBlock>& block, const BlockValidationState& stateIn) override
In commit "mining: add submitBlock IPC method to Mining interface" (1e9eb375c1056c1853f9b46b78c0345031591fb3)
Would be good to have a comment here explaining why no locks are needed, maybe "Unlike most CValidationInterface notifications, BlockChecked is called synchronously with cs_main held, so no extra synchronization is needed"
Done. Thanks.
165 | + * @param[out] reason failure reason (BIP22) 166 | + * @param[out] debug more detailed rejection reason 167 | + * @returns true if the block was accepted as a new block 168 | + * 169 | + * @note unlike the submitblock RPC, this method does NOT add the 170 | + * coinbase witness automatically.
In commit "mining: add submitBlock IPC method to Mining interface" (1e9eb375c1056c1853f9b46b78c0345031591fb3)
IMO the paragraph "Unlike the submitblock RPC, this method does NOT auto-add the coinbase
witness nonce ..." should be removed from the commit description and moved into the code, either here or into a comment inside MinerImpl::submitBlock if it is too much detail to add here.
The difference between submit methods is confusing and deserves a clear explanation that's not hidden in git history. (Also removing this paragraph from the commit description should make that more readable because the paragraph following it better motivates the commit.)
Done. Thanks.
1043 | + 1044 | + if (!new_block && accepted) { 1045 | + reason = "duplicate"; 1046 | + return false; 1047 | + } 1048 | + if (!sc->found) {
In commit "mining: add submitBlock IPC method to Mining interface" (1e9eb375c1056c1853f9b46b78c0345031591fb3)
Is this case ever expected? If not it would seem better to use if (!Assume(sc->found)) and throw an exception, or use CHECK_NOTFATAL. Or if this case is sometime expected, it would be good to have a comment saying when.
Done. Thanks.
Documented the expected !m_found case: ProcessNewBlock() can accept/store a block without connecting it, so no BlockChecked callback is emitted and the result is inconclusive.
Also made submitBlock() return false for that inconclusive case, matching submitblock RPC-style semantics instead of returning true with a non-empty reason.
949 | @@ -909,7 +950,9 @@ class BlockTemplateImpl : public BlockTemplate 950 | bool submitSolution(uint32_t version, uint32_t timestamp, uint32_t nonce, CTransactionRef coinbase) override 951 | { 952 | AddMerkleRootAndCoinbase(m_block_template->block, std::move(coinbase), version, timestamp, nonce); 953 | - return chainman().ProcessNewBlock(std::make_shared<const CBlock>(m_block_template->block), /*force_processing=*/true, /*min_pow_checked=*/true, /*new_block=*/nullptr); 954 | + std::string reason; 955 | + std::string debug; 956 | + return ProcessBlock(chainman(), std::make_shared<const CBlock>(m_block_template->block), /*new_block=*/nullptr, reason, debug);
In commit "refactor: extract ProcessBlock helper for submitSolution and submitBlock" (7049b0ad8734708e8cf40832c9cd31e4d9a64f43)
It seems good for submitSolution and submitBlock to share the same code like this, and for submitSolution to be updated in a separate commit from the one that introduces submitBlock.
But all the other changes in this commit seem like they should be move to the first commit so this PR only adds code once, instead of adding it and immediately moving and refactoring it.
Done. Thanks.
882 | + } 883 | +}; 884 | + 885 | +//! Process a block and capture the validation state via the BlockChecked callback. 886 | +//! Returns whether ProcessNewBlock accepted the block. 887 | +static bool ProcessBlock(ChainstateManager& chainman, const std::shared_ptr<const CBlock>& block, bool* new_block, std::string& reason, std::string& debug)
In commit "refactor: extract ProcessBlock helper for submitSolution and submitBlock" (7049b0ad8734708e8cf40832c9cd31e4d9a64f43)
I think it makes more sense for ProcessBlock to live in node/miner.cpp (and be non-static) than live in node/interfaces.cpp, because interfaces.cpp is meant to contain interface glue code, not application logic.
Also SubmitBlock would seem like a better name than ProcessBlock for this function. More descriptive, more consistent with SubmitBlockStateCatcher, and with the "submit" IPC and RPC method names.
Done. Thanks.
Code review ACK 7049b0ad8734708e8cf40832c9cd31e4d9a64f43, but I didn't closely review the tests. Overall this looks very good and thanks for implementing this feature!
I left some review suggestions below which I think would be good to apply to make the changes clearer, but they are not critical.
1011 | @@ -1012,6 +1012,14 @@ class MinerImpl : public Mining 1012 | return state.IsValid(); 1013 | } 1014 | 1015 | + bool submitBlock(const CBlock& block_in, std::string& reason, std::string& debug) override 1016 | + { 1017 | + auto block = std::make_shared<const CBlock>(block_in); 1018 | + bool new_block; 1019 | + const bool accepted = SubmitBlock(chainman(), block, &new_block, reason, debug); 1020 | + return accepted && new_block && reason.empty();
In bed616c93b43db7b9de3042399a45e76abec3da8 mining: add submitBlock IPC method to Mining interface: would be good to document the reason why we check 3 things:
accepted: processed, but not yet checked for validitynew_block: not a duplicate (which we treat as an error, because it's useful for clients to know)reason.empty(): block was checked for validity and no issue was foundIf a block is not checked, because it doesn't have more work than our tip, reason will be inconclusive. We treat this as an error for the mining client, but it does not mean the block is invalid.
(either here on in the interface doc)
The only way to find out if it's valid is to invalidate the tip. Then the node reorgs and checks and getchaintips will tell you. I don't think we need to support that use case. ForkMonitor uses this to inspect stale blocks.
On a related note, we could add a precious argument to submitBlock() and submitSolution() that, when we have a tip at the same height, rolls the chain back and prefers the new block. Similar to the preciousblock RPC.
Code review b094946d6b158feb178db4a2e9d37b8c18a43d56. A couple of documentation suggestions.
Since my last review, the ProcessBlock helper was renamed to SubmitBlock and now introduced in the first commit, there goes my range-diff :-) See #34644 (review) and #34644 (review)
The commit message for d0547832299215ffcc0f6dd0234ffbe4d17c6a2c refactor: introduce SubmitBlock helper says: "Move the ProcessNewBlock submission wrapper ...", but that's no longer true since it's the first commit.
Would be good to also explain why the code was mostly copied from RPC land, rather than moved and adjusted. Maybe briefly mention the differences, that a followup refactor could combine them, but the current approach is easier to review.
In mining.h, can you document the difference in return value between submitSolution() and submitBlock() in a @note? I think @ryanofsky also suggested this in the last paragraph: #34644 (review)
186 | @@ -167,6 +187,7 @@ async def async_routine(): 187 | # Reconnect nodes so next tests are happy 188 | node.wait_for_rpc_connection() 189 | self.connect_nodes(1, 0) 190 | + self.sync_all()
In commit "test: add IPC submitBlock functional test" (a4ffb68e8537c7b5ed2f124c17fdfae0a9db15ad)
Is this newly needed, or is this a bugfix. Would be good to say why this is needed in code comment.
Thanks. Done.
376 | - block.vtx[0] = coinbase 377 | - block.hashMerkleRoot = block.calc_merkle_root() 378 | original_version = block.nVersion 379 | 380 | + self.log.debug("Disconnect node 2 before block submission tests") 381 | + self.disconnect_nodes(1, 2)
In commit "test: add IPC submitBlock functional test" (a4ffb68e8537c7b5ed2f124c17fdfae0a9db15ad)
It doesn't seem very clear from looking at the code what different the nodes are for and how the test is supposed to operate. It would be good to move some explanation from the commit description to the code so it can be understood in context, and is visible without looking up git history.
Thanks. Done.
Code review ACK a4ffb68e8537c7b5ed2f124c17fdfae0a9db15ad. Code looks very good (thanks for the updates!) but I didn't closely review the functional test. I found the test changes pretty hard to follow. If possible would be nice to break up the test commit into smaller changes and document the code a little better.
Introduce a SubmitBlock() helper in node/miner.cpp that wraps ProcessNewBlock submission and captures validation state through the BlockChecked callback.
Route submitSolution through the helper before adding any new IPC method.
No behavior change.
Add a submitBlock method to the Mining IPC interface, similar to the
submitblock RPC. This accepts a fully assembled block, validates it, and
if accepted as new, processes it into chainstate.
This is needed for Stratum v2 Job Declarator Server (JDS), where accepted
solutions may correspond to jobs not tied to a Bitcoin Core BlockTemplate.
JDS receives PushSolution fields and reconstructs full blocks; without an
IPC submitBlock method, final submission requires the submitblock RPC.
The method returns detailed status (reason/debug strings) matching the
checkBlock pattern, giving callers enough information to handle
validation failures.
Test the new Mining.submitBlock IPC method:
- Invalid block (bad version) returns failure with reason
- Valid block (with a real mempool tx) is accepted and propagates
- Duplicate block returns failure with "duplicate" reason
- Witness commitment without coinbase witness nonce is rejected
(bad-witness-nonce-size), confirming no auto-fix behavior
- submitBlock then submitSolution: duplicate is accepted (submitSolution
returns true for already-known blocks)
- submitSolution then submitBlock interaction (duplicate)
Build candidate blocks from BlockTemplate data in the existing coinbase and
submission test, then exercise checkBlock(), submitSolution(), and
submitBlock() against those candidates. submitBlock() uses an isolated IPC
node for cases that would otherwise affect the main submitSolution() and
checkBlock() assertions.
Thanks @ryanofsky, rebased and addressed both your comments by adding inline comments explaining the test flow.
Specifically:
sync_all() is needed after restarting node 0checkBlock() / submitSolution() checks, while node 2 is isolated for submitBlock() checksI kept this as a single test commit. I considered splitting it, but the helper changes are only meaningful together with the submitBlock coverage.