Add checkBlock() to Mining interface (mining)

https://github.com/bitcoin/bitcoin/pull/31981

Host: ismaelsadeeq  -  PR author: Sjors

The PR branch HEAD was e6e170cf6c67a56b9c14cece66fdc4fab5f3ec6b at the time of this review club meeting.

Notes

Background

StratumV2 (Sv2) protocol defines various roles, one of which is the Template Provider (TP), explained in the template distribution section of the spec. The TP pushes block template updates to miners whenever the chain tip is updated or when mempool fees have increased sufficiently.

Rather than implementing the TP role in Bitcoin Core, the work done in #31098 allows third-party processes to interface with bitcoind through interprocess communication (IPC). The IPC interface, introduced as part of the multiprocess project, is work-in-progress too.

Bitcoin Core now exposes a mining interface through two classes BlockTemplate and Mining which provide key methods needed by the Sv2 TP client, enabling projects like Sjors’ TP client to receive block template updates via the interface.

PR Overview

  • PR #31981 adds a new method to the mining interface called checkBlock that checks the validity of a block template. This check is equivalent to the one done in getblocktemplate RPC in proposal mode.

  • The method responsible for checking whether a block template is valid in Bitcoin core is TestBlockValidity.

Motivation

  • Using the IPC interface to check block validity is more performant than the getblocktemplate RPC, as it avoids serializing up to 4 MB of transaction data as JSON.

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?

  2. Does an Sv2 TP client have to implement the new checkBlock method in the mining interface (yes/no)?

  3. Can you list some checks performed on the block in getblocktemplate proposal mode? Does that include checking for a valid proof of work for the block template?

  4. Apart from performance reasons mentioned in the PR description, are there any reasons an Sv2 TP client would want to use IPC instead of RPC?

  5. What is the key difference between contextual and context-free checks in the block template validity checks?

  6. Is there a change in behavior for TestBlockValidity after this PR, or is it a pure refactor? If yes, what’s the change?

  7. One of the reasons for locking cs_main in TestBlockValidity is to prevent CheckBlock race conditions. Can you describe a scenario when that might happen?

  8. In commit 1d029c23, why do we assert that fChecked should return false? Is this a potential issue?

  9. In this CheckBlock size check, which size rule are we checking? Is it an overestimate check? Which size check is missing there?

  10. In commit cca5993b, can you clarify the note that serialization of BlockValidationState is “fragile”?

