--- Day changed Wed Jul 17 2019
01:44 -!- jonatack [6dca67aa@109.202.103.170] has quit [Ping timeout: 260 seconds]     
03:12 -!- jonatack [56dedeb5@abayonne-654-1-79-181.w86-222.abo.wanadoo.fr] has joined #bitcoin-core-pr-reviews     
03:29 -!- michaelfolkson [~textual@92.19.76.0] has joined #bitcoin-core-pr-reviews     
03:29 -!- michaelfolkson [~textual@92.19.76.0] has quit [Client Quit]     
03:44 -!- jonatack [56dedeb5@abayonne-654-1-79-181.w86-222.abo.wanadoo.fr] has quit [Remote host closed the connection]     
03:52 -!- jonatack [56dedeb5@abayonne-654-1-79-181.w86-222.abo.wanadoo.fr] has joined #bitcoin-core-pr-reviews     
03:59 -!- michaelfolkson [~textual@92.19.76.0] has joined #bitcoin-core-pr-reviews     
04:18 -!- michaelfolkson [~textual@92.19.76.0] has quit [Quit: Sleep mode]     
04:51 -!- jonatack [56dedeb5@abayonne-654-1-79-181.w86-222.abo.wanadoo.fr] has quit [Ping timeout: 260 seconds]     
05:19 -!- michaelfolkson [~textual@46.233.77.203] has joined #bitcoin-core-pr-reviews     
06:55 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
07:21 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-wfcrhtuasdsmyjav] has joined #bitcoin-core-pr-reviews     
07:23 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 244 seconds]     
07:23 -!- jonatack [56dedeb5@abayonne-654-1-79-181.w86-222.abo.wanadoo.fr] has joined #bitcoin-core-pr-reviews     
07:34 -!- jonas___ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
07:50 -!- jonas____ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
07:51 -!- jonas___ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 272 seconds]     
07:51 -!- pinheadmz [~matthewzi@209.209.238.169] has quit [Quit: pinheadmz]     
08:01 -!- sdupre [48172556@dynamic-acs-72-23-37-86.zoominternet.net] has joined #bitcoin-core-pr-reviews     
08:07 -!- jonatack [56dedeb5@abayonne-654-1-79-181.w86-222.abo.wanadoo.fr] has quit [Ping timeout: 260 seconds]     
08:14 -!- jonas____ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 268 seconds]     
08:21 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
08:56 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 246 seconds]     
09:07 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
09:07 -!- michaelfolkson [~textual@46.233.77.203] has quit [Quit: Sleep mode]     
09:11 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 246 seconds]     
09:22 -!- pinheadmz [~matthewzi@209.209.238.169] has joined #bitcoin-core-pr-reviews     
09:22 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
09:27 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 272 seconds]     
09:31 -!- jonatack [58aba822@jau64-1-88-171-168-34.fbx.proxad.net] has joined #bitcoin-core-pr-reviews     
09:36 < jnewbery> we'll get started in about 25 minutes
09:36 < jnewbery> Notes here: https://bitcoin-core-review-club.github.io/15169.html . Please review before the meeting!
09:39 -!- jonatack [58aba822@jau64-1-88-171-168-34.fbx.proxad.net] has quit [Ping timeout: 260 seconds]     
09:40 -!- jonatack [8613b3a3@134.19.179.163] has joined #bitcoin-core-pr-reviews     
09:46 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
09:46 -!- PaulTroon [~paultroon@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
09:52 -!- RyanMilb [b588b44f@181.136.180.79] has joined #bitcoin-core-pr-reviews     
09:57 < digi_james>  Hello everyone :)
09:58 < jonatack> Hi James :)
09:59 < digi_james> jonatack: :)
09:59 < jnewbery> hi!     
10:01 < sosthene> hi     
10:01 < jnewbery> Hi folks. This week, digi_james has kindly offered to host the meeting. He wrote all the notes and questions at https://bitcoin-core-review-club.github.io/15169.html. Thanks James!
10:01 < jonas_> hi     
10:01 < ariard> hi     
10:01 < jnewbery> A reminder again that I'm always looking for suggestions for PRs to cover and volunteers to host. Please comment here: https://github.com/bitcoin-core-review-club/bitcoin-core-review-club.github.io/issues/14 or DM me for either of those.
10:01 < fjahr> hi     
10:01 < elichai2> Hi     
10:02 < jnewbery> It's a bit of work to prepare to host, but I think it's really worth it. You'll get a lot out of really digging into the PR and understanding it well enough to talk about. We already have lightlike and elichai2 lined up to host future meetings.
10:02 < sosthene> wow, that's great
10:02 < jnewbery> The PR this week is really interesting. Even though the number of lines touched is fairly small, it touches on some very important concepts: peer-to-peer, DoS protection and caching. I hadn't reviewed it before James suggested it for PR club, so thanks for pointing me at it!
10:02 < jnewbery> Ok, over to James.
10:03 < digi_james> Thanks John. This was a bit of a tricky PR for me to understand, but I hope you all will appreciate the nuances and perhaps even discover new ones today.
10:03 -!- gm7t2 [~gm7t2@2600:e000:5a:1498:4116:adab:5ff9:ad53] has joined #bitcoin-core-pr-reviews     
10:04 -!- csknk [~csknk@unaffiliated/csknk] has joined #bitcoin-core-pr-reviews     
10:04 < digi_james> I suppose on the surface it would appear to be a performance optimization, verifying inputs fo new transactions being accepted to the mempool in parallel.
10:04 < digi_james> So I suggest we start with that, and then move onto the parts which change behaviour afterwards ...
10:04 -!- lf [~lf@185.245.85.252] has joined #bitcoin-core-pr-reviews     
10:05 < digi_james> So perhaps I'll start out by asking how the parallelisation is achieved?
10:06 -!- michaelfolkson [~textual@84.252.240.63] has joined #bitcoin-core-pr-reviews     
10:06 < digi_james> Is it new code that is queuing the check jobs and executing these?
10:06 -!- gm7t2 [~gm7t2@2600:e000:5a:1498:4116:adab:5ff9:ad53] has quit [Read error: Connection reset by peer]     
10:07 < pinheadmz> no I think it is using something from block validation
10:07 < digi_james> pinheadmz: exactly
10:07 < ariard> well it's now used scriptcheckqueue to parallelize script verification, scriptcheckqueue was only used by ConnectBlock  before
10:07 < ariard> s/used/using/g
10:08 < pinheadmz> this applies to each tx entering the mempool 1 at a time though?
10:08 < pinheadmz> so a tx with 100 inputs can be checked in parrallel
10:08 < digi_james> pinheadmz: yes exactly, parallel input checks for a given transaction
10:09 < digi_james> Is there a difference in validation logic between the script check queue and previously?
10:10 < digi_james> so if you look at RunCheckInputsParallelMaybe
10:10 < jnewbery> Note that the CheckInputs() function is slightly confusingly named (and the comment above is incorrect in parts): https://github.com/bitcoin/bitcoin/blob/459baa1756b7f2d10d261daa0d0f5f4b91cef21f/src/validation.cpp#L1244
10:11 < jnewbery> The comment says "Check whether all inputs of this transaction are valid (no double spends, scripts & sigs, amounts)
10:11 < jnewbery> historically, that was true (it'd check amounts and doublespends). Now it only checks scripts and sigs.
10:11 < jnewbery> That's what's being done in parallel in this PR - script verification
10:12 < lf> It's not clear to me why tx input check can be parallelized: is it not the case that if I create a transaction that uses two identical inputs a parallelized function will return false-positive validity
10:12 < digi_james> Thx John I wasnt aware it didnt do contextual verification
10:13 < lf> ok I think I understand: this is just verify valid inputs, not verify valid transaction
10:14 < jnewbery> yeah, it used to call CheckTxInputs that does some checking of amounts, etc.
10:14 < lf> Verify valid inputs aka assert that script conditions being met are allowed
10:14 < jnewbery> That was removed here: https://github.com/bitcoin/bitcoin/commit/832e0744cb8b1e1625cdb19b257f97316ac16a90#diff-24efdb00bfbe56b140fb006b562cc70bL1164
10:14 < digi_james> John: If there are no contextual checks, how can it be paralellized? Inputs from a given tx double spending for example?
10:15 < jnewbery> All it's checking is the scripts. Those are completely independent from contextual information
10:16 < jnewbery> contextual checks and amount checks are done elsewhere
10:16 < lf> jnewbery so a broad assesment of the change is: "lets separate CheckTxInputs function into more indepedently functional pieces s.t. performance can increase"?
10:16 < digi_james> I see
10:16 < lf> e.g. "before CheckTxInputs can complete, CheckInputs needs to finish and that can be parallelized"?
10:16 < jnewbery> eg AcceptToMemoryPoolWorker() calls CheckTxInputs here: https://github.com/bitcoin/bitcoin/blob/459baa1756b7f2d10d261daa0d0f5f4b91cef21f/src/validation.cpp#L559
10:17 < jnewbery> lf: no, that change that I linked to is from an old PR
10:17 < lf> jnewbery ty, reviewing now
10:17 < jnewbery> this PR is simply changing AcceptToMemoryPool (ATMP) to call CheckInputs() with a checkqueue so that script verification for the different inputs happens in parallel
10:18 < hugohn> Q: is there any scenario where CCheckQueue (scriptcheckqueue) can contain more than one tx to be validated? Or is it guaranteed that the queue can only contain one pending tx at a time?
10:19 < digi_james> The ATMP function takes a TX as an argument
10:19 < digi_james> so it is the inputs of that transaction which are pushed the the CCheckQueue
10:19 < hugohn> yes, all inputs of a given tx
10:20 < digi_james> When the queue is then run, it processes all queued input checkscript jobs of a given transaction.
10:20 < hugohn> my question is is it possible to queue up vChecks for more than 1 tx into scriptcheckqueue? I assume the answer is no, but want to confirm
10:20 < digi_james> I think ATMP locks up cs_main so that would prevent other tx inputs to be queued in the meantime
10:21 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 272 seconds]     
10:21 < digi_james> Sorry it locks the cs.mempool
10:22 < digi_james> Ok, in the interest of time, I suggest also covering the behavioural changes this PR introduces, what are they?
10:22 < hugohn> I see. yes that makes sense. If check queue accepts more than 1 tx at a time, !control.Wait() would not make sense because the failure could be ambiguous (not sure which tx is invalid)
10:22 < digi_james> hugohn: Thats a good point
10:23 < ariard> hugohn: IMO CCheckQueue let you do that, but you have a wrapper around CCheckQueueControl to let verification showing up as atomic for callers
10:24 < jnewbery> hugohn: I believe during block validation, the inputs from all the txs are enqueued onto a single CCheckQueueControl and then dispatched, but like you and james have said, that can't happen for checking txs for mempool entry
10:24 < PaulTroon> hugohn: can a blockvalidation use the queue at the same time as a inputs from a tx is queued?
10:24 < digi_james> ariard: But can you call ATMP for multiple transactions concurrently?
10:24 < digi_james> It would seem the mempool lock acquisition prevents this
10:24 < hugohn> ariard: isn't CCheckQueueControl only to ensure all vChecks in the queue must be finished? it doesn't protect you against 2 concurrent txs in the queue
10:25 < jnewbery> ATMP holds the main lock, so it can only be running in one thread
10:26 < digi_james> jnewbery: can CCheckQueueControl allow two callers to dispatch separate job sets, returning the completion of each separately to each caller as ariard suggested?
10:26 < ariard> digi_james: you can call ATMP for multiple transactions concurrently but given there is only one set of ScriptCheck threads, CCheckQueueControl constructor should be pass the scriptcheckqueue to ensure there isn't conflict
10:27 < jonatack> ariard: bypass?
10:27 < ariard> I mean CCheckQueueControl constructor is locking the passed CCheckQueue
10:27 < ariard> jonatakc: bypass what you mean ? Using the raw scriptcheckqueue without CCheckQueueControl instanciation ?
10:27 < jonatack> ah, be passed
10:28 < ariard> yes sorry
10:28 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
10:29 < digi_james> Ok, I think we should move to the behavioural change regarding invalid transactions
10:29 < jnewbery> ariard: ATMP holds the main lock, so it can't be called concurrently
10:29 -!- michaelf_ [~textual@84.252.240.63] has joined #bitcoin-core-pr-reviews     
10:29 < jonatack> behavior changes: CheckInputs invokes scripts only once and always returns CONSENSUS for script failures. Ban behavior.
10:30 < digi_james> jonatack: Thank you!
10:30 < hugohn> jnewbery: sorry still a bit confused. I see 2 locks used in ATMP, cs_main and pool.cs. which lock prevents you against 1+ txs in the queue?
10:30 < ariard> jnewbery: ah thanks but so why you need to lock scriptcheckqueue in this case ?
10:30 -!- michaelfolkson [~textual@84.252.240.63] has quit [Read error: Connection reset by peer]     
10:30 < hugohn> cs_main right?
10:30 < digi_james> :)     
10:30 < jnewbery> ariard: not sure, perhaps just to be extra safe!
10:30 < jnewbery> let's move on to behavioural changes
10:30 -!- RyanMilb [b588b44f@181.136.180.79] has quit [Quit: Ping timeout (120 seconds)]     
10:31 < ariard> hugohn: in fact in ConnectBlock transaction are mixed, you may have multiplee txs in queue
10:31 < ariard> IMO it's ok because you want atomic validity at block granularity
10:31 < ariard> and not a transaction granularity like in ATMP
10:31 -!- diogorsergio [~diogorser@94.197.120.203.threembb.co.uk] has joined #bitcoin-core-pr-reviews     
10:32 -!- RyanMilb [b588b44f@181.136.180.79] has joined #bitcoin-core-pr-reviews     
10:32 < ariard> *at     
10:32 < hugohn> ariard: it does makes sense for ConnectBlock I think? because block is invalid as long as any tx in the block is invalid
10:32 < hugohn> right
10:32 < ariard> yes we agree on this
10:32 < jnewbery> hugohn ariard: please, let's move on
10:32 < digi_james> As jonatack mentioned, RunCheckInputsMaybeParallel can no longer differentiate between policy invalid and consensus invalid failures during input checks. Rather it just sets the invalid state to Consensus.
10:33 < digi_james> CheckInputs previously would run two script checks during failure. One with policy flags active, so checks for standardness, and if this failed, a second check would be run with consensus flags only.
10:33 < digi_james> Why cant we do this in the new parallelized case?
10:35 < digi_james> Ok, one way to look at is in the CheckQueue design, which was originally written to be called when blocks are connected, there only consensus verification matters, standardness applies to mempool acceptance only
10:36 < jnewbery> digi_james: I think we could call CheckInputs() twice after this PR, but it'd require some code rearrangement, and we'd lose some of the performance benefit of the PR
10:37 < digi_james> john: Interesting. So both with all inputs in vChecks?
10:37 < jnewbery> (ie call CheckInputs() with policy flags, and then a second time with consensus flags)
10:37 < jnewbery> right
10:38 < digi_james> I am thinking out loud. So this would potentially be more scriptchecks than in the single threaded case, albeit parallelized.
10:38 < jnewbery> CheckInputs() is a bit weird right now. We call it and say "please run the scripts with these flags". CheckInputs() runs the scripts with the flags we pass it, but if those fails, it runs the scripts _again_ with different flags
10:38 < michaelf_> <digi_james>: What does "when blocks are connected" mean?
10:38 < jnewbery> this PR removes that second script verification
10:38 < digi_james> michaelf_:  I mean when a block is connected to the tip of the chain.
10:39 < michaelf_> Ah ok thanks
10:39 < lf> chain tip update*?
10:39 < jnewbery> but it could also add back a second call to CheckInputs() with the consensus flags
10:39 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 245 seconds]     
10:39 < jnewbery> The reason Suhas didn't do that is contained in this comment: https://github.com/bitcoin/bitcoin/pull/15169#issuecomment-492232216
10:39 < ariard> jnewbery: or CheckInputs() may also push the script verification in pvChecks
10:39 < jnewbery> "The motivation for this is to mitigate adversarial behavior: in order to determine whether a transaction is invalid according to consensus rules (rather than policy) we end up validating a transaction's scripts twice, so an adversary trying a CPU DoS could construct transactions that only fail policy checks, but otherwise tie up our CPU for twice as long."
10:42 < digi_james> jnewbery: If we add a second CheckInputs run (in the new parallelized case) as you suggested above, how would this be resolved?
10:42 < jnewbery> how would what be resolved?
10:43 < jonatack> Unsure how it relates but Suhas also wrote: "AcceptToMemoryPoolWorker() already invokes CheckInputs multiple times, with different script flags, to detect certain kinds of segwit validation errors."
10:43 < digi_james> Transactions failing policy checks, but tying up CPU for a second round of checks
10:43 < jnewbery> the fact that a malicious peer can make us verify twice at no cost to themselves?
10:43 < jonatack> in the commit message of https://github.com/bitcoin/bitcoin/pull/15169/commits/5f4e514
10:43 < digi_james> jnewbery: exactly
10:43 < jnewbery> that's on open question. We probably want to deprioritize traffic from such peers
10:44 < jnewbery> jonatack: yes, that's right. There are 4 places that ATMP can call CheckInputs() I believe:
10:44 < jnewbery> first, when verifying against policy
10:45 < digi_james> btw I realize this PR review may be difficult to follow, it's not easy to understnad and as you can see there are remaining nuances I haven't fully processed myself. Sorry about that.
10:45 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
10:45 < jnewbery> if that fails, it calls CheckInputs twice more, to see if the failure was due to a segwit witness mutation
10:46 -!- michaelfolkson [~textual@82-132-221-34.dab.02.net] has joined #bitcoin-core-pr-reviews     
10:46 < jnewbery> if it succeeds, we call CheckInputs() again from CheckInputsFromMempoolAndCache()
10:46 < jnewbery> digi_james: don't apologise. It's a great PR to review!
10:47 < jonatack> jnewbery: thank you!
10:47 < digi_james> Are there any remaining questions in regards to the current PR state which we can cover?
10:47 < ariard> digi_james: that's fine you pick up a really nice one :)
10:47 < sosthene> I wonder why we check against policy before consensus rules? Wouldn't it make more sense the other way around?
10:47 < digi_james> policy is tighter than consensus
10:48 < digi_james> so consensus is a second check, with loosened rules if you will
10:48 -!- michaelf_ [~textual@84.252.240.63] has quit [Ping timeout: 244 seconds]     
10:48 < jnewbery> exactly right digi_james. The mainline case is that policy checking succeeds, in which case we don't need to check against consensus
10:48 < digi_james> a consensus failure will always be a policy failure, but not the other way around
10:48 < hugohn> nit: the (nScriptCheckThreads && !single_threaded_validation) check is used 3 times in RunCheckInputsMaybeParallel(). it seems suboptimal & makes things a bit hard to read.
10:49 < jonatack> digi_james: agree, great choice of PR :+1:
10:49 < sosthene> oh ok, I don't know why intuitively I was thinking it was the other way around (consensus tighter than policy)
10:49 < digi_james> jonatack: Thanks for joinin gus
10:49 < digi_james> *us     
10:49 < jnewbery> (but in fact we do also check against consensus rules later (in CheckInputsFromMempoolAndCache(), but that's for slightly different and obscure reasons, which I don't think we have time to get into!)
10:50 < jnewbery> (the comment here: https://github.com/bitcoin/bitcoin/blob/459baa1756b7f2d10d261daa0d0f5f4b91cef21f/src/validation.cpp#L770 explains why)
10:51 < sosthene> digi_james: thanks it was very interesting, it probably wasn't easy to get ready for this one :)
10:51 < sosthene> gotta go now, goodby everyone
10:51 < jonatack> jnewbery: how critical is the caching issue you mention: "I believe this breaks script caching - CheckInputs() will only cache script success if running non-parallel."
10:51 < jnewbery> jonatack: it's pretty bad!
10:52 < hugohn> I thought suhas addressed that?
10:52 < jnewbery> script caching means that once we've accepted the tx to our mempool, we don't need to revalidate the scripts when we receive a block with that tx
10:53 < jonatack> (link to jnewbery comment: https://github.com/bitcoin/bitcoin/pull/15169#discussion_r304044230)
10:53 < ariard> jnewbery: assuming script flags don't change right ?
10:53 < jnewbery> here's a blog post I wrote about script caching when it was introduced: https://bitcointechtalk.com/whats-new-in-bitcoin-core-v0-15-part-5-6a9cfa85821f
10:54 < jnewbery> ariard: exactly correct. The cache entry includes which script flags were used in verification
10:55 < jnewbery> that's one of the obscure reasons that we call CheckInputsFromMempoolAndCache() later - it's so we verify the scripts according to the consensus rules of the next block
10:55 < jnewbery> so according to that blog post, block validation is ~40-50% faster with script caching
10:56 < jonatack> How does this change affect RPC methods such as testmempoolaccept?
10:56 < jnewbery> (or put differently, breaking script caching would ~double block validation time)
10:57 < ariard> Hmmm verifying the scripts according to the consensus rules of the next block, it's really obscure it's in case of softfork ?
10:57 < digi_james> jonatack: as of the current PR, it will return consensus failure
10:57 < digi_james> with script errors, but the 2nd check for policy failure has been removed.
10:58 < digi_james> But the rpc call adds a single_threaded_validation argument, so the inputs are checked in sequence
10:58 < jonatack> thanks!
10:58 < digi_james> ....which allows the caller to receive specific script errors, unlike the parallel case
10:59 < digi_james> Previously, you would get policy or consensus failures
10:59 < jnewbery> ariard: yes that's particularly obscure, but just the fact that we're calling CheckInputs() again seems pretty weird until you realise it's specifically for caching
10:59 < digi_james> That no longer the case
10:59 < ariard> I mean CheckInputsFromMempoolAndCache seems only an optimization in case of softfork activation, which doesn't happen often,so you add a burden for every tx getting into memppol
10:59 < hugohn> jnewbery: the script caching is fixed with this commit, correct? https://github.com/bitcoin/bitcoin/pull/15169/commits/39dfbc9d5e58bdecac52267336ecf532c99de9a2
11:00 < jnewbery> ariard: no, it's an optimization for all txs
11:00 < hugohn> scriptExecutionCache.insert(hash_cache_entry); is called in the running parallel case
11:00 < jnewbery> before CheckInputsFromMempoolAndCache we haven't validated the scripts according to consensus rules
11:00 < ariard> but couldn't we cache at once in the first calls of CheckInputs ?
11:01 < ariard> oh gotcha it's because the check against consensus rules is only in case of standardness check failure
11:01 < jnewbery> ariard: no, because the first time round, if the scripts succeed verification with policy rules, we don't check against consensus rules
11:01 < jnewbery> exactly
11:01 < ariard> that's weird
11:02 < fjahr> Before everyone leaves, question on code structure/naming: I would like to understand the reasoning for wrapping CheckInputs() in RunCheckInputsMaybeParallel(). Rather than that I would probably have put that logic into ChecksInputs() itself and then have it delegate to CheckInputsParallel() or so. I would like to understand if there are other factors at play that I am not seeing, like keeping the changes in
11:02 < fjahr> CheckInputs() as minimal as possible for example. Then the chosen structure would make more sense to me.
11:02 -!- michaelfolkson [~textual@82-132-221-34.dab.02.net] has quit [Remote host closed the connection]     
11:03 < digi_james> +1     
11:03 < jnewbery> I also thought that was a bit weird, but didn't look into alternative ways of doing it
11:04 < digi_james> Ok, if you don't mind, Id suggest wrapping it up here.
11:04 < jnewbery> Thanks digi_james!
11:04 < PaulTroon> +1     
11:04 < digi_james> jnewbery: Cheer!
11:04 < digi_james> *Cheers
11:04 < jnewbery> Tough PR this week. Great job uncovering the subtleties.
11:04 < hugohn> wouldn't one reason be that CheckInputs() is also called by BlockConnected(), which already does things in parallel?
11:05 < jonatack> Thanks digi_james and everyone!
11:05 < hugohn> so if you put the parallel logic inside CheckInputs(), BlockConnected would have double parallelization? haha
11:05 < jnewbery> Next week, we probably have lightlike covering another Suhas PR: 15505 ([p2p] Request NOTFOUND transactions immediately from other outbound peers, when possible)
11:05 < jnewbery> I'll update https://bitcoin-core-review-club.github.io/ once we have confirmation
11:09 < jnewbery> meeting log is up: https://bitcoin-core-review-club.github.io/15169.html
11:10 -!- PaulTroo_ [~paultroon@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
11:10 -!- PaulTroon [~paultroon@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Read error: Connection reset by peer]     
11:14 -!- RyanMilb [b588b44f@181.136.180.79] has quit [Remote host closed the connection]     
11:17 -!- lf [~lf@185.245.85.252] has quit [Quit: lf]     
11:19 -!- diogorsergio [~diogorser@94.197.120.203.threembb.co.uk] has quit [Remote host closed the connection]     
11:20 -!- csknk [~csknk@unaffiliated/csknk] has quit [Quit: leaving]     
11:35 -!- PaulTroo_ [~paultroon@rrcs-184-74-240-156.nyc.biz.rr.com] has quit []     
11:35 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 248 seconds]     
11:39 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
11:53 -!- jonas_ [~jonas@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Quit: leaving]     
13:30 -!- jnewbery [~john@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Ping timeout: 246 seconds]     
13:31 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Ping timeout: 260 seconds]     
13:36 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews     
13:37 -!- jnewbery [~john@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews     
15:29 -!- sdupre [48172556@dynamic-acs-72-23-37-86.zoominternet.net] has quit [Remote host closed the connection]     
15:33 -!- jonatack [8613b3a3@134.19.179.163] has quit [Remote host closed the connection]     
17:01 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-wfcrhtuasdsmyjav] has quit [Quit: Connection closed for inactivity]     
17:08 -!- pinheadmz [~matthewzi@209.209.238.169] has quit [Quit: pinheadmz]     
17:53 -!- michaelfolkson [~textual@2a00:23c5:be04:e501:78ae:269d:7955:2d87] has joined #bitcoin-core-pr-reviews     
17:56 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-ssjdrqagpdvnttom] has joined #bitcoin-core-pr-reviews     
17:59 -!- pinheadmz [~matthewzi@c-73-92-181-51.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews     
18:31 -!- pinheadmz [~matthewzi@c-73-92-181-51.hsd1.ca.comcast.net] has quit [Quit: pinheadmz]     
18:55 -!- michaelfolkson [~textual@2a00:23c5:be04:e501:78ae:269d:7955:2d87] has quit [Quit: Sleep mode]     
19:12 -!- pinheadmz [~matthewzi@209.209.238.169] has joined #bitcoin-core-pr-reviews     
19:50 -!- pinheadmz [~matthewzi@209.209.238.169] has quit [Quit: pinheadmz]     
20:41 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-ssjdrqagpdvnttom] has quit [Quit: Connection closed for inactivity]     
20:44 -!- pinheadmz [~matthewzi@209.209.238.169] has joined #bitcoin-core-pr-reviews     
23:52 -!- pinheadmz [~matthewzi@209.209.238.169] has quit [Quit: pinheadmz]     
23:53 -!- hardforkthis [~hardforkt@li120-195.members.linode.com] has quit [Quit: The Lounge - https://thelounge.chat]     
23:53 -!- hardforkthis [~hardforkt@li120-195.members.linode.com] has joined #bitcoin-core-pr-reviews