--- Log opened Thu Nov 02 00:00:14 2023
01:06 -!- PaperSword [~Thunderbi@securemail.qrsnap.io] has quit [Quit: PaperSword]
01:19 -!- duderonomy [~duderonom@h66-220-99-202.bendor.broadband.dynamic.tds.net] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…]
02:53 -!- puchka [~puchka@185.203.122.108] has joined #bitcoin-core-pr-reviews
03:41 -!- abubakarsadiq [uid602234@id-602234.hampstead.irccloud.com] has joined #bitcoin-core-pr-reviews
04:15 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
06:00 -!- aaron [~aaron@104-177-184-13.lightspeed.miamfl.sbcglobal.net] has joined #bitcoin-core-pr-reviews
06:02 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has quit [Remote host closed the connection]
06:05 -!- __gotcha [~Thunderbi@79.132.249.232] has joined #bitcoin-core-pr-reviews
06:09 -!- __gotcha [~Thunderbi@79.132.249.232] has quit [Ping timeout: 240 seconds]
06:23 -!- kevkevin [~kevkevin@98.226.43.69] has joined #bitcoin-core-pr-reviews
06:28 -!- kevkevin [~kevkevin@98.226.43.69] has quit [Ping timeout: 260 seconds]
06:41 -!- kevkevin [~kevkevin@2601:241:8703:7b30:f4c2:34f3:3c5c:58b8] has joined #bitcoin-core-pr-reviews
06:44 -!- __gotcha [~Thunderbi@79.132.249.232] has joined #bitcoin-core-pr-reviews
06:48 -!- BrinkingChancell [~BrinkingC@2603-7000-4600-0500-e976-a925-df75-f931.res6.spectrum.com] has joined #bitcoin-core-pr-reviews
06:56 -!- pablomartin [~pablomart@92.118.61.223] has joined #bitcoin-core-pr-reviews
07:02 -!- kevkevin [~kevkevin@2601:241:8703:7b30:f4c2:34f3:3c5c:58b8] has quit [Remote host closed the connection]
07:02 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has joined #bitcoin-core-pr-reviews
07:45 -!- willcl-ark_ is now known as willcl-ark
07:45 -!- willcl-ark [~willcl-ar@user/willcl-ark/x-8282106] has changed host
07:48 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Remote host closed the connection]
07:49 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
08:45 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Remote host closed the connection]
08:46 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
08:51 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Ping timeout: 255 seconds]
09:05 -!- duderonomy [~duderonom@h66-220-99-202.bendor.broadband.dynamic.tds.net] has joined #bitcoin-core-pr-reviews
09:23 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
09:25 -!- BrinkingChancell [~BrinkingC@2603-7000-4600-0500-e976-a925-df75-f931.res6.spectrum.com] has left #bitcoin-core-pr-reviews []
09:28 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Ping timeout: 260 seconds]
09:28 -!- grettke [~grettke@065-026-198-174.biz.spectrum.com] has joined #bitcoin-core-pr-reviews
09:30 -!- BrinkingChancell [~BrinkingC@2603-7000-4600-0500-6934-55cb-c2af-cb3b.res6.spectrum.com] has joined #bitcoin-core-pr-reviews
09:30 -!- BrinkingChancell [~BrinkingC@2603-7000-4600-0500-6934-55cb-c2af-cb3b.res6.spectrum.com] has quit [Client Quit]
09:40 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
09:53 -!- vmammal [~vmammal@107.181.222.132] has joined #bitcoin-core-pr-reviews
10:00 < abubakarsadiq> #startmeeting
10:00 < maxedw> hi
10:00 < pablomartin> hello
10:00 < stickies-v> hi
10:00 < abubakarsadiq> hello everyone! welcome back to the second meeting about PR #28368. The notes and yesterday's discussion can be found at https://bitcoincore.reviews/28368.
10:00 < abubakarsadiq> We will be discussing the code review questions.
10:02 < maxedw> happy to be back, think I will be doing more reading and learning on this half
10:02 < abubakarsadiq> Lets jump right in to the next question
10:02 < abubakarsadiq> 9. In 4986edb, why are we adding a new callback `MempoolTransactionsRemovedForConnectedBlock` instead of using `BlockConnected`?
10:03 < abubakarsadiq> https://github.com/bitcoin-core-review-club/bitcoin/commit/4986edb99f8aa73f72e87f3bdc09387c3e516197 commit link
10:03 < maxedw> Is it because it returns the removed transactions directly and not a block?
10:03 < abubakarsadiq> yes @maxedw 
10:04 < maxedw> does the block have those too?
10:04 < pablomartin> the removal of the txs is happening before (on BlockConnected), so you get the size of them also
10:04 < abubakarsadiq> and also vtx does not have some transaction details like base fee.
10:04 < abubakarsadiq> It will not be desirable to modify `CBlock` vtx entries to include details we need for fee estimation.
10:05 < maxedw> understood
10:06 < abubakarsadiq> pablomartin: I dont think the order matters for fee estimator stats, it just needs the list of the transactions and fee details with the height they are/going to be removed from the mempool.
10:07 < abubakarsadiq> after new block is connected probably
10:07 -!- __gotcha [~Thunderbi@79.132.249.232] has quit [Ping timeout: 255 seconds]
10:07 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Remote host closed the connection]
10:08 < abubakarsadiq> it also needs to know when transactions are removed for other reasons apart from `BLOCK` 
10:08 < pablomartin> i see... i thought MempoolTransactionsRemovedForConnectedBlock was triggered after the txs were removed from the mempool... when BlockConnected... no?
10:08 < abubakarsadiq> it is triggered before removal with the mempool transactions that are going to be removed. 
10:09 < pablomartin> oh right...
10:09 < pablomartin> makes sense
10:09 < abubakarsadiq> next question
10:09 < abubakarsadiq> In https://github.com/bitcoin-core-review-club/bitcoin/commit/1d116df4c0e021c4c810450e3e5358f34d72940b, is `kernel/mempool_entry.h` the right place for `NewMempoolTransactionInfo`? What members are included in this struct, and why are they necessary?
10:10 < glozow> hi
10:11 < maxedw> TransactionRef, FeeAmount, VirtualTransactionSize and TxHeight are the members which I assume are needed for the estimation
10:12 < maxedw> I don't know if it's appropriate in `mempool_entry.h`
10:12 < pablomartin> regarding the right place for the struct... I thought due to CTxMemPoolEntry was there... and other mempool_* dont seem to be proper ones... not sure
10:13 < abubakarsadiq> I am thinking maybe NewMempoolTransactionInfo should be moved to `NewMempoolTransactionInfo` to `policy/fees.h` but there are changes needed now after glozow review, will split the struct for transaction addition and removal
10:14 < pablomartin> and src/txmempool.cpp?
10:15 < abubakarsadiq> but overall I think `kernel/mempool_entry.h` is the right place for now
10:16 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
10:16 < glozow> Wouldn't you have a circular dependency if you moved them to policy/fees?
10:16 < aaron> hello
10:17 < abubakarsadiq> yes @glozow, kernel/mempool_entry.h seems the best place for it
10:17 < glozow> makes sense to me
10:17 < abubakarsadiq> 11. Why can’t we use a `std::vector<CTxMempoolEntry>` as a parameter of `MempoolTransactionsRemovedForBlock` callback?
10:18 < pablomartin> oh sorry, said nonsense... that was the proper mempool.. :$
10:20 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Ping timeout: 240 seconds]
10:20 < abubakarsadiq> We only need a few of the fields in `CTxMempoolEntry`, why make a copy of the whole object `CTxMempoolEntry`
10:21 < maxedw> and it can't be shared because it's fired off on a new thread?
10:22 < vmammal> my guess was that `CTxMempoolEntry` isn't in scope at the right time after decoupling the fee estimator from mempool, not sure though
10:22 < abubakarsadiq> Yes, has to be copied I think
10:25 < abubakarsadiq> Copying it  for all the transactions that are going to be removed from the mempool after a new block connection might not be efficient, hence we create `NewMempoolTransactionInfo` and copy all the fields the fee estimator needs.
10:25 < abubakarsadiq> 12. How can you get the base fee of a `CTransactionRef`?
10:27 < vmammal> naively, sum(inputs) minus sum(outputs) but i feel like that's not the answer you're going for
10:27 < maxedw> I might be way off here but does it have anything to do with  the `Workspace` struct?
10:29 < abubakarsadiq> +1 vmammal sum of inputs values - the sum of output values, the value of the inputs are not available in the `CTransactionRef` you get the inputs values using the inputs transaction hash and index number.
10:29 < abubakarsadiq> No helper method for this for `CTransactionRef` thats why we are making copy of the base fee
10:31 < abubakarsadiq> maxedw no does not have to do with `Workspace` struct.
10:32 < abubakarsadiq> 13. In https://github.com/bitcoin-core-review-club/bitcoin/commit/ab4e250d1d209e0c79dba266461e6b0cfd670452#diff-c065d4cd2398ad0dbcef393c5dfc53f465bf44723348892395fffd2fb3bac522, is moving `removeTx` call to reason != `BLOCK` scope necessary? Is it fixing a bug?
10:35 < vmammal> i believe it is necessary
10:35 < vmammal> since tx are removed for block connected through another mechanism ?
10:36 < abubakarsadiq> yes, it is.
10:36 < abubakarsadiq> I wont say its a bug previously, because The reason why this passed before is that the fee estimator must have finished removing all txs whose reason is BLOCK before the mempool clears.
10:37 < abubakarsadiq> Any transaction whose removal reason is BLOCK will be removed by the fee estimator from the processBlock call in removeForBlock.
10:37 < abubakarsadiq> Having removeTx(hash, false); call outside reason != BLOCK is incorrect.
10:37 < abubakarsadiq> It is supposed to be in (reason != BLOCK) scope.
10:37 < abubakarsadiq> https://github.com/bitcoin/bitcoin/pull/10199#discussion_r113795130
10:38 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
10:38 < abubakarsadiq> So the fix is necessary now with this PR.
10:39 < vmammal> good catch
10:40 < pablomartin> thanks for the link
10:40 < abubakarsadiq> 14. Why is the fee estimator not tracking transactions with unconfirmed parents?
10:40 < aaron> yes thanks for the link
10:41 < abubakarsadiq> aaron also see https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1311941008
10:41 < aaron> will do thank you!
10:41 < vmammal> txs that are part of a package are evaluated on the ancestor fee of the package, so a single tx's fee isn't representative of the effective package rate
10:42 < abubakarsadiq> + vmammal the fee estimator is not package aware
10:42 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Ping timeout: 260 seconds]
10:42 < glozow> followup question: then should we ignore transactions with in-block children?
10:44 < abubakarsadiq> which means it only takes into account the transaction's actual fee rate always so transactions with mempool parents are suppossed to be tracked by their ancestor/mining score
10:44 < abubakarsadiq> good question glozow 
10:45 < vmammal> an unconfirmed tx with in-block children? im stumped
10:46 < glozow> Ah no. I mean when a transaction confirms, and has a child in the block
10:46 < pablomartin> if the fee estimator is not package aware... where the ancestor fee of package evaluation takes place?
10:46 < abubakarsadiq> no I think the question is asking should we track confirmed transactions in block that have children in that same block
10:47 < glozow> here: https://github.com/bitcoin/bitcoin/blob/7386da7a0b08cd2df8ba88dae1fab9d36424b15c/src/node/miner.cpp#L302
10:49 < abubakarsadiq> we should not track them as long as we confirm that they are added in that block because of their descendant fee. meaning they are a part of a package
10:49 < vmammal> i feel like yes, ignore the tx's fee if it has in-block children. but then we have the problem of fee estimator lacking sufficient data
10:49 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
10:49 < abubakarsadiq> But right now the fee estimator does that
10:51 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
10:52 < abubakarsadiq> vmammal some might have children but are not confirmed because of the child fee though
10:53 < abubakarsadiq> it is better to have less but accurate dat, than use inaccurate assumptions, pending to the time we make the fee estimator package aware
10:54 < vmammal> agreed
10:55 < abubakarsadiq> there is a PR that is attempting to do that with discussions going on
10:55 < abubakarsadiq> https://github.com/bitcoin/bitcoin/pull/25380
10:55 < abubakarsadiq> 15. In https://github.com/bitcoin-core-review-club/bitcoin/commit/79bcc5ca0679daf6e57fc4d7ce2244262a7cfd13, we pass a copy of transaction parents, `nSizeWithAncestors`, and `nModFeesWithAncestors` to `NewMempoolTransactionInfo`. Is there a better approach to get all that information?
10:57 < vmammal> what other option do you have than to copy?
10:57 < abubakarsadiq> this fields are passed to fix silent merge conflict with #25380, but arent used in this PR anyway.
10:57 < abubakarsadiq> I think its best to leave them out untill when needed
10:58 < abubakarsadiq> nope vmammal
10:59 < abubakarsadiq> Two minutes
10:59 < abubakarsadiq> last question
10:59 < abubakarsadiq> 16. In https://github.com/bitcoin-core-review-club/bitcoin/commit/79bcc5ca0679daf6e57fc4d7ce2244262a7cfd13, why are the block transactions and their ancestors not removed from the mempool in the first loop of `removeForBlock`?
11:00 < abubakarsadiq> #endmeeting
11:00 < maxedw> thanks abubakarsadiq and all!
11:00 < abubakarsadiq> The answer to that I think is Because some parents of a sibling transaction might be cleared from the mempool before we make the copy,its best we finish making the copies before clearing the mempool.
11:01 < abubakarsadiq> thanks everyone for attending and looking at the PR
11:01 < vmammal> thanks. cheers
11:01 < aaron> thank you very much everybody!
11:01 < pablomartin> thanks abubakarsadiq! thanks all! leaving with more doubts and questions but learnt a lot, many thanks!
11:03 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Remote host closed the connection]
11:19 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
11:24 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Ping timeout: 260 seconds]
11:29 -!- vmammal [~vmammal@107.181.222.132] has quit [Quit: Client closed]
11:35 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
11:53 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Remote host closed the connection]
11:54 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
11:59 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Ping timeout: 260 seconds]
12:03 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
12:04 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Remote host closed the connection]
12:04 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
12:09 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Ping timeout: 260 seconds]
12:19 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
13:01 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Remote host closed the connection]
13:03 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
13:07 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Ping timeout: 260 seconds]
13:10 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
13:17 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Ping timeout: 240 seconds]
13:41 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
13:46 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Ping timeout: 255 seconds]
13:57 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
13:59 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
14:01 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Remote host closed the connection]
14:02 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
14:09 -!- kevkevin [~kevkevin@2601:241:8703:7b30:e014:c77c:4638:c21c] has quit [Remote host closed the connection]
14:21 -!- aaron [~aaron@104-177-184-13.lightspeed.miamfl.sbcglobal.net] has quit [Ping timeout: 260 seconds]
14:57 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Remote host closed the connection]
14:57 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
15:16 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Remote host closed the connection]
15:17 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
15:21 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Ping timeout: 260 seconds]
15:29 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
15:57 -!- BrinkingChancell [~BrinkingC@2603-7000-4600-0500-e976-a925-df75-f931.res6.spectrum.com] has joined #bitcoin-core-pr-reviews
15:58 -!- BrinkingChancell [~BrinkingC@2603-7000-4600-0500-e976-a925-df75-f931.res6.spectrum.com] has quit [Client Quit]
16:40 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Remote host closed the connection]
16:41 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
16:46 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Ping timeout: 260 seconds]
17:14 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has joined #bitcoin-core-pr-reviews
17:18 -!- brunoerg [~brunoerg@mvx-177-92-76-170.mundivox.com] has quit [Ping timeout: 255 seconds]
17:49 -!- pablomartin [~pablomart@92.118.61.223] has quit [Ping timeout: 255 seconds]
18:53 -!- hernanmarino [~hernanmar@181.98.56.2] has quit [Ping timeout: 248 seconds]
18:57 -!- hernanmarino [~hernanmar@181.98.56.2] has joined #bitcoin-core-pr-reviews
19:13 -!- abubakarsadiq [uid602234@id-602234.hampstead.irccloud.com] has quit [Quit: Connection closed for inactivity]
19:31 -!- duderonomy [~duderonom@h66-220-99-202.bendor.broadband.dynamic.tds.net] has quit [Ping timeout: 260 seconds]
20:17 -!- pablomartin [~pablomart@92.118.61.217] has joined #bitcoin-core-pr-reviews
22:14 -!- pablomartin [~pablomart@92.118.61.217] has quit [Ping timeout: 272 seconds]
22:49 -!- kevkevin [~kevkevin@2600:1700:b30:47c0:a5:a765:4a98:ed99] has joined #bitcoin-core-pr-reviews
22:53 -!- kevkevin [~kevkevin@2600:1700:b30:47c0:a5:a765:4a98:ed99] has quit [Ping timeout: 248 seconds]
--- Log closed Fri Nov 03 00:00:16 2023