17:00:14 <abubakarsadiq> #startmeeting 
17:00:14 <corebot> abubakarsadiq: Meeting started at 2025-04-30T17:00+0000
17:00:15 <corebot> abubakarsadiq: Current chairs: abubakarsadiq
17:00:16 <corebot> abubakarsadiq: Useful commands: #action #info #idea #link #topic #motion #vote #close #endmeeting
17:00:17 <corebot> abubakarsadiq: See also: https://hcoop-meetbot.readthedocs.io/en/stable/
17:00:18 <corebot> abubakarsadiq: Participants should now identify themselves with '#here' or with an alias like '#here FirstLast'
17:00:24 <dzxzg> Hi
17:00:33 <sebastianvstaa> Hi
17:00:33 <abubakarsadiq> hi hi
17:00:43 <monlovesmango> hey
17:00:59 <Guest16> hi
17:01:22 <abubakarsadiq> welcome to this week review club session \o/
17:01:41 <abubakarsadiq> We will take a loot at #31981
17:01:43 <corebot> https://github.com/bitcoin/bitcoin/issues/31981 | Add checkBlock() to Mining interface by Sjors · Pull Request #31981 · bitcoin/bitcoin · GitHub
17:02:09 <abubakarsadiq> The notes and question are available https://bitcoincore.reviews/31981
17:03:01 <abubakarsadiq> Did you review the PR or read the notes? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?
17:03:52 <monlovesmango> reviewed some of the PR, read the notes. concept ACK
17:04:12 <enochazariah> reviewed the PR partially, and read the notes as well. Conecpt ACK
17:04:21 <sebastianvstaa> concept ACK, approach ACK. Worked through the questions
17:04:26 <abubakarsadiq> nice
17:04:33 <dzxzg> I reviewed the pr commits, and made some experimental changes concept ACK, with questions about the approach
17:04:49 <abubakarsadiq> Lets jump into the conceptual questions then.
17:04:54 <abubakarsadiq> Does an Sv2 TP client have to implement the new checkBlock method in the mining interface (yes/no)?
17:05:12 <sebastianvstaa> no
17:05:25 <monlovesmango> no
17:05:32 <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
17:05:33 <stringintech> yet, so I might be missing something obvious.
17:05:37 <monlovesmango> its the Sv2 TP server that has to implement it right?
17:06:09 <sebastianvstaa> It can still utilize the method provided by Bitcoin Core via IPC
17:06:19 <abubakarsadiq> yes sebastianvstaa: monlovesmango
17:06:59 <abubakarsadiq> TP does not need the checkBlock  method, I think it's the Sv2 server
17:07:53 <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?
17:08:33 <monlovesmango> it seems to check for duplicate and duplicate-invalid blocks
17:08:38 <sebastianvstaa> block size limit, duplicate transactions
17:08:44 <monlovesmango> POW is not checked
17:08:58 <stringintech> Context-free checks like merle root, duplicate txs, block size, …
17:08:59 <stringintech> Contextual checks like block time, tx finality, whether block witness data has been tempered with, block weight, …
17:08:59 <stringintech> And no valid PoW is checked cause the goal would be start solving the block once the checkBlock passes in getblocktemplate proposal mode.
17:09:11 <sebastianvstaa> yes, no PoW
17:09:43 <sebastianvstaa> The idea is to check validity before you spend resources on mining
17:10:20 <monlovesmango> stringintech: very thorough and helpful answer :)
17:10:28 <abubakarsadiq> @stringintech yeah
17:11:29 <abubakarsadiq> sebastianvstaa: I am not sure if that is correct cc @sjors
17:12:02 <sebastianvstaa> abubakarsadiq what statement exactly?
17:12:19 <sebastianvstaa> check validity before mining?
17:12:26 <abubakarsadiq> your statement "The idea is to check validity before you spend resources on mining"
17:12:57 <stringintech> monlovesmango: 🙌🏻
17:13:05 <sebastianvstaa> what else would you do without PoW?
17:13:19 <abubakarsadiq> follow-up question why is POW not checked, but we check that the nBits are valid?
17:13:51 <abubakarsadiq> The next question is
17:13:51 <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?
17:15:05 <stringintech> Not sure but if clients are already using IPC for mining related functions implementing block verification this way too would be easier.
17:15:25 <sebastianvstaa> Better integration with the other components in the multiprocess project?
17:15:33 <monlovesmango> is IPC multi threaded? RPC seems to be single threaded, but maybe i'm wrong on that
17:15:49 <sjors47> sorry for the delay
17:15:51 <monlovesmango> does IPC expose a port to listen?
17:16:31 <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.
17:16:35 <sjors47> monlovesmangonot at the moment
17:16:42 <sjors47> monlovesmango: not at the moment
17:16:50 <sjors47> It listens on a Unix socket
17:17:10 <sjors47> Or maybe TPC port support as also been added.
17:17:43 <sjors47> stringintech: a pool in stratum v2 is just a pool like now.
17:18:10 <abubakarsadiq> yay sjors is here!
17:18:16 <sjors47> But in Stratum v2 the pool needs to verify that the individual miner is not sending them a fake block.
17:18:28 <monlovesmango> is Unix socket more secure than TPC port? I have no idea why else IPC would be advantageous, just trying to ask questions
17:18:52 <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.
17:19:10 <sjors47> monlovesmango: I don't think the reason is security
17:19:25 <sjors47> It's just that Russ Ryanofsky, who's been working on multiprocess IPC for many years, added that
17:19:46 <abubakarsadiq> The next question
17:19:46 <abubakarsadiq> 5. What is the key difference between contextual and context-free checks in the block template validity checks?
17:19:48 <sjors47> It might be a bit more performant.
17:20:47 <sjors47> Continuing as myself under Sjors[m]
17:20:55 <Sjors[m]> If that works
17:21:04 <abubakarsadiq> It works!
17:21:12 <monlovesmango> contextual check references previous block headers
17:21:13 <dzxzg> From a high level, the no context checks in CheckBlock don't have a view of the chainstate, and the contextual checks do
17:21:14 <sebastianvstaa> context free: can be done without knowledge of blockchain current state
17:21:15 <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.
17:21:46 <abubakarsadiq> yep dzxzg
17:22:05 <abubakarsadiq> correct sebastianvstaa
17:22:11 <monlovesmango> santos: I see thanks!
17:22:27 <stringintech> Sjors[m] thanks! the PR description made sense to me; just got a bit lost with Sv2 terminology...
17:22:43 <monlovesmango> is IPC able to process multiple requests at the same time? RPC can't do that right?
17:22:56 <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
17:23:03 <abubakarsadiq> @sjors question why is POW not checked by the pool, but we check that the nBits are valid?
17:23:40 <Sjors[m]> POW is checked by the pool
17:23:53 <Sjors[m]> I dropped that check from the earlier version.
17:24:07 <Sjors[m]> One bit of context...
17:24:27 <Sjors[m]> nBits represents the "real" work that will be in the final block
17:24:37 <Sjors[m]> Shares however use a lower amount of work.
17:24:50 <abubakarsadiq> ohh yeah I locally have the previous version of the PR
17:24:52 <Sjors[m]> So the nBits value won't mach the actual work.
17:25:12 <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.
17:25:30 <Sjors[m]> So the pool needs to check the actual work in the share and pay rewards accordingly.
17:25:46 <monlovesmango> so the POW is checked by the pool outside of bitcoin core?
17:25:55 <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).
17:26:05 <Sjors[m]> monlovesmango: yes
17:26:34 <Sjors[m]> One way to describe a "share" would be as a weak block.
17:26:58 <Sjors[m]> (extremely weak)
17:27:12 <abubakarsadiq> thanks @sjors
17:27:28 <Sjors[m]> santos: you can even do a unix socket of SSH quite easily
17:27:32 <Sjors[m]> * over
17:27:33 <abubakarsadiq> next question
17:27:33 <abubakarsadiq> Is there a change in behavior for TestBlockValidity after this PR, or is it a pure refactor? If yes, what’s the change?
17:28:25 <sebastianvstaa> yes. Returns a boolean value and a string instead in BlockVAlidationState object
17:28:57 <sebastianvstaa> but I wonder why? Why a string instead of a state enum? Seems cumbersome
17:29:04 <dzxzg> It also no longer logs block invalidity reasons as errors
17:29:04 <monlovesmango> I thought it always returned bool?
17:29:15 <stringintech> i noticed two main changes; the error handling and the CheckBlock (a context-free check) which is called before the contextual header check.
17:29:15 <stringintech> and i guess the second one could be considered a behavioral change.
17:29:22 <Sjors[m]> BlockVAlidationState required a bunch of custom code to serialize over IPC
17:29:34 <abubakarsadiq> @sebastianvstaa: Is that a behavior change?
17:29:35 <abubakarsadiq> I think it's the same return values we get previously just in another as boolean and strings now
17:29:41 <santos> context-free checks (CheckBlock) are done first now
17:29:57 <dzxzg> monlovesmango: https://github.com/bitcoin/bitcoin/blob/14b8dfb2bd5e2ca2b7c0c9a7f7d50e1e60adf75c/src/rpc/mining.cpp#L725-L766
17:30:07 <abubakarsadiq> @sjors this brings us to another question  In commit cca5993b, can you clarify the note that serialization of BlockValidationState is “fragile”?
17:30:21 <Sjors[m]> I didn't write that note :-)
17:30:29 <Sjors[m]> I just saw it as an excuse to delete it.
17:30:33 <abubakarsadiq> :D
17:30:51 <Sjors[m]> For historical background how this PR came about...
17:31:03 <Sjors[m]> I initially didn't know about the proposal mode
17:31:16 <Sjors[m]> So I ended up implementing all these checks and then realize it was duplicate work
17:31:35 <abubakarsadiq> yeah I could not answer that as well maybe @ryanofsky can help here.
17:31:36 <Sjors[m]> So then I gradually changed my implementation to be closer to the original.
17:32:02 <Sjors[m]> So may be that my initial design just used a boolean return with a string message.
17:32:19 <dzxzg> I'm not too familiar with the IPC interface, but is serialization of complex C++ types a problem in general?
17:32:23 <Sjors[m]> But it had the benefit of not needing that BlockValidationState struct, so I kept it.
17:32:45 <abubakarsadiq> The next question is
17:32:45 <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?
17:33:03 <Sjors[m]> dzxzg: not sure about "complex" but basically every type has its own serialization logic.
17:33:18 <Sjors[m]> With some "universal" things like byte spans.
17:33:41 <Sjors[m]> So anything that can be serialzed on the p2p network can be handled in a similar way with no extra IPC code.
17:33:56 <Sjors[m]> But BlockValidationState is an internal type, so there's no serialization for it.
17:34:05 <monlovesmango> if checkblock is called for multiple blocks?
17:34:27 <Sjors[m]> See this commit for how it was serialized: https://github.com/bitcoin/bitcoin/pull/31981/commits/9822bd64d26ca056c0fe44e5e2b3e1f38e6021ef
17:34:34 <Sjors[m]> monlovesmango: one block at a time
17:34:46 <Sjors[m]> But potentially extremely frequently.
17:34:47 <stringintech> aren't we working with a fresh instance of a block each time we get into TestBlockValidity?
17:34:49 <dzxzg> So the main reason for the refactor is to drop the `CustomBuildMessage()` logic for BlockValidationState
17:35:11 <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...
17:35:19 <Sjors[m]> That would probably crash the node :-)
17:35:39 <Sjors[m]> But you spin up multiple nodes to distribute the checking work.
17:35:48 <abubakarsadiq> @monlovesmango: what do you mean by "called for multiple blocks? "
17:36:16 <Sjors[m]> stringintech: yes
17:36:57 <monlovesmango> that checkblock is called for different block candidates that are found very close in time
17:37:20 <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?
17:37:21 <stringintech> Sjors[m]: So we only need to slow things down here (implicitly create a queue)?
17:37:38 <Sjors[m]> abubakarsadiq: yes all that can happen at the same time
17:38:04 <Sjors[m]> Even without this feature, I ran into the problem with Stratum v2 that we braodcast our own blocks now.
17:38:14 <Sjors[m]> At the same time the pool is also broadcasting it
17:38:41 <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.
17:38:49 <Sjors[m]> I believe I took care of those races.
17:39:00 <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?
17:39:08 <Sjors[m]> Now with checkBlock we never broadcast it.
17:39:33 <monlovesmango> abubakarsadiq: gotcha, thanks
17:39:40 <dzxzg> Why does the PR move responsibility for locking cs_main from the caller to testblockvalidity
17:39:41 <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 ?
17:39:43 <abubakarsadiq> when called via getblocktemplate in proposal mode
17:40:25 <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.
17:40:43 <Sjors[m]> So probably they should run these checks on different nodes than they use for contructing their own (default) block.
17:41:20 <abubakarsadiq> Lets move to the next question
17:41:20 <abubakarsadiq> In commit 1d029c23, why do we assert that `fChecked` should return `false`? Is this a potential issue?
17:41:28 <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.
17:41:32 <Sjors[m]> santos: when the pool receives a block _proposal_
17:41:48 <abubakarsadiq> Note: this is in an earlier version of the PR
17:42:02 <Sjors[m]> I was going to say, I dropped some of those checks.
17:43:03 <abubakarsadiq> stringintech: I mentioned that will not be an issue for identical blocks when called via the RPC because we dont save the state.
17:44:04 <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.
17:45:28 <Sjors[m]> Mmm, one of the first checks if fChecked
17:45:31 <Sjors[m]> block.hashPrevBlock != *Assert(tip->phashBlock)
17:46:00 <Sjors[m]> And we've locked m_chainman by then
17:46:08 <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 ...)
17:46:33 <abubakarsadiq> This assertion ` Assert(!block.fChecked);`
17:46:40 <Sjors[m]> I initially wrote some assert and assume statement to sanity check my own changes.
17:47:32 <Sjors[m]> But this was a few months ago, so I don't remember either why that particular assert.
17:47:40 <Sjors[m]> And it's gone now.
17:48:16 <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
17:48:16 <abubakarsadiq> It is in this commit https://github.com/bitcoin-core-review-club/bitcoin/blob/1d029c23a143f1b9110377967a7c4a20ee75058d/src/validation.cpp#L4662
17:48:17 <dzxzg> in strings,
17:48:27 <monlovesmango> so can someone summarize what race condition we are trying to prevent when locking `cs_main` in `TestBlockValidity`?
17:49:06 <stringintech> monlovesmango: 👍
17:49:12 <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?
17:49:58 <monlovesmango> I understand the lock is needed to prevent tip from updating, but want to understand the race condition element too
17:50:36 <abubakarsadiq> dzxzg: what will be the benefit of that, how will the enum be useful, is not the same as the boolean output?
17:51:01 <stringintech> abubakarsadiq: Witness data is excluded when performing the check, but later in ContextualCheckBlock we consider it (block weight check).
17:51:03 <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
17:52:33 <abubakarsadiq> stringintech: we also do weight check there but why is it not enough?
17:54:23 <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.
17:54:47 <abubakarsadiq> monlovesmango: same also I don’t understand the second check: `block.vtx.size() * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT`
17:55:12 <Sjors[m]> dzxzg: the caller ultimately only gets a BIP22 string
17:55:29 <Sjors[m]> This was is the case the getblocktemplate RPC too.
17:55:33 <Sjors[m]> Enum is used internally.
17:55:55 <dzxzg> By caller I mean internal users
17:56:03 <Sjors[m]> Remember that RPC uses JSON, which is even worse in terms of what you can serialize
17:56:18 <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?
17:56:20 <Sjors[m]> dzxzg: true
17:58:01 <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.
17:58:19 <abubakarsadiq> But yeah we do check the witness weight as well
17:58:59 <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
17:59:18 <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
17:59:31 <Sjors[m]> So maybe it'll come back, we'll see
18:00:08 <abubakarsadiq> #endmeeting