--- Log opened Wed Nov 01 00:00:14 2023
00:24 -!- PaperSword [~Thunderbi@securemail.qrsnap.io] has quit [Remote host closed the connection]
01:38 -!- jonatack [~jonatack@user/jonatack] has quit [Read error: Connection reset by peer]
01:38 -!- jonatack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
03:52 -!- maxedw [sid625334@id-625334.uxbridge.irccloud.com] has joined #bitcoin-core-pr-reviews
04:42 -!- TheRec_ [~toto@84-75-225-47.dclient.hispeed.ch] has joined #bitcoin-core-pr-reviews
04:42 -!- TheRec [~toto@user/therec] has quit [Ping timeout: 255 seconds]
04:57 -!- abubakarsadiq [uid602234@id-602234.hampstead.irccloud.com] has joined #bitcoin-core-pr-reviews
06:01 -!- pablomartin [~pablomart@92.118.61.226] has joined #bitcoin-core-pr-reviews
06:10 -!- willcl-ark [~willcl-ar@user/willcl-ark/x-8282106] has quit [Quit: left]
06:20 -!- willcl-ark [~willcl-ar@cpc123780-trow7-2-0-cust177.18-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews
06:20 -!- willcl-ark [~willcl-ar@user/willcl-ark/x-8282106] has changed host
06:31 -!- willcl-ark [~willcl-ar@user/willcl-ark/x-8282106] has quit [Quit: left]
06:32 -!- willcl-ark [~willcl-ar@cpc123780-trow7-2-0-cust177.18-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews
06:32 -!- willcl-ark [~willcl-ar@user/willcl-ark/x-8282106] has changed host
06:58 -!- greypw2546002161 [~greypw254@grey.pw] has quit [Quit: I'll be back!]
06:58 -!- greypw2546002161 [~greypw254@grey.pw] has joined #bitcoin-core-pr-reviews
06:59 -!- greypw2546002161 [~greypw254@grey.pw] has quit [Client Quit]
07:01 -!- greypw2546002161 [~greypw254@grey.pw] has joined #bitcoin-core-pr-reviews
08:02 -!- dberkelmans [~dberkelma@mail.inad.nl] has joined #bitcoin-core-pr-reviews
08:05 -!- dberkelmans [~dberkelma@mail.inad.nl] has quit [Client Quit]
08:53 -!- dberkelmans [~dberkelma@mail.inad.nl] has joined #bitcoin-core-pr-reviews
08:55 -!- dberkelmans [~dberkelma@mail.inad.nl] has quit [Client Quit]
09:05 -!- PaperSword [~Thunderbi@securemail.qrsnap.io] has joined #bitcoin-core-pr-reviews
09:43 -!- dberkelmans [~dberkelma@2001:1c03:530d:8100:91a4:e3b2:2378:343a] has joined #bitcoin-core-pr-reviews
09:44 -!- aaron [~aaron@104-177-184-13.lightspeed.miamfl.sbcglobal.net] has joined #bitcoin-core-pr-reviews
09:54 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has joined #bitcoin-core-pr-reviews
09:54 < kevkevin> hi
09:55 < stickies-v> hey kevkevin
09:55 -!- vmammal [~vmammal@107.181.222.132] has joined #bitcoin-core-pr-reviews
09:57 < abubakarsadiq> minutes to go
09:58 -!- henmeh84 [~henning@2a02:8070:4686:d820:fb2b:a312:cfef:30c6] has joined #bitcoin-core-pr-reviews
10:00 < abubakarsadiq> #startmeeting
10:00 < kevkevin> hey guys
10:00 < willcl-ark> hi
10:00 < pablomartin> hello
10:00 < dberkelmans> hi
10:00 < stickies-v> hi
10:00 < maxedw> hi
10:00 < lightlike> Hi
10:00 < henmeh84> hi
10:01 < abubakarsadiq> welcome everyone! Today’s PR is #28368.
10:01 < abubakarsadiq> The notes and questions are available on https://bitcoincore.reviews/28368
10:01 -!- maxedw [sid625334@id-625334.uxbridge.irccloud.com] has quit [Quit: Updating details, brb]
10:01 -!- maxedw [sid625334@id-625334.uxbridge.irccloud.com] has joined #bitcoin-core-pr-reviews
10:01 < hernanmarino> Hello 
10:01 < abubakarsadiq> Anyone joining in for the first time?
10:02 < henmeh84> yes
10:02 < aaron> yes
10:02 < abubakarsadiq> Welcome @henmeh84 @aaron
10:02 < henmeh84> thank you. nice to be here
10:03 < abubakarsadiq> Did everyone get a chance to review the PR? How about a quick y/n from everyone
10:03 < aaron> thank you as well, very exited to be here
10:03 < willcl-ark> light y for me
10:03 < pablomartin> concept ack and trying to review the code while testing it at the moment... never been at this part of the code, very interesting and id like to dedicate more time to it. It seems there are a lot of benefits made by this change.
10:03 < hernanmarino> y. Light review, code review pending 
10:04 < kevkevin> I did very shortly
10:04 < stickies-v> mostly reviewed the notes/questions
10:04 < maxedw> y
10:04 < abubakarsadiq>  Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?
10:05 -!- BrinkingChancell [~BrinkingC@2603-7000-4600-0500-ecba-258b-c302-0a29.res6.spectrum.com] has joined #bitcoin-core-pr-reviews
10:05 < abubakarsadiq> lets jump right in
10:05 < hernanmarino> Approach ACK 
10:05 < maxedw> Concept ACK
10:05 < abubakarsadiq>  Why is it beneficial to remove CTxMempool dependency on CBlockPolicyEstimator?
10:06 < BrinkingChancell> utACK 79bcc5
10:06 < maxedw> It seems like when there are changes to the mempool a synchronous update of the PolicyEstimator fires off
10:07 < willcl-ark> Currently block processing (and subsequently relaying new blocks to peers) is blocked while we update fee estimations based on the new block, which is not ideal.
10:07 < glozow> hi
10:07 < BrinkingChancell> hi
10:08 < abubakarsadiq> yes @maxedw @willcl-ark, in master CTxMemPool updates the CBlockPolicyEstimator synchronously
10:08 < pablomartin> willcl-ark +1 also would be very useful in order to test other fee estimators (/PRs)
10:08 < BrinkingChancell> agreed, that we can get improved asynchronous processing. In the previous architecture, updating the fee estimator was a synchronous task carried out within the `ConnectTip` function series
10:09 < abubakarsadiq> yes @BrnkingChancell #28368 will enable the fee estimator to update asynchronously in the background and stop blocking connectTip (block processing) during the fee estimator updates
10:10 < abubakarsadiq> Adding other complex fee estimation stuff during updates will be efficient.
10:11 < maxedw> Isn't it also run synchronously when a tx is added or removed from the mempool? That seems less of a problem to me than during a new block.
10:11 < abubakarsadiq> This brings us to the second question
10:11 < abubakarsadiq> 2. Are there any benefits of the `CBlockPolicyEstimator` being a member of `CTxMempool` ?  Are there downsides to removing it?
10:11 < maxedw> just a general thought on synchronous code being simpler
10:12 < maxedw> it's much easier to reason about and to write without error
10:12 < BrinkingChancell> agreed that synchronous code is easier to reason about
10:12 < maxedw> Also if things are processed in their own thread, do we have to worry about accessing the required memory to make the fee estimations?
10:13 < willcl-ark> the estimator does enjoy the transaction metadata the CTxMempoolEntry's have, fee, ancestor info etc.
10:13 < glozow> yeah you don't need to make copies of all the info you need
10:13 < abubakarsadiq> @maxedw yes its also synchronous on tx removal
10:16 < abubakarsadiq> @will-clark thats why we now have NewMempoolTransactionInfo struct to pass copies of all the info the fee estimator needs
10:18 < BrinkingChancell> The downsides to removing it include that we have more code to read, write, and reason about. The asynchronous way might also be considered more complex. For instance, there might now be greater risk of inconsistency while doing asynchronous fee estimates
10:18 < abubakarsadiq> @maxedw in #28368 the call is processed in `CSchedular` thread,  all the info the fee estimation need are passed in the callback parameter
10:18 < abubakarsadiq> The fee estimator has its own version of the mempool `mapMempoolTxs` it does not have to worry about accessing memory
10:20 < willcl-ark> BrinkingChancell:  That more depends on whether the validation interface guarantees ordering of callbacks (which AFAIU it does), so IMO there shouldn't be an inconsistency as such introduced here?
10:21 < abubakarsadiq> +1 willcl-ark
10:21 < BrinkingChancell> ahh, I see. that makes sense
10:22 < abubakarsadiq> Also the fee estimator will now have limited access to transaction data during updates, this is okay because the fee estimator doesn't require all that information.
10:23 < abubakarsadiq> 3. The first attempt #11775 split the CValidationInterface into CValidationInterface and MempoolInterface. What is the distinction between the two interfaces?
10:24 < maxedw> The `CValidationInterface` seemed to focus on `BlockConnected` / `UpdatedBlockTip` whereas the `MempoolInterface` was for txs added and removed from the mempool. I'm not totally clear on the advantage of the split but I read in your notes that notifications could come from difference places such as mempool vs validation code and so the split could facilitate that.
10:27 < abubakarsadiq> @maxedw yes,  `CValidationInterface` callback notifications are fired from validation events such as block connection.
10:27 < abubakarsadiq> whereas `MempoolInterface` callback notifications are fired from mempool events such as adding/removing or transactions in the mempool
10:27 < BrinkingChancell> I had a similar understanding to maxedw.
10:27 < BrinkingChancell> I also had a question about this. Howcome it's not called `CMempoolInterface`? Do the class names follow different naming conventions for a technical reason? or is it something else?
10:28 < abubakarsadiq> The C is the legacy Hungarian naming convention used before
10:28 < maxedw> If it was going to be any prefix, I would have thought I for Interface
10:28 < maxedw> but it seems quite an old school microsofty thing to do
10:28 < maxedw> we used to do it in VB6 back in the day
10:30 < abubakarsadiq> see https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c
10:30 < abubakarsadiq> there is a disclaimer not to use that anymore
10:31 < abubakarsadiq> 4 What do you think is better or worse about the approach taken in this approach, versus the one taken in #11775 split the `CValidationInterface`?
10:31 < maxedw> the current approach is less code and perhaps a bit simpler?
10:32 < maxedw> but if these really are two concepts coming from two different places and they had started life as two things, would people recommend bringing them together?
10:32 < willcl-ark> Whilst it seems nice organisationally to split them, as mentioned in the original commit message they still had a shared backend (to remain well-ordered), and it doesn't seem to offer much practical benefit to split: https://github.com/bitcoin/bitcoin/pull/11775/commits/ae5e07196cd2693fbac601b68038cabc072eceac
10:33 < abubakarsadiq> @maxedw yes the code evolves since #11775, some of the changes have already been implemented, and this change is minimally scoped to making fee estimator update asynchronously in the background
10:33 < abubakarsadiq> +1 willcl-ark
10:33 < maxedw> minimal scope must help with getting it merged
10:33 < willcl-ark> ISTM like either approach could be taken in the long run, but as abubakarsadiq says this keeps the change as small as possible
10:34 < abubakarsadiq> 5. Can you list the other subscribers to `CValidationInterface`?
10:35 < maxedw> It was used in a few places, `mining.cpp` for BlockChecked and also in `NotificationsProxy`, `PeerManager` and `BaseIndex`.
10:36 < abubakarsadiq> +1 with `CZMQNotificationInterface`
10:36 < willcl-ark> Does the ZMQ interface rely on it? Not sure, but a few other managers e.g. PeerManager and some Indexes I guess?
10:37 < BrinkingChancell> I got BlockConnected, BlockDisconnected, UpdatedBlockTip, BlockChecked, NewPoWValidBlock, submitblock_StateCatcher, TestSubscriber, TestSubscriberNoop, OutpointsUpdater, TransactionsDelta, CZMQNotificationInterface
10:37 < BrinkingChancell> but not terribly confident in this answer
10:37 < BrinkingChancell> what's the best approach to find all subscribers to an interface?
10:37 < abubakarsadiq>  BlockConnected, BlockDisconnected, UpdatedBlockTip, BlockChecked, NewPoWValidBlock this are `CValidationInterface` callbacks
10:38 < abubakarsadiq> @willcl-ark `CZMQNotificationInterface` is a client of `CValidationInterface` AFAIU
10:39 < abubakarsadiq> @BrinkingChancell hint: find all subclasses of  `CValidationInterface`
10:41 < abubakarsadiq> this brings us to a similar question
10:41 < abubakarsadiq>  6. Why is implementing a `CValidationInterface` method equivalent to “subscribing to the event”
10:42 < maxedw> I didn't get to the bottom of that. I can see that a list of callbacks are kept and I know there is this scheduler thread but exactly how the whole thing hangs together I didn't get to.
10:43 < vmammal> 6. i also found this one challenging to articulate
10:45 < abubakarsadiq> All subclasses of  `CValidationInterface` are clients .  the subclass can implement `CValidationInterface` methods (callbacks)
10:45 < abubakarsadiq> Any implemented `CValidationInterface` method from a `CValidationInteface` subclass will be executed every time the method callback is fired using `CMainSignals`.
10:45 < abubakarsadiq> Callbacks are fired whenever the corresponding event occurs. check in `src/validation.cpp` and `src/txmempool.cpp`
10:47 < willcl-ark> that makes sense
10:47 < abubakarsadiq> specifically e.g https://github.com/bitcoin/bitcoin/blob/eca2e430acf50f11da2220f56d13e20073a57c9b/src/txmempool.cpp#L504C16-L504C16
10:47 < maxedw> I'm not fully there yet on that one, will have to read the code a bit more
10:49 < maxedw> in my mind the subclasses are their own objects so I don't really get how something knows to call them
10:49 < willcl-ark> So you subclass CValidationInterface, override the methods you want callbacks for, and then register you interface to be called back every time that event fires from a signal?
10:49 < maxedw> I think it's that register step I'm missing
10:49 < abubakarsadiq> Another thing to note is that The callbacks can either be executed synchronously or asynchronously depending on the callback whenever they are fired
10:50 < abubakarsadiq> @maxedw I added a point about the subscription in the notes I think
10:51 < maxedw> thank you I will have a read
10:52 < abubakarsadiq> `7. BlockConnected` and `NewPoWValidBlock` are different callbacks. Which one is asynchronous and which one is synchronous? How can you tell?
10:53 < maxedw> `BlockConnected` is asynchronous. I know this because that's what the comment said :p
10:53 < BrinkingChancell> same!
10:54 < willcl-ark> as good-a-way to find an answer as any I've heard.
10:54 < maxedw> the method signatures looked quite similar so I couldn't tell much from that
10:54 < abubakarsadiq> what about `NewPoWValidBlock`
10:55 < BrinkingChancell> the `BlockConnected` function has a lambda expression that is passed to an event queuing mechanism
10:55 < abubakarsadiq> +1 bingo
10:56 < abubakarsadiq> All asynchronous callbacks have a docstring indicating that they are executed in the background. 
10:56 < abubakarsadiq> Furthermore, it goes down to the way the callback `CMainSignals` in `validationinterface.cpp` method is defined, all synchronous callbacks are executed while asynchronous callbacks are added to the processing queue to be executed asynchronously by the `CSchedular` thread while the thread that fired the event continues its execution.
10:56 < abubakarsadiq> see the difference between https://github.com/bitcoin/bitcoin/blob/d53400e75e2a4573229dba7f1a0da88eb936811c/src/validationinterface.cpp#L260 and https://github.com/bitcoin/bitcoin/blob/d53400e75e2a4573229dba7f1a0da88eb936811c/src/validationinterface.cpp#L227
10:56 < pablomartin> ah!
10:56 < abubakarsadiq> `NewPoWValidBlock` is synchronous
10:57 < BrinkingChancell> `NewPoWValidBlock` is synchronous. There is no mention of callbacks, promises, async/await patterns..
10:57 < maxedw> the one calls `ENQUEUE_AND_LOG_EVENT` and the other doesn't
11:00 < willcl-ark> So net_processing::PeerManager is the only one using synchronous version, for better relay performance?
11:00 < abubakarsadiq> #endmeeting
11:00 < willcl-ark> thanks abubakarsadiq ! Very interesting area of the codebase!
11:01 < abubakarsadiq> seems like it @willcl-ark
11:01 < BrinkingChancell> thanks everyone! this was very helpful
11:01 < maxedw> thanks abubakarsadiq! Very good pacing of questions.
11:01 < maxedw> and thanks everyone else for helping me understand better
11:02 < pablomartin> thanks abubakarsadiq and all!
11:02 < abubakarsadiq> We should meet again same time tomorrow to discuss some code review questions,
11:02 < abubakarsadiq> Thank you all for attending and reviewing the PR
11:02 < hernanmarino> Thanks ! See you next time 
11:02 < dberkelmans> Thanks
11:02 < BrinkingChancell> will be here tomorrow!
11:02 < henmeh84> Thank you
11:03 < willcl-ark> it's once a month BrinkingChancell :'(
11:03 < willcl-ark> or is it fortnightly. Check bitcoincore.reviews !
11:03 < hernanmarino> abubakarsadiq: Tomorrow is impossible for me, but will code review on github soon
11:03 < willcl-ark> oh sorry, missed abubakarsadiq message
11:03 < pablomartin> abubakarsadiq, BrinkingChancell: I'm ok with that
11:05 < aaron> Very informative meeting everyone! will be here tomorrow same time!
11:09 -!- vmammal [~vmammal@107.181.222.132] has quit [Quit: Client closed]
11:14 -!- PaperSword [~Thunderbi@securemail.qrsnap.io] has quit [Quit: PaperSword]
11:17 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
11:34 -!- PaperSword [~Thunderbi@securemail.qrsnap.io] has joined #bitcoin-core-pr-reviews
11:38 -!- GxqoRR [~GxqoRR@136.27.41.156] has joined #bitcoin-core-pr-reviews
11:38 -!- GxqoRR [~GxqoRR@136.27.41.156] has quit [Client Quit]
11:44 -!- PaperSword [~Thunderbi@securemail.qrsnap.io] has quit [Quit: PaperSword]
11:45 -!- PaperSword [~Thunderbi@securemail.qrsnap.io] has joined #bitcoin-core-pr-reviews
12:16 -!- henmeh84 [~henning@2a02:8070:4686:d820:fb2b:a312:cfef:30c6] has quit [Quit: Leaving]
12:22 -!- dberkelmans [~dberkelma@2001:1c03:530d:8100:91a4:e3b2:2378:343a] has quit [Quit: Client closed]
13:14 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
13:24 -!- abubakarsadiq [uid602234@id-602234.hampstead.irccloud.com] has quit [Quit: Connection closed for inactivity]
13:29 -!- BrinkingChancell [~BrinkingC@2603-7000-4600-0500-ecba-258b-c302-0a29.res6.spectrum.com] has quit [Quit: Client closed]
13:30 -!- abubakarsadiq [uid602234@id-602234.hampstead.irccloud.com] has joined #bitcoin-core-pr-reviews
13:38 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has quit [Remote host closed the connection]
13:39 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has joined #bitcoin-core-pr-reviews
13:44 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has quit [Ping timeout: 255 seconds]
13:55 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has joined #bitcoin-core-pr-reviews
14:21 < stickies-v> today's logs are available for perusal already: https://bitcoincore.reviews/28368 - we're back tomorrow at 17:00 UTC for the second part!
14:36 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has quit [Remote host closed the connection]
14:51 -!- dlb76 [~dlb76@user/dlb76] has quit [Ping timeout: 240 seconds]
14:51 -!- dlb76 [~dlb76@user/dlb76] has joined #bitcoin-core-pr-reviews
14:58 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has joined #bitcoin-core-pr-reviews
15:28 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has quit [Remote host closed the connection]
15:30 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has joined #bitcoin-core-pr-reviews
15:34 -!- aaron [~aaron@104-177-184-13.lightspeed.miamfl.sbcglobal.net] has quit [Ping timeout: 260 seconds]
15:35 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has quit [Ping timeout: 260 seconds]
16:02 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has joined #bitcoin-core-pr-reviews
16:07 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has quit [Ping timeout: 260 seconds]
16:48 -!- lbia [~lbia@user/lbia] has quit [Quit: lbia]
17:08 -!- lbia [~lbia@user/lbia] has joined #bitcoin-core-pr-reviews
17:14 -!- willcl-ark [~willcl-ar@user/willcl-ark/x-8282106] has quit [Ping timeout: 255 seconds]
17:17 -!- willcl-ark [~willcl-ar@cpc123780-trow7-2-0-cust177.18-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews
17:17 -!- willcl-ark [~willcl-ar@user/willcl-ark/x-8282106] has changed host
17:18 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has joined #bitcoin-core-pr-reviews
17:24 -!- willcl-ark_ [~willcl-ar@cpc123780-trow7-2-0-cust177.18-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews
17:24 -!- willcl-ark [~willcl-ar@user/willcl-ark/x-8282106] has quit [Ping timeout: 264 seconds]
19:44 -!- abubakarsadiq [uid602234@id-602234.hampstead.irccloud.com] has quit [Quit: Connection closed for inactivity]
22:26 -!- PaperSword [~Thunderbi@securemail.qrsnap.io] has quit [Quit: PaperSword]
22:29 -!- PaperSword [~Thunderbi@securemail.qrsnap.io] has joined #bitcoin-core-pr-reviews
22:38 -!- pablomartin [~pablomart@92.118.61.226] has quit [Ping timeout: 248 seconds]
22:39 -!- grettke [~grettke@065-026-198-174.biz.spectrum.com] has quit [Quit: grettke]
--- Log closed Thu Nov 02 00:00:14 2023