Meeting Log

  117:00 <abubakarsadiq> #startmeeting
  217:00 <corebot> abubakarsadiq: Meeting started at 2025-04-30T17:00+0000
  317:00 <corebot> abubakarsadiq: Current chairs: abubakarsadiq
  417:00 <corebot> abubakarsadiq: Useful commands: #action #info #idea #link #topic #motion #vote #close #endmeeting
  517:00 <corebot> abubakarsadiq: See also: https://hcoop-meetbot.readthedocs.io/en/stable/
  617:00 <corebot> abubakarsadiq: Participants should now identify themselves with '#here' or with an alias like '#here FirstLast'
  717:00 <dzxzg> Hi
  817:00 <sebastianvstaa> Hi
  917:00 <abubakarsadiq> hi hi
 1017:00 <monlovesmango> hey
 1117:00 <Guest16> hi
 1217:01 <abubakarsadiq> welcome to this week review club session \o/
 1317:01 <abubakarsadiq> We will take a loot at #31981
 1417:01 <corebot> https://github.com/bitcoin/bitcoin/issues/31981 | Add checkBlock() to Mining interface by Sjors · Pull Request #31981 · bitcoin/bitcoin · GitHub
 1517:02 <abubakarsadiq> The notes and question are available https://bitcoincore.reviews/31981
 1617:03 <abubakarsadiq> Did you review the PR or read the notes? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?
 1717:03 <monlovesmango> reviewed some of the PR, read the notes. concept ACK
 1817:04 <enochazariah> reviewed the PR partially, and read the notes as well. Conecpt ACK
 1917:04 <sebastianvstaa> concept ACK, approach ACK. Worked through the questions
 2017:04 <abubakarsadiq> nice
 2117:04 <dzxzg> I reviewed the pr commits, and made some experimental changes concept ACK, with questions about the approach
 2217:04 <abubakarsadiq> Lets jump into the conceptual questions then.
 2317:04 <abubakarsadiq> Does an Sv2 TP client have to implement the new checkBlock method in the mining interface (yes/no)?
 2417:05 <sebastianvstaa> no
 2517:05 <monlovesmango> no
 2617:05 <stringintech> I'm a bit confused by the terminology here. The PR description mentions “pools” may want to verify templates using checkBlock IPC (or getblocktemplate proposal mode). The question asks if “Sv2 TP clients” need to implement this method. Are “pools” and “Sv2 TP clients” the same thing here? I haven't gone through the Sv2 documentation
 2717:05 <stringintech> yet, so I might be missing something obvious.
 2817:05 <monlovesmango> its the Sv2 TP server that has to implement it right?
 2917:06 <sebastianvstaa> It can still utilize the method provided by Bitcoin Core via IPC
 3017:06 <abubakarsadiq> yes sebastianvstaa: monlovesmango
 3117:06 <abubakarsadiq> TP does not need the checkBlock method, I think it's the Sv2 server
 3217:07 <abubakarsadiq> Question 2: Can you list some checks performed on the block in getblocktemplate proposal mode? Does that include checking for a valid proof of work for the block template?
 3317:08 <monlovesmango> it seems to check for duplicate and duplicate-invalid blocks
 3417:08 <sebastianvstaa> block size limit, duplicate transactions
 3517:08 <monlovesmango> POW is not checked
 3617:08 <stringintech> Context-free checks like merle root, duplicate txs, block size, …
 3717:08 <stringintech> Contextual checks like block time, tx finality, whether block witness data has been tempered with, block weight, …
 3817:08 <stringintech> And no valid PoW is checked cause the goal would be start solving the block once the checkBlock passes in getblocktemplate proposal mode.
 3917:09 <sebastianvstaa> yes, no PoW
 4017:09 <sebastianvstaa> The idea is to check validity before you spend resources on mining
 4117:10 <monlovesmango> stringintech: very thorough and helpful answer :)
 4217:10 <abubakarsadiq> @stringintech yeah
 4317:11 <abubakarsadiq> sebastianvstaa: I am not sure if that is correct cc @sjors
 4417:12 <sebastianvstaa> abubakarsadiq what statement exactly?
 4517:12 <sebastianvstaa> check validity before mining?
 4617:12 <abubakarsadiq> your statement "The idea is to check validity before you spend resources on mining"
 4717:12 <stringintech> monlovesmango: 🙌🏻
 4817:13 <sebastianvstaa> what else would you do without PoW?
 4917:13 <abubakarsadiq> follow-up question why is POW not checked, but we check that the nBits are valid?
 5017:13 <abubakarsadiq> The next question is
 5117:13 <abubakarsadiq> 4. Apart from performance reasons mentioned in the PR description, are there any reasons an Sv2 TP client would want to use IPC instead of RPC?
 5217:15 <stringintech> Not sure but if clients are already using IPC for mining related functions implementing block verification this way too would be easier.
 5317:15 <sebastianvstaa> Better integration with the other components in the multiprocess project?
 5417:15 <monlovesmango> is IPC multi threaded? RPC seems to be single threaded, but maybe i'm wrong on that
 5517:15 <sjors47> sorry for the delay
 5617:15 <monlovesmango> does IPC expose a port to listen?
 5717:16 <abubakarsadiq> I think another reason apart from performance I read is that RPC is poll-based. An Sv2 TP client prefers push-based communication or long-polling, as seen in the waitNext implementation.
 5817:16 <sjors47> monlovesmangonot at the moment
 5917:16 <sjors47> monlovesmango: not at the moment
 6017:16 <sjors47> It listens on a Unix socket
 6117:17 <sjors47> Or maybe TPC port support as also been added.
 6217:17 <sjors47> stringintech: a pool in stratum v2 is just a pool like now.
 6317:18 <abubakarsadiq> yay sjors is here!
 6417:18 <sjors47> But in Stratum v2 the pool needs to verify that the individual miner is not sending them a fake block.
 6517:18 <monlovesmango> is Unix socket more secure than TPC port? I have no idea why else IPC would be advantageous, just trying to ask questions
 6617:18 <sjors47> So the way they could do that currently is to use the getblocktemplate RPC in "proposal" mode, and basically just give it a serialized hex block.
 6717:19 <sjors47> monlovesmango: I don't think the reason is security
 6817:19 <sjors47> It's just that Russ Ryanofsky, who's been working on multiprocess IPC for many years, added that
 6917:19 <abubakarsadiq> The next question
 7017:19 <abubakarsadiq> 5. What is the key difference between contextual and context-free checks in the block template validity checks?
 7117:19 <sjors47> It might be a bit more performant.
 7217:20 <sjors47> Continuing as myself under Sjors[m]
 7317:20 <Sjors[m]> If that works
 7417:21 <abubakarsadiq> It works!
 7517:21 <monlovesmango> contextual check references previous block headers
 7617:21 <dzxzg> From a high level, the no context checks in CheckBlock don't have a view of the chainstate, and the contextual checks do
 7717:21 <sebastianvstaa> context free: can be done without knowledge of blockchain current state
 7817:21 <santos> monlovesmango: Local IPC channels often have lower overhead than TCP and are easy to secure via OS‐provided permissions, but they don’t give you the same out-of-the-box, cryptographic authentication (or optional TLS encryption) that you get if you run over TCP.
 7917:21 <abubakarsadiq> yep dzxzg
 8017:22 <abubakarsadiq> correct sebastianvstaa
 8117:22 <monlovesmango> santos: I see thanks!
 8217:22 <stringintech> Sjors[m] thanks! the PR description made sense to me; just got a bit lost with Sv2 terminology...
 8317:22 <monlovesmango> is IPC able to process multiple requests at the same time? RPC can't do that right?
 8417:22 <Sjors[m]> stringintech: I don't have the full log before I entered, so feel free to ask a followup question if I missed somethign
 8517:23 <abubakarsadiq> @sjors question why is POW not checked by the pool, but we check that the nBits are valid?
 8617:23 <Sjors[m]> POW ␝is␏ checked by the pool
 8717:23 <Sjors[m]> I dropped that check from the earlier version.
 8817:24 <Sjors[m]> One bit of context...
 8917:24 <Sjors[m]> nBits represents the "real" work that will be in the final block
 9017:24 <Sjors[m]> Shares however use a lower amount of work.
 9117:24 <abubakarsadiq> ohh yeah I locally have the previous version of the PR
 9217:24 <Sjors[m]> So the nBits value won't mach the actual work.
 9317:25 <Sjors[m]> Bitcoin Core simply ignores that when checking the template, it doesn't check the actual work, it only checks the promise of work.
 9417:25 <Sjors[m]> So the pool needs to check the actual work in the share and pay rewards accordingly.
 9517:25 <monlovesmango> so the POW is checked by the pool outside of bitcoin core?
 9617:25 <Sjors[m]> And the share has enough work for the whole network to accept it, then it immediately gets broadcast (and you get your share payment).
 9717:26 <Sjors[m]> monlovesmango: yes
 9817:26 <Sjors[m]> One way to describe a "share" would be as a weak block.
 9917:26 <Sjors[m]> (extremely weak)
