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.

7a2d88c: 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.

b72108f nit: changes in the wrong commit?

Done. Thanks.

b72108f: let's add at least one real transaction to the block.

It's also useful, see my earlier comment, to test:

1. a non-segwit block
2. some invalid encodings, like having a segwit op_return output, but missing the coinbase witness

Done. Thanks.

b72108f: 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.

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 9e12fb4 with the common helper.

Added the submitSolution changes in #34672.
Preferred to open a separate PR to keep the this one focused on submitBlock.

In commit "refactor: extract ProcessBlock helper for submitSolution and submitBlock" (9e12fb4):

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.

nit suggestion:

In commit "test: add IPC submitBlock functional test" (354538d):

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.

In commit "test: add IPC submitBlock functional test" (354538d):

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

• OS: Fedora Linux 43
• Cap'n Proto: 1.0.1
• Build: cmake -B build -DENABLE_IPC=ON -DCapnProto_DIR="/usr/lib64/cmake/CapnProto"

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.