Add submitBlock(), misc. test improvements by Sjors · Pull Request #12 · 2140-dev/bitcoin-capnp-types

Sjors · 2026-03-05T19:14:25Z

This adds submitBlock() which is introduced by bitcoin/bitcoin#34644.

This PR also cleans up existing tests a bit.

Best reviewed per commit.

Sjors · 2026-03-06T15:06:35Z

Rebased after #9.

Sjors · 2026-03-11T16:09:07Z

Rebased after #14.

Sjors · 2026-05-28T12:49:03Z

bitcoin/bitcoin#34644 landed in master and should be in v32. Ready for review.

plebhash · 2026-05-28T20:51:04Z

+/// submitBlock with a template block should be rejected (unsolved high-hash).
+#[tokio::test]
+#[serial_test::serial]
+async fn mining_submit_block() {


IIUC this test is asserting an error case (block without solution), so IMO the test name should express that in a more explicit way

Suggested change

/// submitBlock with a template block should be rejected (unsolved high-hash).

#[tokio::test]

#[serial_test::serial]

async fn mining_submit_block() {

/// submitBlock with a template block should be rejected (unsolved high-hash).

#[tokio::test]

#[serial_test::serial]

async fn mining_submit_block_unresolved() {

moreover, I'd also add a mining_submit_block_resolved() counterpart, asserting the successful case

We now cover both cases.

plebhash · 2026-05-28T21:07:08Z

+#[serial_test::serial]
+async fn mining_submit_block() {
+    with_mining_client(|_client, thread, mining| async move {
+        let template = make_block_template(&mining, &thread).await;


IIUC this will generate a regtest block, for which the blockhash will be essentially pseudorandom

at regtest genesis difficulty, any random sha256d hash has 50% chance of passing the PoW diff target

so this test will fail non-determinstically for reasons that have nothing to do with the correctness of the underlying submitBlock implementation that we're trying to validate

This is now deterministic.

Shourya742 · 2026-05-29T07:24:27Z

+        let _reason = results.get_reason().unwrap();
+        let _debug = results.get_debug().unwrap();


Shouldn't this be asserted instead of unwrap or maybe we can remove it?

I've already refactored some things offline, but will take a look if it still applies...

Found a similar pattern in mining_check_block_and_interrupt and a bunch of other places, so I added commits to fix those too.

Sjors · 2026-05-29T07:39:53Z

@plebhash thanks for looking at the test. In this project tests are not supposed to be thorough end-to-end integration tests, so I hadn't really looked the vibed initial version. But they also shouldn't be unstable. And they should probably cover several expected responses.

I added util/miner.rs which ensures a block either has sufficient PoW or doesn't.

In anticipation of possible change in how submitSolution() will deal with duplicates, I added better coverage for that. See bitcoin/bitcoin#34672 (comment).

I cleaned up the existing submitSolution() tests to use helpers and avoid duplicate boilerplate.

The submitBlock() test now covers:

an invalid block
valid block with low PoW
good block
duplicate block

Best reviewed per commit.

Shourya742 · 2026-05-29T08:41:04Z

+        assert!(
+            template
+                .interrupt_wait_request()
+                .send()
+                .promise
+                .await
+                .is_ok(),
+            "interruptWait should not fail"
+        );


We can remove this assert

Here and in similar examples I dropped the outer assert! and instead use .expect.

Shourya742 · 2026-05-29T08:41:25Z

+        assert!(
+            mining.interrupt_request().send().promise.await.is_ok(),
+            "interrupt should not fail"
+        );


We can remove this assert

Shourya742 · 2026-05-29T08:59:43Z

+fn header_has_valid_pow(header: &BlockHeader) -> bool {
+    Target::from_compact(header.bits).is_met_by(header.block_hash())
+}


We can remove this helper.

Shourya742 · 2026-05-29T09:00:48Z

+#[path = "util/miner.rs"]
+mod miner_util;


We don't really use path attribute that often, can we please correct the imports and have indirections via the util module

I add a commit to clean that up for pre-existing code. And changed later commits to conform.

Shourya742 · 2026-05-29T09:01:48Z

+async fn template_block(
+    template: &mining_capnp::block_template::Client,
+    thread: &thread::Client,
+) -> Vec<u8> {
+    transform_template_block(template, thread, |block| block.to_vec()).await
+}
+
+async fn transform_template_block<F>(
+    template: &mining_capnp::block_template::Client,
+    thread: &thread::Client,
+    build_block: F,
+) -> Vec<u8>
+where
+    F: FnOnce(&[u8]) -> Vec<u8>,
+{
+    let mut get_block_req = template.get_block_request();
+    get_block_req
+        .get()
+        .get_context()
+        .unwrap()
+        .set_thread(thread.clone());
+    let get_block_resp = get_block_req.send().promise.await.unwrap();
+    build_block(get_block_resp.get().unwrap().get_result().unwrap())
+}
+
+async fn submit_solution(
+    template: &mining_capnp::block_template::Client,
+    thread: &thread::Client,
+    version: u32,
+    timestamp: u32,
+    nonce: u32,
+    coinbase: &[u8],
+) -> bool {
+    let mut req = template.submit_solution_request();
+    req.get().get_context().unwrap().set_thread(thread.clone());


lets remove these, makes code hard to understand and keep them inline

I added these helpers to make the tests themselves less verbose, to make it easier to see what different things are being tested.

But after playing around with this a bit, it's not really that helpful. I inlined them again.

To make things slightly less ugly, I replaced the repetitive req.get() with a local variable and use a local scope to group the assignments.

I also added a helper to get a block from a block template.

This removes explicit path attributes for the pre-existing test util modules and lets tests/test.rs import them through tests/util/mod.rs instead. Review comment: 2140-dev#12 (comment)

Sjors · 2026-05-29T10:49:15Z

Applied @Shourya742's suggestions.

I also renamed tests/util/miner.rs to tests/util/block.rs.

I still find constructing request quite ugly:

let mut req = template.submit_solution_request();
{
   let mut params = req.get();
   params.set_version(solution.version);
   params.set_timestamp(solution.timestamp);
   params.set_nonce(solution.nonce);
   params.set_coinbase(&solution.coinbase);
   params.get_context().unwrap().set_thread(thread.clone());
}
let resp = req.send().

In c++ jargon: why can't we pass a struct into submit_solution_request, instead of using .set_? I guess that requires changes to the underlying crate?

Sjors mentioned this pull request Mar 5, 2026

mining: add submitBlock to IPC Mining interface bitcoin/bitcoin#34644

Merged

Sjors force-pushed the 2026/03/submitblock branch from 6ed8511 to ca979c4 Compare March 6, 2026 15:06

plebhash mentioned this pull request Mar 8, 2026

need to implement PushSolution on jd_server_sv2 and BitcoinCoreSv2JDP stratum-mining/sv2-apps#322

Open

Sjors force-pushed the 2026/03/submitblock branch 2 times, most recently from b407807 to fa5ca24 Compare March 11, 2026 16:09

Sjors force-pushed the 2026/03/submitblock branch from fa5ca24 to fa7c2fa Compare May 28, 2026 12:38

Sjors marked this pull request as ready for review May 28, 2026 12:47

This was referenced May 28, 2026

Add getMemoryLoad() #13

Draft

Add getTransactionsByTxID() and getTransactionsByWitnessID() #11

Draft

plebhash requested review from Shourya742 and plebhash May 28, 2026 20:38

plebhash mentioned this pull request May 28, 2026

implement handle_push_solution on jd_server_sv2 + bitcoin_core_sv2 stratum-mining/sv2-apps#441

Open

2 tasks

plebhash reviewed May 28, 2026

View reviewed changes

Shourya742 reviewed May 29, 2026

View reviewed changes

Sjors force-pushed the 2026/03/submitblock branch from fa7c2fa to 6d2f7e4 Compare May 29, 2026 07:39

Sjors changed the title ~~Add submitBlock()~~ Add submitBlock(), misc. test improvements May 29, 2026

Shourya742 reviewed May 29, 2026

View reviewed changes

Sjors added 7 commits May 29, 2026 11:37

test: route existing helpers through util module

a1fdaf2

This removes explicit path attributes for the pre-existing test util modules and lets tests/test.rs import them through tests/util/mod.rs instead. Review comment: 2140-dev#12 (comment)

test: check discarded results explicitly

9ec4ad8

test: add mining block helper

2dabcfc

test: add block template helpers

d51eb79

test: submit solved block template solution

ce8af75

test: cover duplicate submit solution

6aaf2fc

capnp: add Mining.submitBlock() method

43a69ca

Sjors force-pushed the 2026/03/submitblock branch from 6d2f7e4 to 43a69ca Compare May 29, 2026 10:48

		let _reason = results.get_reason().unwrap();
		let _debug = results.get_debug().unwrap();

		#[path = "util/miner.rs"]
		mod miner_util;

Conversation

Sjors commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Mar 6, 2026

Uh oh!

Sjors commented Mar 11, 2026

Uh oh!

Sjors commented May 28, 2026

Uh oh!

plebhash May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented May 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Sjors commented Mar 5, 2026 •

edited

Loading

plebhash May 28, 2026 •

edited

Loading