--- Log opened Wed Apr 30 00:00:29 2025
01:42 -!- kevkevin [~kevkevin@209.242.39.30] has joined #bitcoin-core-pr-reviews
01:46 -!- kevkevin [~kevkevin@209.242.39.30] has quit [Ping timeout: 260 seconds]
03:30 -!- kevkevin [~kevkevin@209.242.39.30] has joined #bitcoin-core-pr-reviews
03:34 -!- kevkevin [~kevkevin@209.242.39.30] has quit [Ping timeout: 252 seconds]
03:46 -!- kevkevin_ [~kevkevin@209.242.39.30] has joined #bitcoin-core-pr-reviews
03:50 -!- kevkevin_ [~kevkevin@209.242.39.30] has quit [Ping timeout: 260 seconds]
05:43 -!- abubakarsadiq [uid602234@id-602234.hampstead.irccloud.com] has joined #bitcoin-core-pr-reviews
06:11 -!- eugenesiegel [~eugenesie@user/eugenesiegel] has joined #bitcoin-core-pr-reviews
06:26 -!- eugenesiegel [~eugenesie@user/eugenesiegel] has quit [Ping timeout: 240 seconds]
06:32 -!- eugenesiegel [~eugenesie@user/eugenesiegel] has joined #bitcoin-core-pr-reviews
07:05 -!- pablomartin [~pablomart@86.red-79-150-165.dynamicip.rima-tde.net] has quit [Quit: ZNC 1.8.2+deb3.1+deb12u1 - https://znc.in]
07:35 -!- eugenesiegel [~eugenesie@user/eugenesiegel] has quit [Quit: Client closed]
07:35 -!- kevkevin [~kevkevin@209.242.39.30] has joined #bitcoin-core-pr-reviews
07:50 -!- eugenesiegel [~eugenesie@user/eugenesiegel] has joined #bitcoin-core-pr-reviews
07:59 -!- eugenesiegel [~eugenesie@user/eugenesiegel] has quit [Ping timeout: 240 seconds]
08:06 -!- Guest16 [~Guest16@static-102-213-241-213.as329174.net] has joined #bitcoin-core-pr-reviews
08:06 -!- Guest16 [~Guest16@static-102-213-241-213.as329174.net] has quit [Client Quit]
08:21 -!- eugenesiegel [~eugenesie@user/eugenesiegel] has joined #bitcoin-core-pr-reviews
08:47 -!- eugenesiegel [~eugenesie@user/eugenesiegel] has quit [Quit: Client closed]
08:55 -!- sebastianvstaa [~sebastian@45.86.202.131] has joined #bitcoin-core-pr-reviews
09:04 -!- pablomartin [~pablomart@217.130.254.81] has joined #bitcoin-core-pr-reviews
09:21 -!- Talkless [~Talkless@138.199.6.197] has joined #bitcoin-core-pr-reviews
09:32 -!- letim1291 [~letim1291@197.237.30.125] has joined #bitcoin-core-pr-reviews
09:34 -!- letim2 [~letim1291@197.237.30.125] has joined #bitcoin-core-pr-reviews
09:36 -!- letim2 [~letim1291@197.237.30.125] has quit [Client Quit]
09:49 -!- stringintech [~stringint@2602:fa59:3:451::1] has joined #bitcoin-core-pr-reviews
09:56 -!- monlovesmango [~monlovesm@154.47.22.81] has joined #bitcoin-core-pr-reviews
09:57 -!- dzxzg [~dzxzg@user/dzxzg] has joined #bitcoin-core-pr-reviews
09:58 -!- Guest16 [~Guest16@static-102-213-241-213.as329174.net] has joined #bitcoin-core-pr-reviews
10:00 < abubakarsadiq> #startmeeting
10:00 < corebot> abubakarsadiq: Meeting started at 2025-04-30T17:00+0000
10:00 < corebot> abubakarsadiq: Current chairs: abubakarsadiq
10:00 < corebot> abubakarsadiq: Useful commands: #action #info #idea #link #topic #motion #vote #close #endmeeting
10:00 < corebot> abubakarsadiq: See also: https://hcoop-meetbot.readthedocs.io/en/stable/
10:00 < corebot> abubakarsadiq: Participants should now identify themselves with '#here' or with an alias like '#here FirstLast'
10:00 < dzxzg> Hi
10:00 < sebastianvstaa> Hi
10:00 < abubakarsadiq> hi hi
10:00 -!- enochazariah [~enochazar@102.91.4.156] has joined #bitcoin-core-pr-reviews
10:00 < monlovesmango> hey
10:00 < Guest16> hi
10:01 < abubakarsadiq> welcome to this week review club session \o/
10:01 < abubakarsadiq> We will take a loot at #31981
10:01 < corebot> https://github.com/bitcoin/bitcoin/issues/31981 | Add checkBlock() to Mining interface by Sjors · Pull Request #31981 · bitcoin/bitcoin · GitHub
10:02 -!- stringintech [~stringint@2602:fa59:3:451::1] has quit [Ping timeout: 240 seconds]
10:02 < abubakarsadiq> The notes and question are available https://bitcoincore.reviews/31981
10:02 -!- Guest93 [~Guest93@197.248.59.233] has joined #bitcoin-core-pr-reviews
10:02 -!- pablomartin [~pablomart@217.130.254.81] has quit [Ping timeout: 260 seconds]
10:02 -!- stringintech [~stringint@151.233.164.7] has joined #bitcoin-core-pr-reviews
10:02 -!- Guest93 [~Guest93@197.248.59.233] has quit [Client Quit]
10:03 < abubakarsadiq> Did you review the PR or read the notes? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?
10:03 -!- Guest93 [~Guest93@197.248.59.233] has joined #bitcoin-core-pr-reviews
10:03 < monlovesmango> reviewed some of the PR, read the notes. concept ACK
10:04 -!- Guest93 is now known as Anney
10:04 < enochazariah> reviewed the PR partially, and read the notes as well. Conecpt ACK
10:04 < sebastianvstaa> concept ACK, approach ACK. Worked through the questions
10:04 < abubakarsadiq> nice 
10:04 < dzxzg> I reviewed the pr commits, and made some experimental changes concept ACK, with questions about the approach
10:04 < abubakarsadiq> Lets jump into the conceptual questions then.
10:04 < abubakarsadiq> Does an Sv2 TP client have to implement the new checkBlock method in the mining interface (yes/no)?
10:05 < sebastianvstaa> no
10:05 < monlovesmango> no
10: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
10:05 < stringintech> yet, so I might be missing something obvious.
10:05 < monlovesmango> its the Sv2 TP server that has to implement it right?
10:05 -!- santos [~santos@32.141.102.6] has joined #bitcoin-core-pr-reviews
10:06 < sebastianvstaa> It can still utilize the method provided by Bitcoin Core via IPC
10:06 < abubakarsadiq> yes sebastianvstaa: monlovesmango
10:06 < abubakarsadiq> TP does not need the checkBlock  method, I think it's the Sv2 server
10: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?
10:08 < monlovesmango> it seems to check for duplicate and duplicate-invalid blocks
10:08 < sebastianvstaa> block size limit, duplicate transactions
10:08 < monlovesmango> POW is not checked
10:08 < stringintech> Context-free checks like merle root, duplicate txs, block size, …
10:08 < stringintech> Contextual checks like block time, tx finality, whether block witness data has been tempered with, block weight, …
10: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.
10:09 < sebastianvstaa> yes, no PoW
10:09 -!- stringintech [~stringint@151.233.164.7] has quit [Quit: Client closed]
10:09 < sebastianvstaa> The idea is to check validity before you spend resources on mining
10:09 -!- stringintech [~stringint@151.233.164.7] has joined #bitcoin-core-pr-reviews
10:10 < monlovesmango> stringintech: very thorough and helpful answer :)
10:10 < abubakarsadiq> @stringintech yeah
10:10 -!- Guest36 [~Guest36@41.90.185.156] has joined #bitcoin-core-pr-reviews
10:11 < abubakarsadiq> sebastianvstaa: I am not sure if that is correct cc @sjors
10:12 < sebastianvstaa> abubakarsadiq what statement exactly?
10:12 < sebastianvstaa> check validity before mining?
10:12 < abubakarsadiq> your statement "The idea is to check validity before you spend resources on mining"
10:12 -!- PaperSword [~Thunderbi@securemail.qrsnap.io] has joined #bitcoin-core-pr-reviews
10:12 < stringintech> monlovesmango: 🙌🏻
10:13 < sebastianvstaa> what else would you do without PoW?
10:13 < abubakarsadiq> follow-up question why is POW not checked, but we check that the nBits are valid?
10:13 < abubakarsadiq> The next question is 
10: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?
10:14 -!- Guest70 [~Guest70@102.0.21.30] has joined #bitcoin-core-pr-reviews
10:14 -!- pseudoramdom [~pseudoram@2601:640:8b00:2d59:8062:3489:c56d:f6e8] has joined #bitcoin-core-pr-reviews
10:14 -!- Guest70 [~Guest70@102.0.21.30] has quit [Client Quit]
10:15 < stringintech> Not sure but if clients are already using IPC for mining related functions implementing block verification this way too would be easier.
10:15 -!- santos [~santos@32.141.102.6] has quit [Ping timeout: 240 seconds]
10:15 < sebastianvstaa> Better integration with the other components in the multiprocess project?
10:15 < monlovesmango> is IPC multi threaded? RPC seems to be single threaded, but maybe i'm wrong on that
10:15 -!- sjors47 [~sjors@2a10:3781:4c82:0:fda1:ce97:522e:508f] has joined #bitcoin-core-pr-reviews
10:15 < sjors47> sorry for the delay
10:15 < monlovesmango> does IPC expose a port to listen?
10:16 -!- santos [~santos@32.141.102.6] has joined #bitcoin-core-pr-reviews
10:16 -!- Guest16 [~Guest16@static-102-213-241-213.as329174.net] has quit [Ping timeout: 240 seconds]
10:16 -!- Guest36 [~Guest36@41.90.185.156] has quit [Quit: Client closed]
10:16 -!- enochazariah [~enochazar@102.91.4.156] has quit [Quit: Client closed]
10: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.
10:16 < sjors47> monlovesmangonot at the moment
10:16 < sjors47> monlovesmango: not at the moment
10:16 < sjors47> It listens on a Unix socket
10:16 -!- Guest16 [~Guest16@static-102-213-241-213.as329174.net] has joined #bitcoin-core-pr-reviews
10:17 < sjors47> Or maybe TPC port support as also been added.
10:17 -!- Anney [~Guest93@197.248.59.233] has quit [Quit: Client closed]
10:17 < sjors47> stringintech: a pool in stratum v2 is just a pool like now.
10:18 < abubakarsadiq> yay sjors is here!
10:18 < sjors47> But in Stratum v2 the pool needs to verify that the individual miner is not sending them a fake block.
10: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
10: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.
10:19 < sjors47> monlovesmango: I don't think the reason is security
10:19 < sjors47> It's just that Russ Ryanofsky, who's been working on multiprocess IPC for many years, added that
10:19 < abubakarsadiq> The next question
10:19 < abubakarsadiq> 5. What is the key difference between contextual and context-free checks in the block template validity checks?
10:19 < sjors47> It might be a bit more performant.
10:20 -!- Sjors[m] [~provooste@2620:6e:a000:ce11::1f] has joined #bitcoin-core-pr-reviews
10:20 -!- sipa [~pw@user/sipa] has joined #bitcoin-core-pr-reviews
10:20 < sjors47> Continuing as myself under Sjors[m]
10:20 < Sjors[m]> If that works
10:21 < abubakarsadiq> It works!
10:21 < monlovesmango> contextual check references previous block headers
10: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
10:21 < sebastianvstaa> context free: can be done without knowledge of blockchain current state
10: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.
10:21 -!- Guest95 [~Guest95@105.163.158.235] has joined #bitcoin-core-pr-reviews
10:21 < abubakarsadiq> yep dzxzg
10:22 -!- Guest95 [~Guest95@105.163.158.235] has quit [Client Quit]
10:22 < abubakarsadiq> correct sebastianvstaa
10:22 < monlovesmango> santos: I see thanks!
10:22 < stringintech> Sjors[m] thanks! the PR description made sense to me; just got a bit lost with Sv2 terminology...
10:22 -!- Guest31 [~Guest31@102.0.21.30] has joined #bitcoin-core-pr-reviews
10:22 < monlovesmango> is IPC able to process multiple requests at the same time? RPC can't do that right?
10: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
10:23 < abubakarsadiq> @sjors question why is POW not checked by the pool, but we check that the nBits are valid?
10:23 < Sjors[m]> POW is checked by the pool
10:23 < Sjors[m]> I dropped that check from the earlier version.
10:24 < Sjors[m]> One bit of context...
10:24 -!- sjors47 [~sjors@2a10:3781:4c82:0:fda1:ce97:522e:508f] has quit [Quit: Client closed]
10:24 < Sjors[m]> nBits represents the "real" work that will be in the final block
10:24 < Sjors[m]> Shares however use a lower amount of work.
10:24 < abubakarsadiq> ohh yeah I locally have the previous version of the PR
10:24 < Sjors[m]> So the nBits value won't mach the actual work.
10: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.
10:25 < Sjors[m]> So the pool needs to check the actual work in the share and pay rewards accordingly.
10:25 < monlovesmango> so the POW is checked by the pool outside of bitcoin core?
10: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).
10:26 < Sjors[m]> monlovesmango: yes
10:26 < Sjors[m]> One way to describe a "share" would be as a weak block.
10:26 < Sjors[m]> (extremely weak)
10:27 < abubakarsadiq> thanks @sjors
10:27 < Sjors[m]> santos: you can even do a unix socket of SSH quite easily
10:27 < Sjors[m]> * over
10:27 < abubakarsadiq> next question 
10: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?
10:28 < sebastianvstaa> yes. Returns a boolean value and a string instead in BlockVAlidationState object
10:28 < sebastianvstaa> but I wonder why? Why a string instead of a state enum? Seems cumbersome
10:29 < dzxzg> It also no longer logs block invalidity reasons as errors
10:29 < monlovesmango> I thought it always returned bool?
10: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.
10:29 < stringintech> and i guess the second one could be considered a behavioral change.
10:29 < Sjors[m]> BlockVAlidationState required a bunch of custom code to serialize over IPC
10:29 < abubakarsadiq> @sebastianvstaa: Is that a behavior change?
10:29 < abubakarsadiq> I think it's the same return values we get previously just in another as boolean and strings now
10:29 < santos> context-free checks (CheckBlock) are done first now
10:29 < dzxzg> monlovesmango: https://github.com/bitcoin/bitcoin/blob/14b8dfb2bd5e2ca2b7c0c9a7f7d50e1e60adf75c/src/rpc/mining.cpp#L725-L766
10:30 < abubakarsadiq> @sjors this brings us to another question  In commit cca5993b, can you clarify the note that serialization of BlockValidationState is “fragile”?
10:30 < Sjors[m]> I didn't write that note :-)
10:30 < Sjors[m]> I just saw it as an excuse to delete it.
10:30 < abubakarsadiq> :D
10:30 < Sjors[m]> For historical background how this PR came about...
10:31 < Sjors[m]> I initially didn't know about the proposal mode
10:31 < Sjors[m]> So I ended up implementing all these checks and then realize it was duplicate work
10:31 < abubakarsadiq> yeah I could not answer that as well maybe @ryanofsky can help here.
10:31 < Sjors[m]> So then I gradually changed my implementation to be closer to the original.
10:32 < Sjors[m]> So may be that my initial design just used a boolean return with a string message.
10:32 < dzxzg> I'm not too familiar with the IPC interface, but is serialization of complex C++ types a problem in general?
10:32 < Sjors[m]> But it had the benefit of not needing that BlockValidationState struct, so I kept it.
10:32 < abubakarsadiq> The next question is
10: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?
10:33 < Sjors[m]> dzxzg: not sure about "complex" but basically every type has its own serialization logic.
10:33 < Sjors[m]> With some "universal" things like byte spans. 
10: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.
10:33 < Sjors[m]> But BlockValidationState is an internal type, so there's no serialization for it.
10:34 < monlovesmango> if checkblock is called for multiple blocks?
10:34 < Sjors[m]> See this commit for how it was serialized: https://github.com/bitcoin/bitcoin/pull/31981/commits/9822bd64d26ca056c0fe44e5e2b3e1f38e6021ef
10:34 < Sjors[m]> monlovesmango: one block at a time
10:34 < Sjors[m]> But potentially extremely frequently.
10:34 < stringintech> aren't we working with a fresh instance of a block each time we get into TestBlockValidity?
10:34 < dzxzg> So the main reason for the refactor is to drop the `CustomBuildMessage()` logic for BlockValidationState
10: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...
10:35 < Sjors[m]> That would probably crash the node :-)
10:35 -!- pseudoramdom [~pseudoram@2601:640:8b00:2d59:8062:3489:c56d:f6e8] has quit [Ping timeout: 260 seconds]
10:35 < Sjors[m]> But you spin up multiple nodes to distribute the checking work.
10:35 < abubakarsadiq> @monlovesmango: what do you mean by "called for multiple blocks? "
10:36 < Sjors[m]> stringintech: yes
10:36 < monlovesmango> that checkblock is called for different block candidates that are found very close in time
10: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?
10:37 < stringintech> Sjors[m]: So we only need to slow things down here (implicitly create a queue)?
10:37 < Sjors[m]> abubakarsadiq: yes all that can happen at the same time
10:38 < Sjors[m]> Even without this feature, I ran into the problem with Stratum v2 that we braodcast our own blocks now.
10:38 < Sjors[m]> At the same time the pool is also broadcasting it
10: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. 
10:38 < Sjors[m]> I believe I took care of those races.
10: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?
10:39 < Sjors[m]> Now with checkBlock we never broadcast it.
10:39 < monlovesmango> abubakarsadiq: gotcha, thanks
10:39 < dzxzg> Why does the PR move responsibility for locking cs_main from the caller to testblockvalidity
10: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 ?
10:39 < abubakarsadiq> when called via getblocktemplate in proposal mode
10: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.
10:40 < Sjors[m]> So probably they should run these checks on different nodes than they use for contructing their own (default) block.
10:41 < abubakarsadiq> Lets move to the next question
10:41 < abubakarsadiq> In commit 1d029c23, why do we assert that `fChecked` should return `false`? Is this a potential issue?
10: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.
10:41 < Sjors[m]> santos: when the pool receives a block _proposal_
10:41 < abubakarsadiq> Note: this is in an earlier version of the PR
10:42 < Sjors[m]> I was going to say, I dropped some of those checks.
10: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.
10: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.
10:45 -!- letim1291 [~letim1291@197.237.30.125] has quit [Quit: Client closed]
10:45 < Sjors[m]> Mmm, one of the first checks if fChecked
10:45 < Sjors[m]> block.hashPrevBlock != *Assert(tip->phashBlock)
10:46 < Sjors[m]> And we've locked m_chainman by then
10: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 ...)
10:46 -!- Guest2 [~Guest29@static-108-53-246-133.nwrknj.fios.verizon.net] has joined #bitcoin-core-pr-reviews
10:46 < abubakarsadiq> This assertion ` Assert(!block.fChecked);`
10:46 < Sjors[m]> I initially wrote some assert and assume statement to sanity check my own changes.
10:47 -!- Guest16 [~Guest16@static-102-213-241-213.as329174.net] has quit [Quit: Client closed]
10:47 < Sjors[m]> But this was a few months ago, so I don't remember either why that particular assert.
10:47 < Sjors[m]> And it's gone now.
10:47 -!- Guest16 [~Guest16@static-102-213-241-213.as329174.net] has joined #bitcoin-core-pr-reviews
10: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
10:48 < abubakarsadiq> It is in this commit https://github.com/bitcoin-core-review-club/bitcoin/blob/1d029c23a143f1b9110377967a7c4a20ee75058d/src/validation.cpp#L4662
10:48 < dzxzg> in strings,
10:48 < monlovesmango> so can someone summarize what race condition we are trying to prevent when locking `cs_main` in `TestBlockValidity`?
10:48 -!- Guest16 [~Guest16@static-102-213-241-213.as329174.net] has quit [Client Quit]
10:49 < stringintech> monlovesmango: 👍
10: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?
10:49 < monlovesmango> I understand the lock is needed to prevent tip from updating, but want to understand the race condition element too
10:50 -!- Guest16 [~Guest16@static-102-213-241-213.as329174.net] has joined #bitcoin-core-pr-reviews
10:50 < abubakarsadiq> dzxzg: what will be the benefit of that, how will the enum be useful, is not the same as the boolean output?
10:51 < stringintech> abubakarsadiq: Witness data is excluded when performing the check, but later in ContextualCheckBlock we consider it (block weight check).
10: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
10:51 -!- Guest2 [~Guest29@static-108-53-246-133.nwrknj.fios.verizon.net] has quit [Quit: Client closed]
10:52 < abubakarsadiq> stringintech: we also do weight check there but why is it not enough?
10: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.
10:54 < abubakarsadiq> monlovesmango: same also I don’t understand the second check: `block.vtx.size() * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT`
10:55 < Sjors[m]> dzxzg: the caller ultimately only gets a BIP22 string
10:55 < Sjors[m]> This was is the case the getblocktemplate RPC too.
10:55 < Sjors[m]> Enum is used internally.
10:55 < dzxzg> By caller I mean internal users
10:56 < Sjors[m]> Remember that RPC uses JSON, which is even worse in terms of what you can serialize
10: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?
10:56 < Sjors[m]> dzxzg: true
10: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.
10:58 < abubakarsadiq> But yeah we do check the witness weight as well
10: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
10: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
10:59 < Sjors[m]> So maybe it'll come back, we'll see
11:00 < abubakarsadiq> #endmeeting
11:00 < dzxzg> Sjors[m]: I grant that I might be wrong, I'll look over russ's suggestion and experiment and come back to it, agreed (y)
11:00 < corebot> abubakarsadiq: Meeting ended at 2025-04-30T18:00+0000
11:00 < corebot> abubakarsadiq: Raw log: https://achow101.com/ircmeetings/2025/bitcoin-core-pr-reviews.2025-04-30_17_00.log.json
11:00 < corebot> abubakarsadiq: Formatted log: https://achow101.com/ircmeetings/2025/bitcoin-core-pr-reviews.2025-04-30_17_00.log.html
11:00 < corebot> abubakarsadiq: Minutes: https://achow101.com/ircmeetings/2025/bitcoin-core-pr-reviews.2025-04-30_17_00.html
11:00 < monlovesmango> abubakarsadiq: can you tell me what TX_NO_WITNESS does (in the third criteria of the size check)? i could not figure that out
11:00 < dzxzg> Thanks all, thanks Sjors[m], abubakarsadiq!
11:01 < monlovesmango> thanks abubakarsadiq and Sjors[m] for hosting!! learned a bit about Sv2!
11:01 < Sjors[m]> Thanks for having me!
11:01 < sebastianvstaa> Thanks all :)
11:02 < Sjors[m]> Feel free to re-ask some questions on the PR itself, as I'll be able to do a bit more digging and give more precise answers than in a chat.
11:02 -!- kevkevin [~kevkevin@209.242.39.30] has quit [Remote host closed the connection]
11:02 < stringintech> Thank you all!!
11:03 < abubakarsadiq> thanks all for joining, as sjors mentioned please leave review in the PR itself.
11:04 -!- sebastianvstaa [~sebastian@45.86.202.131] has quit [Quit: Client closed]
11:06 -!- Guest16 [~Guest16@static-102-213-241-213.as329174.net] has quit [Quit: Client closed]
11:21 -!- pablomartin [~pablomart@176.red-88-26-183.staticip.rima-tde.net] has joined #bitcoin-core-pr-reviews
11:22 -!- kevkevin [~kevkevin@142.147.59.145] has joined #bitcoin-core-pr-reviews
11:23 -!- stringintech [~stringint@151.233.164.7] has quit [Quit: Client closed]
11:25 -!- pablomartin [~pablomart@176.red-88-26-183.staticip.rima-tde.net] has quit [Ping timeout: 245 seconds]
11:30 -!- monlovesmango [~monlovesm@154.47.22.81] has quit [Quit: leaving]
11:43 -!- eugenesiegel [~eugenesie@user/eugenesiegel] has joined #bitcoin-core-pr-reviews
11:45 -!- dzxzg [~dzxzg@user/dzxzg] has quit [Ping timeout: 240 seconds]
11:47 -!- Guest31 [~Guest31@102.0.21.30] has quit [Quit: Client closed]
11:50 -!- pablomartin [~pablomart@86.red-79-150-165.dynamicip.rima-tde.net] has joined #bitcoin-core-pr-reviews
12:00 -!- eugenesiegel [~eugenesie@user/eugenesiegel] has quit [Quit: Client closed]
12:44 -!- santos [~santos@32.141.102.6] has quit [Quit: Client closed]
13:50 -!- kevkevin [~kevkevin@142.147.59.145] has quit [Remote host closed the connection]
13:50 -!- kevkevin [~kevkevin@142.147.59.145] has joined #bitcoin-core-pr-reviews
13:55 -!- kevkevin [~kevkevin@142.147.59.145] has quit [Ping timeout: 252 seconds]
13:56 -!- Talkless [~Talkless@138.199.6.197] has quit [Quit: Konversation terminated!]
14:48 -!- kevkevin [~kevkevin@209.242.39.30] has joined #bitcoin-core-pr-reviews
15:33 -!- abubakarsadiq [uid602234@id-602234.hampstead.irccloud.com] has quit [Quit: Connection closed for inactivity]
15:41 -!- TheRec [~toto@84-75-225-47.dclient.hispeed.ch] has joined #bitcoin-core-pr-reviews
15:41 -!- TheRec [~toto@user/therec] has changed host
15:42 -!- TheRec_ [~toto@84-75-225-47.dclient.hispeed.ch] has quit [Read error: Connection reset by peer]
16:42 -!- grettke [~grettke@syn-184-055-133-000.res.spectrum.com] has quit [Quit: grettke]
17:19 -!- kevkevin [~kevkevin@209.242.39.30] has quit [Remote host closed the connection]
17:28 -!- S3RK [~S3RK@user/s3rk] has quit [Ping timeout: 260 seconds]
17:29 -!- S3RK [~S3RK@user/s3rk] has joined #bitcoin-core-pr-reviews
17:49 -!- grettke [~grettke@syn-184-055-133-000.res.spectrum.com] has joined #bitcoin-core-pr-reviews
17:51 -!- grettke [~grettke@syn-184-055-133-000.res.spectrum.com] has quit [Client Quit]
17:58 -!- grettke [~grettke@syn-184-055-133-000.res.spectrum.com] has joined #bitcoin-core-pr-reviews
18:39 -!- kevkevin [~kevkevin@209.242.39.30] has joined #bitcoin-core-pr-reviews
18:43 -!- kevkevin [~kevkevin@209.242.39.30] has quit [Ping timeout: 260 seconds]
19:54 -!- grettke [~grettke@syn-184-055-133-000.res.spectrum.com] has quit [Quit: grettke]
20:05 -!- grettke [~grettke@syn-184-055-133-000.res.spectrum.com] has joined #bitcoin-core-pr-reviews
20:06 -!- grettke [~grettke@syn-184-055-133-000.res.spectrum.com] has quit [Client Quit]
21:02 -!- Talkless [~Talkless@138.199.6.197] has joined #bitcoin-core-pr-reviews
21:39 -!- Talkless [~Talkless@138.199.6.197] has quit [Quit: Konversation terminated!]
22:22 -!- instagibbs [~instagibb@pool-100-15-116-202.washdc.fios.verizon.net] has quit [Ping timeout: 252 seconds]
22:23 -!- jamesob15665 [~jamesob@pool-108-44-244-6.clppva.fios.verizon.net] has quit [Ping timeout: 252 seconds]
22:23 -!- jamesob443688173 [~jamesob@pool-108-44-244-6.clppva.fios.verizon.net] has quit [Ping timeout: 252 seconds]
22:36 -!- jamesob15665 [~jamesob@pool-108-44-244-6.clppva.fios.verizon.net] has joined #bitcoin-core-pr-reviews
22:36 -!- jamesob443688173 [~jamesob@pool-108-44-244-6.clppva.fios.verizon.net] has joined #bitcoin-core-pr-reviews
22:36 -!- instagibbs [~instagibb@pool-100-15-116-202.washdc.fios.verizon.net] has joined #bitcoin-core-pr-reviews
23:05 -!- jon_atack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
23:07 -!- jonatack [~jonatack@user/jonatack] has quit [Ping timeout: 252 seconds]
--- Log closed Thu May 01 00:00:31 2025