10017:27 <abubakarsadiq> thanks @sjors
10117:27 <Sjors[m]> santos: you can even do a unix socket of SSH quite easily
10217:27 <Sjors[m]> * over
10317:27 <abubakarsadiq> next question
10417:27 <abubakarsadiq> Is there a change in behavior for TestBlockValidity after this PR, or is it a pure refactor? If yes, what’s the change?
10517:28 <sebastianvstaa> yes. Returns a boolean value and a string instead in BlockVAlidationState object
10617:28 <sebastianvstaa> but I wonder why? Why a string instead of a state enum? Seems cumbersome
10717:29 <dzxzg> It also no longer logs block invalidity reasons as errors
10817:29 <monlovesmango> I thought it always returned bool?
10917:29 <stringintech> i noticed two main changes; the error handling and the CheckBlock (a context-free check) which is called before the contextual header check.
11017:29 <stringintech> and i guess the second one could be considered a behavioral change.
11117:29 <Sjors[m]> BlockVAlidationState required a bunch of custom code to serialize over IPC
11217:29 <abubakarsadiq> @sebastianvstaa: Is that a behavior change?
11317:29 <abubakarsadiq> I think it's the same return values we get previously just in another as boolean and strings now
11417:29 <santos> context-free checks (CheckBlock) are done first now
11517:29 <dzxzg> monlovesmango: https://github.com/bitcoin/bitcoin/blob/14b8dfb2bd5e2ca2b7c0c9a7f7d50e1e60adf75c/src/rpc/mining.cpp#L725-L766
11617:30 <abubakarsadiq> @sjors this brings us to another question In commit cca5993b, can you clarify the note that serialization of BlockValidationState is “fragile”?
11717:30 <Sjors[m]> I didn't write that note :-)
11817:30 <Sjors[m]> I just saw it as an excuse to delete it.
11917:30 <abubakarsadiq> :D
12017:30 <Sjors[m]> For historical background how this PR came about...
12117:31 <Sjors[m]> I initially didn't know about the proposal mode
12217:31 <Sjors[m]> So I ended up implementing all these checks and then realize it was duplicate work
12317:31 <abubakarsadiq> yeah I could not answer that as well maybe @ryanofsky can help here.
12417:31 <Sjors[m]> So then I gradually changed my implementation to be closer to the original.
12517:32 <Sjors[m]> So may be that my initial design just used a boolean return with a string message.
12617:32 <dzxzg> I'm not too familiar with the IPC interface, but is serialization of complex C++ types a problem in general?
12717:32 <Sjors[m]> But it had the benefit of not needing that BlockValidationState struct, so I kept it.
12817:32 <abubakarsadiq> The next question is
12917:32 <abubakarsadiq> One of the reasons for locking `cs_main` in `TestBlockValidity` is to prevent `CheckBlock` race conditions. Can you describe a scenario when that might happen?
13017:33 <Sjors[m]> dzxzg: not sure about "complex" but basically every type has its own serialization logic.
13117:33 <Sjors[m]> With some "universal" things like byte spans.
13217:33 <Sjors[m]> So anything that can be serialzed on the p2p network can be handled in a similar way with no extra IPC code.
13317:33 <Sjors[m]> But BlockValidationState is an internal type, so there's no serialization for it.
13417:34 <monlovesmango> if checkblock is called for multiple blocks?
13517:34 <Sjors[m]> See this commit for how it was serialized: https://github.com/bitcoin/bitcoin/pull/31981/commits/9822bd64d26ca056c0fe44e5e2b3e1f38e6021ef
13617:34 <Sjors[m]> monlovesmango: one block at a time
13717:34 <Sjors[m]> But potentially extremely frequently.
13817:34 <stringintech> aren't we working with a fresh instance of a block each time we get into TestBlockValidity?
13917:34 <dzxzg> So the main reason for the refactor is to drop the `CustomBuildMessage()` logic for BlockValidationState
14017:35 <Sjors[m]> Let's say you're a pool and there's 10000 miners that are each proposing a new block every 1 second... and you want to check them all...
14117:35 <Sjors[m]> That would probably crash the node :-)
14217:35 <Sjors[m]> But you spin up multiple nodes to distribute the checking work.
14317:35 <abubakarsadiq> @monlovesmango: what do you mean by "called for multiple blocks? "
14417:36 <Sjors[m]> stringintech: yes
14517:36 <monlovesmango> that checkblock is called for different block candidates that are found very close in time
14617:37 <abubakarsadiq> @sjors is it possible we receive the same block from the P2P network and are trying to validate and update its state, while simultaneously calling getblocktemplate in proposal mode with same block. that would cause a race no?
14717:37 <stringintech> Sjors[m]: So we only need to slow things down here (implicitly create a queue)?
14817:37 <Sjors[m]> abubakarsadiq: yes all that can happen at the same time
14917:38 <Sjors[m]> Even without this feature, I ran into the problem with Stratum v2 that we braodcast our own blocks now.
15017:38 <Sjors[m]> At the same time the pool is also broadcasting it
15117:38 <Sjors[m]> So you can have a race where we receive a block over p2p (coming from the pool) and we're publishing one ourselves.
15217:38 <Sjors[m]> I believe I took care of those races.
15317:39 <abubakarsadiq> monlovesmango: I don't think that will cause a race since they are not identical blocks, even if they are identical correct me if I am wrong @sjors. `TestBlockValidity` does not save the block so a race will not happen no?
15417:39 <Sjors[m]> Now with checkBlock we never broadcast it.
15517:39 <monlovesmango> abubakarsadiq: gotcha, thanks
15617:39 <dzxzg> Why does the PR move responsibility for locking cs_main from the caller to testblockvalidity
15717:39 <santos> So now is checkBlock() called when the node receiver a new block or when the pool receives a new block template proposal ? Is this correct ?
15817:39 <abubakarsadiq> when called via getblocktemplate in proposal mode
15917:40 <Sjors[m]> The blocking of cs_main however is of course not good for the pool if they're trying to stay up to date.
16017:40 <Sjors[m]> So probably they should run these checks on different nodes than they use for contructing their own (default) block.
16117:41 <abubakarsadiq> Lets move to the next question
16217:41 <abubakarsadiq> In commit 1d029c23, why do we assert that `fChecked` should return `false`? Is this a potential issue?
16317:41 <stringintech> abubakarsadiq: I thought so (regarding your explaination race condition for identical blocks) but i noticed in the code that may be worried about a race condition on fChecked flag; which i could not understand why.
16417:41 <Sjors[m]> santos: when the pool receives a block _proposal_
16517:41 <abubakarsadiq> Note: this is in an earlier version of the PR
16617:42 <Sjors[m]> I was going to say, I dropped some of those checks.
16717:43 <abubakarsadiq> stringintech: I mentioned that will not be an issue for identical blocks when called via the RPC because we dont save the state.
16817:44 <abubakarsadiq> yeah @sjors to answer the question myself I was a bit confused by the assertion. I believe it is an issue we should return instead. We might attempt to check a block that we already received via P2P and stored it's state.
16917:45 <Sjors[m]> Mmm, one of the first checks if fChecked
17017:45 <Sjors[m]> block.hashPrevBlock != *Assert(tip->phashBlock)
17117:46 <Sjors[m]> And we've locked m_chainman by then
17217:46 <stringintech> abubakarsadiq: yes i understand that. which is why i didnt get why we are worried about the fChecked race condition (not sure if i read this in the code comments or commit description or ...)
17317:46 <abubakarsadiq> This assertion ` Assert(!block.fChecked);`
17417:46 <Sjors[m]> I initially wrote some assert and assume statement to sanity check my own changes.
17517:47 <Sjors[m]> But this was a few months ago, so I don't remember either why that particular assert.
17617:47 <Sjors[m]> And it's gone now.
17717:48 <dzxzg> It seems like avoiding BlockValidationState is a nice win for the IPC interface, but it would be nice for internal users of `TestBlockValidity` to have the enum... I wonder the refactor would be simpler if testblockvalidity remained mostly as is and you placed a little wrapper around TestBlockValidity, that returns a bool and modifies the passed
17817:48 <abubakarsadiq> It is in this commit https://github.com/bitcoin-core-review-club/bitcoin/blob/1d029c23a143f1b9110377967a7c4a20ee75058d/src/validation.cpp#L4662
17917:48 <dzxzg> in strings,
18017:48 <monlovesmango> so can someone summarize what race condition we are trying to prevent when locking `cs_main` in `TestBlockValidity`?
18117:49 <stringintech> monlovesmango: 👍
18217:49 <abubakarsadiq> Final Question In this https://github.com/bitcoin/bitcoin/blob/9efe5466885862bc1fb830524f7ce23e785fcac0/src/validation.cpp#L4078 `CheckBlock` size check, which size rule are we checking? Is it an overestimate check? Which size check is missing there?
18317:49 <monlovesmango> I understand the lock is needed to prevent tip from updating, but want to understand the race condition element too
18417:50 <abubakarsadiq> dzxzg: what will be the benefit of that, how will the enum be useful, is not the same as the boolean output?
18517:51 <stringintech> abubakarsadiq: Witness data is excluded when performing the check, but later in ContextualCheckBlock we consider it (block weight check).
18617:51 <monlovesmango> I didn't quite undertand the size check logic, but looks like validating there are txs, and that it doesn't exceed max block size
18717:52 <abubakarsadiq> stringintech: we also do weight check there but why is it not enough?
18817:54 <dzxzg> abubakarsadiq: Every caller of TestBlockValidity cares about two things: Was it valid? If not, what was the reason? That is solved in a precise and reusable way with an enum. It seems to me that only the ipc interface suffers from the awkardness of packing and serializing all of this over the wire.
18917:54 <abubakarsadiq> monlovesmango: same also I don’t understand the second check: `block.vtx.size() * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT`
19017:55 <Sjors[m]> dzxzg: the caller ultimately only gets a BIP22 string
19117:55 <Sjors[m]> This was is the case the getblocktemplate RPC too.
19217:55 <Sjors[m]> Enum is used internally.
19317:55 <dzxzg> By caller I mean internal users
19417:56 <Sjors[m]> Remember that RPC uses JSON, which is even worse in terms of what you can serialize
19517:56 <stringintech> abubakarsadiq: if you mean why CheckBlock size check is not enough because we are not considering witness data and until we are sure the witness data is not tempered with in ContextualCheckBlock we can not check it?
19617:56 <Sjors[m]> dzxzg: true
19717:58 <abubakarsadiq> Yeah I think it is an overestimate check for the weight that enable us to fail fast when the non-witness block weight exceed the maximum block weight.
19817:58 <abubakarsadiq> But yeah we do check the witness weight as well
19917:58 <monlovesmango> I can understand not using teh enum if we are actually getting rid of the enum altogether. but if the enum is going to continue to exists it seems better to use the enum for consistency
20017:59 <Sjors[m]> dzxzg: Russ also suggested in his recent review to keep the enum, but I'm not entirely sure why yet: https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2035713597
20117:59 <Sjors[m]> So maybe it'll come back, we'll see
20218:00 <abubakarsadiq> #endmeeting