Fee Estimator updates from Validation Interface/CScheduler thread (tx fees and policy)


Host: ismaelsadeeq  -  PR author: ismaelsadeeq

The PR branch HEAD was 79bcc5ca0679daf6e57fc4d7ce2244262a7cfd13 at the time of this review club meeting.


  • The CBlockPolicyEstimator is used to estimate what feerate a user should place on their transaction to be confirmed within a targeted number of blocks. RPCs estimaterawfees and estimatesmartfees can be used to obtain a feerate estimate for a given confirmation target. Fee estimation is also heavily used in the wallet.

  • Before this PR, CTxMempool owns a CBlockPolicyEstimator as one of its members and, whenever a transaction is added or removed from the mempool in addUnchecked, removedUnchecked and removeForBlock methods, it also updates the fee estimator synchronously.

  • removeForBlock calls CBlockPolicyEstimator::processBlock to update the fee stats when transactions are removed from the mempool after a new block connection.

  • Since removeForBlock is part of a series of function calls within ConnectTip, block processing is blocked by fee estimator updates. Adding more steps to CBlockPolicyEstimator::processBlock can slow down block processing even further.

  • CValidationInterface includes validation-related events such as addition of a transaction to the mempool TransactionAddedToMempool, the removal of a transaction from the mempool TransactionRemovedFromMempool, the connection of a block to the chain BlockConnected, and others.

    • Subclasses of CValidationInterface are called subscribers or clients of CValidationInterface. They can implement the callbacks they need and must register with CMainSignals using RegisterValidationInterface. Validation and mempool use CMainSignals to “notify” subscribers of these events.

    • Depending on the event, the callbacks may be executed immediately or added to a queue of callbacks to be executed asynchronously on a different thread.

  • PR #28368 removes CTxMempool’s’ dependency on CBlockPolicyEstimator by making CBlockPolicyEstimator a client of CValidationInterface, instead of an internal component of CTxMemPool.

    • The PR makes CBlockPolicyEstimator update asynchronously in response to CValidationInterface events, which means updating the fee estimator no longer blocks block processing.

    • The PR also adds a new CValidationInterface callback MempoolTransactionsRemovedForConnectedBlock which is fired in removeForBlock.

    • The PR also modifies the TransactionAddedToMempool callback parameter from CTransactionRef to a new struct NewMempoolTransactionInfo which has CTransactionRef and other information the fee estimator needs such as the virtual size and fees.

  • There has been an attempt at this PR #11775



  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?

  2. Why is it beneficial to remove CTxMempool dependency on CBlockPolicyEstimator?

  3. Are there any benefits of the CBlockPolicyEstimator being a member of CTxMempool? Are there downsides to removing it?

  4. The first attempt #11775 split the CValidationInterface into CValidationInterface and MempoolInterface. What is the distinction between the two interfaces?

  5. What do you think is better or worse about the approach taken in this approach, versus the one taken in #11775 split the CValidationInterface?

  6. Can you list the other subscribers to CValidationInterface?

  7. Why is implementing a CValidationInterface method equivalent to “subscribing to the event”?

  8. BlockConnected and NewPoWValidBlock are different callbacks. Which one is asynchronous and which one is synchronous? How can you tell?

Code Review

  1. In 4986edb, why are we adding a new callback MempoolTransactionsRemovedForConnectedBlock instead of using BlockConnected?

  2. In 1d116df, is kernel/mempool_entry.h the right place for NewMempoolTransactionInfo? What members are included in this struct, and why are they necessary?

  3. Why can’t we use a std::vector<CTxMempoolEntry> as a parameter of MempoolTransactionsRemovedForBlock callback?

  4. How can you get the base fee of a CTransactionRef?

  5. In ab4e250, is moving removeTx call to reason != BLOCK scope necessary? Is it fixing a bug?

  6. Why is the fee estimator not tracking transactions with unconfirmed parents?

  7. In 79bcc5c, we pass a copy of transaction parents, nSizeWithAncestors, and nModFeesWithAncestors to NewMempoolTransactionInfo. Is there a better approach to get all that information?

  8. In 79bcc5c, why are the block transactions and their ancestors not removed from the mempool in the first loop of removeForBlock?

