Fee Estimator updates from Validation Interface/CScheduler thread (
tx fees and policy) Nov 1, 2023
The PR branch HEAD was 79bcc5ca0679daf6e57fc4d7ce2244262a7cfd13 at the time of this review club meeting.
is used to estimate what feerate a user should place on their transaction to be confirmed within a targeted number of blocks. RPCs
can be used to obtain a feerate estimate for a given confirmation target. Fee estimation is also heavily used in the
Before this PR,
as one of its members and, whenever a transaction is added or removed from the mempool in
methods, it also updates the fee estimator synchronously.
to update the fee stats when transactions are removed from the mempool after a new block connection.
removeForBlock is part of a series of function calls within
, 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
, the removal of a transaction from the mempool
, the connection of a block to the chain
, and others.
are called subscribers or clients of
CValidationInterface. They can implement the callbacks they need and must register with
Validation and mempool use
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.
CTxMempool’s’ dependency on
CBlockPolicyEstimator by making
CBlockPolicyEstimator a client of
CValidationInterface, instead of an internal component of
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
MempoolTransactionsRemovedForConnectedBlock which is fired in
The PR also modifies the
TransactionAddedToMempool callback parameter from
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
Did you review the PR?
Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?
Why is it beneficial to remove
CTxMempool dependency on
Are there any benefits of the
CBlockPolicyEstimator being a member of
CTxMempool? Are there downsides to removing it?
The first attempt
#11775 split the
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
Can you list the other subscribers to
Why is implementing a
CValidationInterface method equivalent to “subscribing to the event”?
NewPoWValidBlock are different callbacks. Which one is asynchronous and which one is synchronous? How can you tell?
, why are we adding a new callback
MempoolTransactionsRemovedForConnectedBlock instead of using
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
How can you get the base fee of a
, 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?
, we pass a copy of transaction
NewMempoolTransactionInfo. Is there a better approach to get all that information?
, why are the block transactions and their ancestors not removed from the mempool in the
first loop of
1 17:00 <abubakarsadiq> #startmeeting
2 17:00 <kevkevin> hey guys
4 17:00 <pablomartin> hello
10 17:01 <abubakarsadiq> welcome everyone! Today’s PR is #28368.
12 17:01 <hernanmarino> Hello
13 17:01 <abubakarsadiq> Anyone joining in for the first time?
16 17:02 <abubakarsadiq> Welcome @henmeh84 @aaron
17 17:02 <henmeh84> thank you. nice to be here
18 17:03 <abubakarsadiq> Did everyone get a chance to review the PR? How about a quick y/n from everyone
19 17:03 <aaron> thank you as well, very exited to be here
20 17:03 <willcl-ark> light y for me
21 17: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.
22 17:03 <hernanmarino> y. Light review, code review pending
23 17:04 <kevkevin> I did very shortly
24 17:04 <stickies-v> mostly reviewed the notes/questions
26 17:04 <abubakarsadiq> Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?
27 17:05 <abubakarsadiq> lets jump right in
28 17:05 <hernanmarino> Approach ACK
29 17:05 <maxedw> Concept ACK
30 17:05 <abubakarsadiq> Why is it beneficial to remove CTxMempool dependency on CBlockPolicyEstimator?
31 17:06 <BrinkingChancell> utACK 79bcc5
32 17:06 <maxedw> It seems like when there are changes to the mempool a synchronous update of the PolicyEstimator fires off
33 17: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.
35 17:07 <BrinkingChancell> hi
36 17:08 <abubakarsadiq> yes @maxedw @willcl-ark, in master CTxMemPool updates the CBlockPolicyEstimator synchronously
37 17:08 <pablomartin> willcl-ark +1 also would be very useful in order to test other fee estimators (/PRs)
38 17: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
39 17: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
40 17:10 <abubakarsadiq> Adding other complex fee estimation stuff during updates will be efficient.
41 17: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.
42 17:11 <abubakarsadiq> This brings us to the second question
43 17:11 <abubakarsadiq> 2. Are there any benefits of the `CBlockPolicyEstimator` being a member of `CTxMempool` ? Are there downsides to removing it?
44 17:11 <maxedw> just a general thought on synchronous code being simpler
45 17:12 <maxedw> it's much easier to reason about and to write without error
46 17:12 <BrinkingChancell> agreed that synchronous code is easier to reason about
47 17: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?
48 17:13 <willcl-ark> the estimator does enjoy the transaction metadata the CTxMempoolEntry's have, fee, ancestor info etc.
49 17:13 <glozow> yeah you don't need to make copies of all the info you need
50 17:13 <abubakarsadiq> @maxedw yes its also synchronous on tx removal
51 17:16 <abubakarsadiq> @will-clark thats why we now have NewMempoolTransactionInfo struct to pass copies of all the info the fee estimator needs
52 17: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
53 17: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
54 17:18 <abubakarsadiq> The fee estimator has its own version of the mempool `mapMempoolTxs` it does not have to worry about accessing memory
55 17: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?
56 17:21 <abubakarsadiq> +1 willcl-ark
57 17:21 <BrinkingChancell> ahh, I see. that makes sense
58 17: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.
59 17:23 <abubakarsadiq> 3. The first attempt #11775 split the CValidationInterface into CValidationInterface and MempoolInterface. What is the distinction between the two interfaces?
60 17: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.
61 17:27 <abubakarsadiq> @maxedw yes, `CValidationInterface` callback notifications are fired from validation events such as block connection.
62 17:27 <abubakarsadiq> whereas `MempoolInterface` callback notifications are fired from mempool events such as adding/removing or transactions in the mempool
63 17:27 <BrinkingChancell> I had a similar understanding to maxedw.
64 17: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?
65 17:28 <abubakarsadiq> The C is the legacy Hungarian naming convention used before
66 17:28 <maxedw> If it was going to be any prefix, I would have thought I for Interface
67 17:28 <maxedw> but it seems quite an old school microsofty thing to do
68 17:28 <maxedw> we used to do it in VB6 back in the day
70 17:30 <abubakarsadiq> there is a disclaimer not to use that anymore
71 17: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`?
72 17:31 <maxedw> the current approach is less code and perhaps a bit simpler?
73 17: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?
75 17: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
76 17:33 <abubakarsadiq> +1 willcl-ark
77 17:33 <maxedw> minimal scope must help with getting it merged
78 17: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
79 17:34 <abubakarsadiq> 5. Can you list the other subscribers to `CValidationInterface`?
80 17:35 <maxedw> It was used in a few places, `mining.cpp` for BlockChecked and also in `NotificationsProxy`, `PeerManager` and `BaseIndex`.
81 17:36 <abubakarsadiq> +1 with `CZMQNotificationInterface`
82 17: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?
83 17:37 <BrinkingChancell> I got BlockConnected, BlockDisconnected, UpdatedBlockTip, BlockChecked, NewPoWValidBlock, submitblock_StateCatcher, TestSubscriber, TestSubscriberNoop, OutpointsUpdater, TransactionsDelta, CZMQNotificationInterface
84 17:37 <BrinkingChancell> but not terribly confident in this answer
85 17:37 <BrinkingChancell> what's the best approach to find all subscribers to an interface?
86 17:37 <abubakarsadiq> BlockConnected, BlockDisconnected, UpdatedBlockTip, BlockChecked, NewPoWValidBlock this are `CValidationInterface` callbacks
87 17:38 <abubakarsadiq> @willcl-ark `CZMQNotificationInterface` is a client of `CValidationInterface` AFAIU
88 17:39 <abubakarsadiq> @BrinkingChancell hint: find all subclasses of `CValidationInterface`
89 17:41 <abubakarsadiq> this brings us to a similar question
90 17:41 <abubakarsadiq> 6. Why is implementing a `CValidationInterface` method equivalent to “subscribing to the event”
91 17: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.
92 17:43 <vmammal> 6. i also found this one challenging to articulate
93 17:45 <abubakarsadiq> All subclasses of `CValidationInterface` are clients . the subclass can implement `CValidationInterface` methods (callbacks)
94 17:45 <abubakarsadiq> Any implemented `CValidationInterface` method from a `CValidationInteface` subclass will be executed every time the method callback is fired using `CMainSignals`.
95 17:45 <abubakarsadiq> Callbacks are fired whenever the corresponding event occurs. check in `src/validation.cpp` and `src/txmempool.cpp`
96 17:47 <willcl-ark> that makes sense
98 17:47 <maxedw> I'm not fully there yet on that one, will have to read the code a bit more
99 17:49 <maxedw> in my mind the subclasses are their own objects so I don't really get how something knows to call them
100 17: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?
101 17:49 <maxedw> I think it's that register step I'm missing
102 17: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
103 17:50 <abubakarsadiq> @maxedw I added a point about the subscription in the notes I think
104 17:51 <maxedw> thank you I will have a read
105 17:52 <abubakarsadiq> `7. BlockConnected` and `NewPoWValidBlock` are different callbacks. Which one is asynchronous and which one is synchronous? How can you tell?
106 17:53 <maxedw> `BlockConnected` is asynchronous. I know this because that's what the comment said :p
107 17:53 <BrinkingChancell> same!
108 17:54 <willcl-ark> as good-a-way to find an answer as any I've heard.
109 17:54 <maxedw> the method signatures looked quite similar so I couldn't tell much from that
110 17:54 <abubakarsadiq> what about `NewPoWValidBlock`
111 17:55 <BrinkingChancell> the `BlockConnected` function has a lambda expression that is passed to an event queuing mechanism
112 17:55 <abubakarsadiq> +1 bingo
113 17:56 <abubakarsadiq> All asynchronous callbacks have a docstring indicating that they are executed in the background.
114 17: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.
116 17:56 <pablomartin> ah!
117 17:56 <abubakarsadiq> `NewPoWValidBlock` is synchronous
118 17:57 <BrinkingChancell> `NewPoWValidBlock` is synchronous. There is no mention of callbacks, promises, async/await patterns..
119 17:57 <maxedw> the one calls `ENQUEUE_AND_LOG_EVENT` and the other doesn't
120 18:00 <willcl-ark> So net_processing::PeerManager is the only one using synchronous version, for better relay performance?
121 18:00 <abubakarsadiq> #endmeeting Meeting 2
122 17:00 <abubakarsadiq> #startmeeting
124 17:00 <pablomartin> hello
126 17: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.
127 17:00 <abubakarsadiq> We will be discussing the code review questions.
128 17:02 <maxedw> happy to be back, think I will be doing more reading and learning on this half
129 17:02 <abubakarsadiq> Lets jump right in to the next question
130 17:02 <abubakarsadiq> 9. In 4986edb, why are we adding a new callback `MempoolTransactionsRemovedForConnectedBlock` instead of using `BlockConnected`?
132 17:03 <maxedw> Is it because it returns the removed transactions directly and not a block?
133 17:03 <abubakarsadiq> yes @maxedw
134 17:04 <maxedw> does the block have those too?
135 17:04 <pablomartin> the removal of the txs is happening before (on BlockConnected), so you get the size of them also
136 17:04 <abubakarsadiq> and also vtx does not have some transaction details like base fee.
137 17:04 <abubakarsadiq> It will not be desirable to modify `CBlock` vtx entries to include details we need for fee estimation.
138 17:05 <maxedw> understood
139 17: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.
140 17:07 <abubakarsadiq> after new block is connected probably
141 17:08 <abubakarsadiq> it also needs to know when transactions are removed for other reasons apart from `BLOCK`
142 17:08 <pablomartin> i see... i thought MempoolTransactionsRemovedForConnectedBlock was triggered after the txs were removed from the mempool... when BlockConnected... no?
143 17:08 <abubakarsadiq> it is triggered before removal with the mempool transactions that are going to be removed.
144 17:09 <pablomartin> oh right...
145 17:09 <pablomartin> makes sense
146 17:09 <abubakarsadiq> next question
149 17:11 <maxedw> TransactionRef, FeeAmount, VirtualTransactionSize and TxHeight are the members which I assume are needed for the estimation
150 17:12 <maxedw> I don't know if it's appropriate in `mempool_entry.h`
151 17: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
152 17: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
153 17:14 <pablomartin> and src/txmempool.cpp?
154 17:15 <abubakarsadiq> but overall I think `kernel/mempool_entry.h` is the right place for now
155 17:16 <glozow> Wouldn't you have a circular dependency if you moved them to policy/fees?
157 17:17 <abubakarsadiq> yes @glozow, kernel/mempool_entry.h seems the best place for it
158 17:17 <glozow> makes sense to me
159 17:17 <abubakarsadiq> 11. Why can’t we use a `std::vector<CTxMempoolEntry>` as a parameter of `MempoolTransactionsRemovedForBlock` callback?
160 17:18 <pablomartin> oh sorry, said nonsense... that was the proper mempool.. :$
161 17:20 <abubakarsadiq> We only need a few of the fields in `CTxMempoolEntry`, why make a copy of the whole object `CTxMempoolEntry`
162 17:21 <maxedw> and it can't be shared because it's fired off on a new thread?
163 17: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
164 17:22 <abubakarsadiq> Yes, has to be copied I think
165 17: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.
166 17:25 <abubakarsadiq> 12. How can you get the base fee of a `CTransactionRef`?
167 17:27 <vmammal> naively, sum(inputs) minus sum(outputs) but i feel like that's not the answer you're going for
168 17:27 <maxedw> I might be way off here but does it have anything to do with the `Workspace` struct?
169 17: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.
170 17:29 <abubakarsadiq> No helper method for this for `CTransactionRef` thats why we are making copy of the base fee
171 17:31 <abubakarsadiq> maxedw no does not have to do with `Workspace` struct.
173 17:35 <vmammal> i believe it is necessary
174 17:35 <vmammal> since tx are removed for block connected through another mechanism ?
175 17:36 <abubakarsadiq> yes, it is.
176 17: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.
177 17:37 <abubakarsadiq> Any transaction whose removal reason is BLOCK will be removed by the fee estimator from the processBlock call in removeForBlock.
178 17:37 <abubakarsadiq> Having removeTx(hash, false); call outside reason != BLOCK is incorrect.
179 17:37 <abubakarsadiq> It is supposed to be in (reason != BLOCK) scope.
181 17:38 <abubakarsadiq> So the fix is necessary now with this PR.
182 17:39 <vmammal> good catch
183 17:40 <pablomartin> thanks for the link
184 17:40 <abubakarsadiq> 14. Why is the fee estimator not tracking transactions with unconfirmed parents?
185 17:40 <aaron> yes thanks for the link
187 17:41 <aaron> will do thank you!
188 17: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
189 17:42 <abubakarsadiq> + vmammal the fee estimator is not package aware
190 17:42 <glozow> followup question: then should we ignore transactions with in-block children?
191 17: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
192 17:44 <abubakarsadiq> good question glozow
193 17:45 <vmammal> an unconfirmed tx with in-block children? im stumped
194 17:46 <glozow> Ah no. I mean when a transaction confirms, and has a child in the block
195 17:46 <pablomartin> if the fee estimator is not package aware... where the ancestor fee of package evaluation takes place?
196 17:46 <abubakarsadiq> no I think the question is asking should we track confirmed transactions in block that have children in that same block
198 17: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
199 17: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
200 17:49 <abubakarsadiq> But right now the fee estimator does that
201 17:52 <abubakarsadiq> vmammal some might have children but are not confirmed because of the child fee though
202 17: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
203 17:54 <vmammal> agreed
204 17:55 <abubakarsadiq> there is a PR that is attempting to do that with discussions going on
207 17:57 <vmammal> what other option do you have than to copy?
208 17:57 <abubakarsadiq> this fields are passed to fix silent merge conflict with #25380, but arent used in this PR anyway.
209 17:57 <abubakarsadiq> I think its best to leave them out untill when needed
210 17:58 <abubakarsadiq> nope vmammal
211 17:59 <abubakarsadiq> Two minutes
212 17:59 <abubakarsadiq> last question
214 18:00 <abubakarsadiq> #endmeeting