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.
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.
Why is it beneficial to remove CTxMempool dependency on CBlockPolicyEstimator?
Are there any benefits of the CBlockPolicyEstimator being a member of CTxMempool? Are there downsides to removing it?
The first attempt #11775 split the CValidationInterface into CValidationInterface and MempoolInterface. What is the distinction between the two interfaces?
What do you think is better or worse about the approach taken in this approach, versus the one taken in #11775 split the CValidationInterface?
Can you list the other subscribers to CValidationInterface?
Why is implementing a CValidationInterface method equivalent to “subscribing to the event”?
BlockConnected and NewPoWValidBlock are different callbacks. Which one is asynchronous and which one is synchronous? How can you tell?
Code Review
In 4986edb, why are we adding a new callback MempoolTransactionsRemovedForConnectedBlock instead of using BlockConnected?
In 1d116df, is kernel/mempool_entry.h the right place for NewMempoolTransactionInfo? What members are included in this struct, and why are they necessary?
Why can’t we use a std::vector<CTxMempoolEntry> as a parameter of MempoolTransactionsRemovedForBlock callback?
How can you get the base fee of a CTransactionRef?
In ab4e250, is moving removeTx call to reason != BLOCK scope necessary? Is it fixing a bug?
Why is the fee estimator not tracking transactions with unconfirmed parents?
In 79bcc5c, we pass a copy of transaction parents, nSizeWithAncestors, and nModFeesWithAncestors to NewMempoolTransactionInfo. Is there a better approach to get all that information?
In 79bcc5c, why are the block transactions and their ancestors not removed from the mempool in the first loop of removeForBlock?
<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.
<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.
<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
<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
<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
<abubakarsadiq> @maxedw in #28368 the call is processed in `CSchedular` thread, all the info the fee estimation need are passed in the callback parameter
<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?
<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.
<abubakarsadiq> 3. The first attempt #11775 split the CValidationInterface into CValidationInterface and MempoolInterface. What is the distinction between the two interfaces?
<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.
<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?
<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`?
<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?
<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
<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.
<abubakarsadiq> Any implemented `CValidationInterface` method from a `CValidationInteface` subclass will be executed every time the method callback is fired using `CMainSignals`.
<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?
<abubakarsadiq> Another thing to note is that The callbacks can either be executed synchronously or asynchronously depending on the callback whenever they are fired
<abubakarsadiq> `7. BlockConnected` and `NewPoWValidBlock` are different callbacks. Which one is asynchronous and which one is synchronous? How can you tell?
<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.
<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.
<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.
<pablomartin> i see... i thought MempoolTransactionsRemovedForConnectedBlock was triggered after the txs were removed from the mempool... when BlockConnected... no?
<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
<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
<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.
<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.
<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.
<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
<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
<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
<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