Meeting 1

  117:00 <abubakarsadiq> #startmeeting
  217:00 <kevkevin> hey guys
  317:00 <willcl-ark> hi
  417:00 <pablomartin> hello
  517:00 <dberkelmans> hi
  617:00 <stickies-v> hi
  717:00 <maxedw> hi
  817:00 <lightlike> Hi
  917:00 <henmeh84> hi
 1017:01 <abubakarsadiq> welcome everyone! Today’s PR is #28368.
 1117:01 <abubakarsadiq> The notes and questions are available on https://bitcoincore.reviews/28368
 1217:01 <hernanmarino> Hello
 1317:01 <abubakarsadiq> Anyone joining in for the first time?
 1417:02 <henmeh84> yes
 1517:02 <aaron> yes
 1617:02 <abubakarsadiq> Welcome @henmeh84 @aaron
 1717:02 <henmeh84> thank you. nice to be here
 1817:03 <abubakarsadiq> Did everyone get a chance to review the PR? How about a quick y/n from everyone
 1917:03 <aaron> thank you as well, very exited to be here
 2017:03 <willcl-ark> light y for me
 2117: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.
 2217:03 <hernanmarino> y. Light review, code review pending
 2317:04 <kevkevin> I did very shortly
 2417:04 <stickies-v> mostly reviewed the notes/questions
 2517:04 <maxedw> y
 2617:04 <abubakarsadiq> Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?
 2717:05 <abubakarsadiq> lets jump right in
 2817:05 <hernanmarino> Approach ACK
 2917:05 <maxedw> Concept ACK
 3017:05 <abubakarsadiq> Why is it beneficial to remove CTxMempool dependency on CBlockPolicyEstimator?
 3117:06 <BrinkingChancell> utACK 79bcc5
 3217:06 <maxedw> It seems like when there are changes to the mempool a synchronous update of the PolicyEstimator fires off
 3317: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.
 3417:07 <glozow> hi
 3517:07 <BrinkingChancell> hi
 3617:08 <abubakarsadiq> yes @maxedw @willcl-ark, in master CTxMemPool updates the CBlockPolicyEstimator synchronously
 3717:08 <pablomartin> willcl-ark +1 also would be very useful in order to test other fee estimators (/PRs)
 3817: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
 3917: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
 4017:10 <abubakarsadiq> Adding other complex fee estimation stuff during updates will be efficient.
 4117: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.
 4217:11 <abubakarsadiq> This brings us to the second question
 4317:11 <abubakarsadiq> 2. Are there any benefits of the `CBlockPolicyEstimator` being a member of `CTxMempool` ? Are there downsides to removing it?
 4417:11 <maxedw> just a general thought on synchronous code being simpler
 4517:12 <maxedw> it's much easier to reason about and to write without error
 4617:12 <BrinkingChancell> agreed that synchronous code is easier to reason about
 4717: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?
 4817:13 <willcl-ark> the estimator does enjoy the transaction metadata the CTxMempoolEntry's have, fee, ancestor info etc.
 4917:13 <glozow> yeah you don't need to make copies of all the info you need
 5017:13 <abubakarsadiq> @maxedw yes its also synchronous on tx removal
 5117:16 <abubakarsadiq> @will-clark thats why we now have NewMempoolTransactionInfo struct to pass copies of all the info the fee estimator needs
 5217: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
 5317: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
 5417:18 <abubakarsadiq> The fee estimator has its own version of the mempool `mapMempoolTxs` it does not have to worry about accessing memory
 5517: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?
 5617:21 <abubakarsadiq> +1 willcl-ark
 5717:21 <BrinkingChancell> ahh, I see. that makes sense
 5817: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.
 5917:23 <abubakarsadiq> 3. The first attempt #11775 split the CValidationInterface into CValidationInterface and MempoolInterface. What is the distinction between the two interfaces?
 6017: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.
 6117:27 <abubakarsadiq> @maxedw yes, `CValidationInterface` callback notifications are fired from validation events such as block connection.
 6217:27 <abubakarsadiq> whereas `MempoolInterface` callback notifications are fired from mempool events such as adding/removing or transactions in the mempool
 6317:27 <BrinkingChancell> I had a similar understanding to maxedw.
 6417: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?
 6517:28 <abubakarsadiq> The C is the legacy Hungarian naming convention used before
 6617:28 <maxedw> If it was going to be any prefix, I would have thought I for Interface
 6717:28 <maxedw> but it seems quite an old school microsofty thing to do
 6817:28 <maxedw> we used to do it in VB6 back in the day
 6917:30 <abubakarsadiq> see https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c
 7017:30 <abubakarsadiq> there is a disclaimer not to use that anymore
 7117: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`?
 7217:31 <maxedw> the current approach is less code and perhaps a bit simpler?
 7317: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?
 7417: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
 7517: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
 7617:33 <abubakarsadiq> +1 willcl-ark
 7717:33 <maxedw> minimal scope must help with getting it merged
 7817: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
 7917:34 <abubakarsadiq> 5. Can you list the other subscribers to `CValidationInterface`?
 8017:35 <maxedw> It was used in a few places, `mining.cpp` for BlockChecked and also in `NotificationsProxy`, `PeerManager` and `BaseIndex`.
 8117:36 <abubakarsadiq> +1 with `CZMQNotificationInterface`
 8217: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?
 8317:37 <BrinkingChancell> I got BlockConnected, BlockDisconnected, UpdatedBlockTip, BlockChecked, NewPoWValidBlock, submitblock_StateCatcher, TestSubscriber, TestSubscriberNoop, OutpointsUpdater, TransactionsDelta, CZMQNotificationInterface
 8417:37 <BrinkingChancell> but not terribly confident in this answer
 8517:37 <BrinkingChancell> what's the best approach to find all subscribers to an interface?
 8617:37 <abubakarsadiq> BlockConnected, BlockDisconnected, UpdatedBlockTip, BlockChecked, NewPoWValidBlock this are `CValidationInterface` callbacks
 8717:38 <abubakarsadiq> @willcl-ark `CZMQNotificationInterface` is a client of `CValidationInterface` AFAIU
 8817:39 <abubakarsadiq> @BrinkingChancell hint: find all subclasses of `CValidationInterface`
 8917:41 <abubakarsadiq> this brings us to a similar question
 9017:41 <abubakarsadiq> 6. Why is implementing a `CValidationInterface` method equivalent to “subscribing to the event”
 9117: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.
 9217:43 <vmammal> 6. i also found this one challenging to articulate
 9317:45 <abubakarsadiq> All subclasses of `CValidationInterface` are clients . the subclass can implement `CValidationInterface` methods (callbacks)
 9417:45 <abubakarsadiq> Any implemented `CValidationInterface` method from a `CValidationInteface` subclass will be executed every time the method callback is fired using `CMainSignals`.
 9517:45 <abubakarsadiq> Callbacks are fired whenever the corresponding event occurs. check in `src/validation.cpp` and `src/txmempool.cpp`
 9617:47 <willcl-ark> that makes sense
 9717:47 <abubakarsadiq> specifically e.g https://github.com/bitcoin/bitcoin/blob/eca2e430acf50f11da2220f56d13e20073a57c9b/src/txmempool.cpp#L504C16-L504C16
 9817:47 <maxedw> I'm not fully there yet on that one, will have to read the code a bit more
 9917:49 <maxedw> in my mind the subclasses are their own objects so I don't really get how something knows to call them
10017: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?
10117:49 <maxedw> I think it's that register step I'm missing
10217: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
10317:50 <abubakarsadiq> @maxedw I added a point about the subscription in the notes I think
10417:51 <maxedw> thank you I will have a read
10517:52 <abubakarsadiq> `7. BlockConnected` and `NewPoWValidBlock` are different callbacks. Which one is asynchronous and which one is synchronous? How can you tell?
10617:53 <maxedw> `BlockConnected` is asynchronous. I know this because that's what the comment said :p
10717:53 <BrinkingChancell> same!
10817:54 <willcl-ark> as good-a-way to find an answer as any I've heard.
10917:54 <maxedw> the method signatures looked quite similar so I couldn't tell much from that
11017:54 <abubakarsadiq> what about `NewPoWValidBlock`
11117:55 <BrinkingChancell> the `BlockConnected` function has a lambda expression that is passed to an event queuing mechanism
11217:55 <abubakarsadiq> +1 bingo
11317:56 <abubakarsadiq> All asynchronous callbacks have a docstring indicating that they are executed in the background.
11417: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.
11517: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
11617:56 <pablomartin> ah!
11717:56 <abubakarsadiq> `NewPoWValidBlock` is synchronous
11817:57 <BrinkingChancell> `NewPoWValidBlock` is synchronous. There is no mention of callbacks, promises, async/await patterns..
11917:57 <maxedw> the one calls `ENQUEUE_AND_LOG_EVENT` and the other doesn't
12018:00 <willcl-ark> So net_processing::PeerManager is the only one using synchronous version, for better relay performance?
12118:00 <abubakarsadiq> #endmeeting

Meeting 2

12217:00 <abubakarsadiq> #startmeeting
12317:00 <maxedw> hi
12417:00 <pablomartin> hello
12517:00 <stickies-v> hi
12617: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.
12717:00 <abubakarsadiq> We will be discussing the code review questions.
12817:02 <maxedw> happy to be back, think I will be doing more reading and learning on this half
12917:02 <abubakarsadiq> Lets jump right in to the next question
13017:02 <abubakarsadiq> 9. In 4986edb, why are we adding a new callback `MempoolTransactionsRemovedForConnectedBlock` instead of using `BlockConnected`?
13117:03 <abubakarsadiq> https://github.com/bitcoin-core-review-club/bitcoin/commit/4986edb99f8aa73f72e87f3bdc09387c3e516197 commit link
13217:03 <maxedw> Is it because it returns the removed transactions directly and not a block?
13317:03 <abubakarsadiq> yes @maxedw
13417:04 <maxedw> does the block have those too?
13517:04 <pablomartin> the removal of the txs is happening before (on BlockConnected), so you get the size of them also
13617:04 <abubakarsadiq> and also vtx does not have some transaction details like base fee.
13717:04 <abubakarsadiq> It will not be desirable to modify `CBlock` vtx entries to include details we need for fee estimation.
13817:05 <maxedw> understood
13917: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.
14017:07 <abubakarsadiq> after new block is connected probably
14117:08 <abubakarsadiq> it also needs to know when transactions are removed for other reasons apart from `BLOCK`
14217:08 <pablomartin> i see... i thought MempoolTransactionsRemovedForConnectedBlock was triggered after the txs were removed from the mempool... when BlockConnected... no?
14317:08 <abubakarsadiq> it is triggered before removal with the mempool transactions that are going to be removed.
14417:09 <pablomartin> oh right...
14517:09 <pablomartin> makes sense
14617:09 <abubakarsadiq> next question
14717: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?
14817:10 <glozow> hi
14917:11 <maxedw> TransactionRef, FeeAmount, VirtualTransactionSize and TxHeight are the members which I assume are needed for the estimation
15017:12 <maxedw> I don't know if it's appropriate in `mempool_entry.h`
15117: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
15217: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
15317:14 <pablomartin> and src/txmempool.cpp?
15417:15 <abubakarsadiq> but overall I think `kernel/mempool_entry.h` is the right place for now
15517:16 <glozow> Wouldn't you have a circular dependency if you moved them to policy/fees?
15617:16 <aaron> hello
15717:17 <abubakarsadiq> yes @glozow, kernel/mempool_entry.h seems the best place for it
15817:17 <glozow> makes sense to me
15917:17 <abubakarsadiq> 11. Why can’t we use a `std::vector<CTxMempoolEntry>` as a parameter of `MempoolTransactionsRemovedForBlock` callback?
16017:18 <pablomartin> oh sorry, said nonsense... that was the proper mempool.. :$
16117:20 <abubakarsadiq> We only need a few of the fields in `CTxMempoolEntry`, why make a copy of the whole object `CTxMempoolEntry`
16217:21 <maxedw> and it can't be shared because it's fired off on a new thread?
16317: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
16417:22 <abubakarsadiq> Yes, has to be copied I think
16517: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.
16617:25 <abubakarsadiq> 12. How can you get the base fee of a `CTransactionRef`?
16717:27 <vmammal> naively, sum(inputs) minus sum(outputs) but i feel like that's not the answer you're going for
16817:27 <maxedw> I might be way off here but does it have anything to do with the `Workspace` struct?
16917: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.
17017:29 <abubakarsadiq> No helper method for this for `CTransactionRef` thats why we are making copy of the base fee
17117:31 <abubakarsadiq> maxedw no does not have to do with `Workspace` struct.
17217: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?
17317:35 <vmammal> i believe it is necessary
17417:35 <vmammal> since tx are removed for block connected through another mechanism ?
17517:36 <abubakarsadiq> yes, it is.
17617: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.
17717:37 <abubakarsadiq> Any transaction whose removal reason is BLOCK will be removed by the fee estimator from the processBlock call in removeForBlock.
17817:37 <abubakarsadiq> Having removeTx(hash, false); call outside reason != BLOCK is incorrect.
17917:37 <abubakarsadiq> It is supposed to be in (reason != BLOCK) scope.
18017:37 <abubakarsadiq> https://github.com/bitcoin/bitcoin/pull/10199#discussion_r113795130
18117:38 <abubakarsadiq> So the fix is necessary now with this PR.
18217:39 <vmammal> good catch
18317:40 <pablomartin> thanks for the link
18417:40 <abubakarsadiq> 14. Why is the fee estimator not tracking transactions with unconfirmed parents?
18517:40 <aaron> yes thanks for the link
18617:41 <abubakarsadiq> aaron also see https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1311941008
18717:41 <aaron> will do thank you!
18817: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
18917:42 <abubakarsadiq> + vmammal the fee estimator is not package aware
19017:42 <glozow> followup question: then should we ignore transactions with in-block children?
19117: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
19217:44 <abubakarsadiq> good question glozow
19317:45 <vmammal> an unconfirmed tx with in-block children? im stumped
19417:46 <glozow> Ah no. I mean when a transaction confirms, and has a child in the block
19517:46 <pablomartin> if the fee estimator is not package aware... where the ancestor fee of package evaluation takes place?
19617:46 <abubakarsadiq> no I think the question is asking should we track confirmed transactions in block that have children in that same block
19717:47 <glozow> here: https://github.com/bitcoin/bitcoin/blob/7386da7a0b08cd2df8ba88dae1fab9d36424b15c/src/node/miner.cpp#L302
19817: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
19917: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
20017:49 <abubakarsadiq> But right now the fee estimator does that
20117:52 <abubakarsadiq> vmammal some might have children but are not confirmed because of the child fee though
20217: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
20317:54 <vmammal> agreed
20417:55 <abubakarsadiq> there is a PR that is attempting to do that with discussions going on
20517:55 <abubakarsadiq> https://github.com/bitcoin/bitcoin/pull/25380
20617: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?
20717:57 <vmammal> what other option do you have than to copy?
20817:57 <abubakarsadiq> this fields are passed to fix silent merge conflict with #25380, but arent used in this PR anyway.
20917:57 <abubakarsadiq> I think its best to leave them out untill when needed
21017:58 <abubakarsadiq> nope vmammal
21117:59 <abubakarsadiq> Two minutes
21217:59 <abubakarsadiq> last question
21317: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`?
21418:00 <abubakarsadiq> #endmeeting