Fee Estimator updates from Validation Interface/CScheduler thread#28368
pull
ismaelsadeeq
wants to merge
7
commits into
bitcoin:master
from
ismaelsadeeq:08-2023-fee-estimator-updates-from-validation-interface-signal
changing
20
files
+307−123
This Pr will enable fee estimator to listen to ValidationInterface notifications to process new transactions added and removed from the mempool.
This PR includes the following changes:
Added a new callback to the Validation Interface MempoolTransactionsRemovedForConnectedBlock, which notifies listeners about the transactions that have been removed due to a new block being connected, along with the height at which the transactions were removed.
Modified the TransactionAddedToMempool callback parameter to include additional information about the transaction needed for fee estimation.
Updated CBlockPolicyEstimator to process transactions using CTransactionRef instead of CTxMempoolEntry.
Implemented the CValidationInterface interface in CBlockPolicyEstimater and overridden the TransactionAddedToMempool, TransactionRemovedFromMempool, and MempoolTransactionsRemovedForConnectedBlock methods to receive updates from their notifications.
Prior to this PR, the fee estimator updates from the mempool, i.e whenever a new block is connected all transactions in the block that are in our mempool are going to be removed using the removeForBlock function in txmempool.cpp.
This removal triggered updates to the fee estimator. As a result, the fee estimator would block mempool’s cs until it finished updating every time a new block was connected.
Instead of being blocked only on mempool tx removal, we were blocking on both tx removal and fee estimator updating.
If we want to further improve fee estimation, or add heavy-calulation steps to it, it is currently not viable as we would be slowing down block relay in the process
This PR is smaller in terms of the changes made compared to #11775, as it focuses solely on enabling fee estimator updates from the validationInterface/cscheduler thread notifications.
I have not split the validation interface because, as I understand it, the rationale behind the split in #11775 was to have MempoolInterface signals come from the mempool and CValidationInterface events come from validation. I believe this separation can be achieved in a separate refactoring PR when the need arises.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#28960 (kernel: Remove dependency on CScheduler by TheCharlatan)
#28886 (refactor: Replace sets of txiter with CTxMemPoolEntryRefs by TheCharlatan)
#28830 ([refactor] Check CTxMemPool options in ctor by TheCharlatan)
#28690 (build: Introduce internal kernel library by TheCharlatan)
#28687 ([POC][WIP] C++20 std::views::reverse by stickies-v)
#28335 (RFC: Remove boost usage from kernel api / headers by TheCharlatan)
#27277 (Move log messages: tx enqueue to mempool, allocation to blockstorage by Sjors)
#25038 (policy: nVersion=3 and Package RBF by glozow)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
ismaelsadeeq marked this as ready for review
on Aug 30, 2023
ismaelsadeeq marked this as a draft
on Aug 30, 2023
willcl-ark
commented at 10:52 am on August 30, 2023:
member
Nice work!
So if correctly implemented this should speed up block relay, and enable (future) fee estimator changes to be added without blocking critical codepaths.
It would be nice to try and benchmark this somehow, but I’m unsure immediately what the best way to do this would be…
@glozow@TheCharlatan@instagibbs would be curious to know what y’all think of the approach here.
instagibbs
commented at 2:54 pm on August 30, 2023:
member
I haven’t spent much/any time on the interfaces themselves, will defer to others
I will also change this in a separate commit to use static cast with CPP std
ismaelsadeeq
commented at 10:36 am on September 1, 2023:
Fixed thanks.
in
src/txmempool.cpp:614
in
a7250f6597outdated
610@@ -618,33 +611,25 @@ void CTxMemPool::removeConflicts(const CTransaction &tx)
611 }
612613 /**
614- * Called when a block is connected. Removes from mempool and updates the miner fee estimator.
615+ * Called when a block is connected. Removes from mempool` .
0 * Called when a block is connected. Removes from mempool.
ismaelsadeeq
commented at 10:36 am on September 1, 2023:
Updated thank you.
vincenzopalazzo
commented at 3:26 pm on August 30, 2023:
none
I did a first pass on this but I should take a look in a more deep way
glozow added the label
TX fees and policy
on Aug 31, 2023
glozow
commented at 8:36 am on August 31, 2023:
member
Concept ACK! Background fee estimator would be really nice (fwiw it seems like this was always the plan: #10199 (review) and #11775 had a lot of fans). Updating asynchronously instead of blocking removeForBlock / ConnectTip should make things faster - I personally don’t need a bench to be convinced though it’d be nice to see. I don’t think CTxMemPool should know anything about there being a fee estimator; the dependency should be the other way around. I agree that if we want to pursue having multiple fee estimator approaches (e.g. #27995) it would be best for them to be in the background so we don’t need to worry about the performance impact on every mempool operation. I can also imagine using this to test fee estimator PRs, as it’d be easy to create 2 fee estimators that use the exact same mempool data and compare their results.
I haven’t reviewed the new interface closely yet but will do as soon as I can.
darosior
commented at 12:56 pm on August 31, 2023:
member
Concept ACK. Not sure i’ll be able to review anytime soon though.
Don’t forget to remove policy/fees.h from the includes
ismaelsadeeq
commented at 10:37 am on September 1, 2023:
I remove the include thank you.
in
src/txmempool.cpp:618
in
5896c742caoutdated
633@@ -634,17 +634,20 @@ void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigne
634 }
635 // Before the txs in the new block have been removed from the mempool, update policy estimates
636 if (minerPolicyEstimator) {minerPolicyEstimator->processBlock(nBlockHeight, entries);}
637+ std::vector<CTransactionRef> txs_removed_for_block;
c343bb37ebe15219ac840de2deef1111a5d9be31
not a fan of this logic being repeated in 2 places, and imo it should live in the fee estimator instead of in validation
Perhaps instead add these bools to the NewMempoolTransactionInfo struct:
And the fee estimator decides on validForFeeEstimation based on these variables.
ismaelsadeeq
commented at 10:39 am on September 1, 2023:
Edit: I updated and now the fee estimator decides.
Thank you
in
src/policy/fees.cpp:600
in
bbfe01fa9foutdated
585- // affect the estimate. We'll potentially double count transactions in 1-block reorgs.
586- // Ignore txs if BlockPolicyEstimator is not in sync with ActiveChain().Tip().
587- // It will be synced next time a block is processed.
588- return;
589- }
590-
I think its worth to be in its own PR, its not part of this.
I removed it from this PR.
in
src/txmempool.cpp:493
in
4f22c0da09outdated
489 // in transactions included in blocks can subscribe to the BlockConnected
490 // notification.
491 GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx(), reason, mempool_sequence);
492+ if (minerPolicyEstimator) {
493+ minerPolicyEstimator->removeTx(hash, false);
494+ }
Ok, I think this should be its own commit with this explanation as it is not relevant to refactoring processBlock
glozow
commented at 5:13 pm on August 31, 2023:
member
Did a first pass
ismaelsadeeq force-pushed
on Sep 1, 2023
ismaelsadeeq marked this as ready for review
on Sep 1, 2023
ismaelsadeeq force-pushed
on Sep 1, 2023
DrahtBot added the label
CI failed
on Sep 1, 2023
ismaelsadeeq force-pushed
on Sep 1, 2023
DrahtBot removed the label
CI failed
on Sep 1, 2023
in
src/validationinterface.h:140
in
db4849c354outdated
135+ * as a result of new block being connected.
136+ * MempoolBlockConnect will be fired before BlockConnected.
137+ *
138+ * Called on a background thread.
139+ */
140+ virtual void MempoolBlockConnect(const std::vector<CTransactionRef>& txs_removed_for_block, unsigned int nBlockHeight) {}
TheCharlatan
commented at 8:42 am on September 3, 2023:
Nit: The notification name is a bit non-descript and does not fit the existing naming pattern. How about “MempoolTransactionsRemovedForConnectedBlock”?
ismaelsadeeq
commented at 5:36 pm on September 3, 2023:
TheCharlatan
commented at 8:54 am on September 3, 2023:
This is always false, since node.fee_estimator is reset a couple of lines further up. I’d move this to the line where node.fee_estimator->Flush() is called.
ismaelsadeeq
commented at 5:36 pm on September 3, 2023:
Good catch.
I moved it below the Flush
in
src/validationinterface.h:10
in
a34aaf88c1outdated
So I find the iterator of the tx from mapMemPoolTxs and capture all the values I need (tx fee rate and entry height) before removing the tx.
Before this pull request, we could get all those details from the tx because it’s in CTxMempoolEntry format.
TheCharlatan
commented at 5:53 pm on September 3, 2023:
Thanks for the explainer :)
Feel free to resolve.
DrahtBot added the label
CI failed
on Sep 3, 2023
ismaelsadeeq force-pushed
on Sep 3, 2023
TheCharlatan
commented at 7:30 pm on September 3, 2023:
contributor
Approach ACK
Will give this another pass soon.
DrahtBot removed the label
CI failed
on Sep 4, 2023
ismaelsadeeq force-pushed
on Sep 7, 2023
ismaelsadeeq force-pushed
on Sep 7, 2023
DrahtBot added the label
CI failed
on Sep 7, 2023
ismaelsadeeq renamed this:
Fee Estimator updates from Validation Interface/Cschedular thread
Fee Estimator updates from Validation Interface/CSchedular thread
on Sep 8, 2023
DrahtBot renamed this:
Fee Estimator updates from Validation Interface/CSchedular thread
Fee Estimator updates from Validation Interface/CScheduler thread
on Sep 10, 2023
DrahtBot removed the label
CI failed
on Sep 10, 2023
ismaelsadeeq force-pushed
on Sep 13, 2023
ismaelsadeeq
commented at 1:08 pm on September 13, 2023:
member
Force pushed to 55beb4f39bb029e19b333dea205e4b035d01bd12
Modified MempoolTransactionsRemovedForConnectedBlock callback parameter to vector of NewMempoolTransactionInfo instead of CTransactionRef to resolve conflict with #25380.
See comment
Hence add CTransactionRef back to NewMempoolTransactionInfo
DrahtBot added the label
Needs rebase
on Sep 13, 2023
ismaelsadeeq force-pushed
on Sep 13, 2023
ismaelsadeeq force-pushed
on Sep 14, 2023
DrahtBot removed the label
Needs rebase
on Sep 14, 2023
DrahtBot added the label
CI failed
on Sep 19, 2023
DrahtBot removed the label
CI failed
on Sep 20, 2023
DrahtBot added the label
Needs rebase
on Oct 2, 2023
ismaelsadeeq force-pushed
on Oct 5, 2023
DrahtBot removed the label
Needs rebase
on Oct 5, 2023
ismaelsadeeq force-pushed
on Oct 13, 2023
ismaelsadeeq force-pushed
on Oct 13, 2023
in
src/kernel/mempool_entry.h:191
in
2d4b66b3b9outdated
186 };
187188+struct NewMempoolTransactionInfo {
189+ CTransactionRef m_tx;
190+ std::vector<CTransactionRef> m_parents;
191+ //! The fee the added transaction paid
I should test it but to me looks good, I had a similar code in my branch :)
ismaelsadeeq force-pushed
on Oct 14, 2023
in
src/test/fuzz/package_eval.cpp:23
in
79bcc5ca06outdated
19@@ -20,6 +20,8 @@
2021 using node::NodeContext;
2223+struct NewMempoolTransactionInfo;
TheCharlatan
commented at 12:58 pm on October 26, 2023:
In commit 1d116df4c0e021c4c810450e3e5358f34d72940b:
Following up on #28368 (review), in implementation/.cpp files foward-declarations don’t make much sense. Since it is already transitively included through test/util/txmempool.h this can just be removed again. Same for the other fuzz test.
ismaelsadeeq
commented at 11:36 am on October 30, 2023:
Thank you for you review, removed the forward declaration.
TheCharlatan
commented at 5:45 pm on October 29, 2023:
Nit: I think this would be nicer with designated initializers instead of setting all the values after initialization. Also might make things more readable in general when initializing NewMempoolTransactionInfo.
ismaelsadeeq
commented at 11:38 am on October 30, 2023:
Fixed, thank you
in
src/kernel/mempool_entry.h:170
in
79bcc5ca06outdated
TheCharlatan
commented at 7:24 am on October 30, 2023:
In commit 79bcc5ca0679daf6e57fc4d7ce2244262a7cfd13 the last paragraph of its commit message seems like it misses part of a sentence.
ismaelsadeeq
commented at 11:39 am on October 30, 2023:
Updated the commit message
TheCharlatan approved
TheCharlatan
commented at 7:24 am on October 30, 2023:
contributor
ACK79bcc5ca0679daf6e57fc4d7ce2244262a7cfd13
DrahtBot requested review from darosior
on Oct 30, 2023
DrahtBot requested review from glozow
on Oct 30, 2023
DrahtBot requested review from vincenzopalazzo
on Oct 30, 2023
DrahtBot removed review request from vincenzopalazzo
on Oct 30, 2023
DrahtBot requested review from vincenzopalazzo
on Oct 30, 2023
ismaelsadeeq force-pushed
on Oct 30, 2023
in
src/validationinterface.h:146
in
59db4d1739outdated
142 /**
143 * Notifies listeners of a block being connected.
144- * Provides a vector of transactions evicted from the mempool as a result.
145+ * Provides the block that was connected.
146+ * The block contains a vector of transactions from the new block,
147+ * some of the transactions in the vector may not be in our mempool.
nit 4986edb99f8aa73f72e87f3bdc09387c3e516197
I don’t think these comments are necessary since they’re just facts about block transactions that might not be relevant to the user
ismaelsadeeq
commented at 7:17 pm on November 3, 2023:
Maybe create a NewMempoolTransactionInfo constructor that takes a CTxMemPoolEntry as a param instead?
ismaelsadeeq
commented at 7:18 pm on November 3, 2023:
Done
in
src/validationinterface.h:141
in
59db4d1739outdated
136+ * as a result of new block being connected.
137+ * MempoolTransactionsRemovedForConnectedBlock will be fired before BlockConnected.
138+ *
139+ * Called on a background thread.
140+ */
141+ virtual void MempoolTransactionsRemovedForConnectedBlock(const std::vector<NewMempoolTransactionInfo>& txs_removed_for_block, unsigned int nBlockHeight) {}
It’s quite confusing to store info about a transaction that isn’t new and isn’t in the mempool in a struct called NewMempoolTransactionInfo. It also has fields that don’t apply to a removed transaction - we aren’t able to populate any of the validforfeeestimate bools. Why not make separate structs for newly added transactions and removed transactions?
ismaelsadeeq
commented at 7:22 pm on November 3, 2023:
Thank you @glozow
Now using two structs RemovedMempoolTransactionInfo for this callback and NewMempoolTransactionInfo for `TransactionAddedToMempool callback.
Since they have similar fields, I created a struct TransactionInfo with all the similar fields so that they can both have a member object of type TransactionInfo.
pablomartin4btc
commented at 6:55 pm on October 30, 2023:
member
Concept ACK
I agree with @glozow regarding the correct objs dependency direction (fee estimator-> CTxMemPool) and that this would be very useful in order to test fee estimator PRs.
I’m trying to finish a code review and perform some testing before the Review club in the next 2 days. In the meantime, could the label be added to this PR?
in
src/test/fuzz/policy_estimator.cpp:83
in
59db4d1739outdated
TheCharlatan
commented at 9:52 pm on October 30, 2023:
Nit: There is trailing whitespace here in commit 5ab08d23b2e88c1a8fda1535114e04b695c3483b. Also, why remove the const qualifier? Can’t all the tx_info be made const?
ismaelsadeeq
commented at 1:35 pm on October 31, 2023:
Thank you, Will update if there is need to re touch.
TheCharlatan approved
TheCharlatan
commented at 9:59 pm on October 30, 2023:
contributor
Re-ACK59db4d1739de1a008e6f34d0f03c3e8e8da60a93
DrahtBot removed review request from vincenzopalazzo
on Oct 30, 2023
DrahtBot requested review from vincenzopalazzo
on Oct 30, 2023
DrahtBot requested review from pablomartin4btc
on Oct 30, 2023
DrahtBot removed review request from vincenzopalazzo
on Oct 30, 2023
DrahtBot requested review from vincenzopalazzo
on Oct 30, 2023
DrahtBot removed review request from vincenzopalazzo
on Oct 31, 2023
DrahtBot requested review from vincenzopalazzo
on Oct 31, 2023
glozow added the label
Review club
on Nov 1, 2023
hernanmarino
commented at 4:14 pm on November 1, 2023:
contributor
Approach ACK . I’m really curious about benchmarks but I don’t think they are really necessary to make a decision about this. I also believe this PR will have a higher impact in the future, should any changes to fee estimation be necessary. I ’ll try to take a deeper look and code review in a couple of days, if still unmerged.
DrahtBot removed review request from vincenzopalazzo
on Nov 1, 2023
DrahtBot requested review from vincenzopalazzo
on Nov 1, 2023
TheCharlatan
commented at 8:07 pm on November 1, 2023:
contributor
Seems like with the changes here, the policy/fees can be removed the kernel library. If you want to, you can integrate this one-line commit, otherwise I’ll make a tiny follow up.
DrahtBot removed review request from vincenzopalazzo
on Nov 1, 2023
DrahtBot requested review from vincenzopalazzo
on Nov 1, 2023
in
src/validationinterface.cpp:222
in
4986edb99foutdated
nit: This log should probably say “num txs removed=” instead of “txs=”.
ismaelsadeeq
commented at 7:22 pm on November 3, 2023:
Fixed
in
src/kernel/mempool_entry.h:203
in
bb22a5148aoutdated
188+ *
189+ * It is the primary metric by which the mining algorithm selects
190+ * transactions.
191+ */
192+ int64_t m_virtual_transaction_size;
193+ unsigned int txHeight;
what is the purpose of adding m_fee_per_k here? it adds 64b to every TxStatsInfo we store (note that there is one for every unconfirmed transaction). You can get the same information from the NewMempoolTransactionInfo in processBlock.
ismaelsadeeq
commented at 7:23 pm on November 3, 2023:
9b0843042ab5c30e6ef4753d590c69c4811d9a70 seems to be doing multiple things: (1) making CBlockPolicyEstimator react to validationinterface and (2) delegating the validForFeeEstimation from validation to the CBlockPolicyEstimator by providing these bools in the TransactionAddedToMempool signal (but this isn’t fully done until the next commit which cleans things up).
These should be separate commits, e.g. (2) and then (1).
ismaelsadeeq
commented at 7:32 pm on November 3, 2023:
Thank you
This is fixed.
a469d937dbbadedeb58e0d5d1c44ed7881a7ebb9 introduces NewMempoolTransactionInfo with all the necessary fields and updates TransactionAddedToMempool callback parameter.
ac82f58e45421aa5aec1d3e6e992d1c369472a10 updates the fee estimator from CValidationInterface notifications and manage delegating validForFeeEstimation to CBlockPolicyEstimator::processTransaction together the cleanups.
This is quite verbose and repeated in several places. Why not create a dedicated constructor?
ismaelsadeeq
commented at 7:32 pm on November 3, 2023:
Done
in
src/validationinterface.h:140
in
4986edb99foutdated
135+ * as a result of new block being connected.
136+ * MempoolTransactionsRemovedForConnectedBlock will be fired before BlockConnected.
137+ *
138+ * Called on a background thread.
139+ */
140+ virtual void MempoolTransactionsRemovedForConnectedBlock(const std::vector<CTransactionRef>& txs_removed_for_block, unsigned int nBlockHeight) {}
These fields added in 53bbc791d47a51a7c2bbefb32835fd4001b68890 aren’t necessary as you’re not using them anywhere. At quick glance I don’t think we should ever tell the fee estimator about modified fees.
ismaelsadeeq
commented at 4:13 pm on November 2, 2023:
these fields were added to solve silent merge conflict #25380 (comment) , but you are right they should be added when needed.
Edit: removed from this PR, they can be added when needed
glozow
commented at 3:44 pm on November 2, 2023:
member
Approach ACK, but some of the changes seem to be interleaved across multiple commits / lumped together in a single commit. For example, processBlock’s txs_removed_for_block param changes type 3 times in this PR. In an intermediate step where you don’t have fee information for a tx, you add a TxStatsInfo::CAmount to store it, but then don’t delete it at the end when we don’t need it anymore.
I’d suggest making this PR 1 change per commit, and 1 commit per change:
move removeTx into the reason != BLOCK condition
remove C-style casts
Introduce the RemovedMempoolTransactionInfo struct and MempoolTransactionsRemovedForBlock callback
Update the fee estimator interface to use RemovedMempoolEntry
Introduce a NewMempoolTransactionInfo struct and re-delegate calculation of validForFeeEstimation from validation to fee estimator (including the cleanups)
Make the fee estimator a client of CValidationInterface
DrahtBot removed the label
CI failed
on Nov 2, 2023
ismaelsadeeq force-pushed
on Nov 3, 2023
ismaelsadeeq
commented at 7:44 pm on November 3, 2023:
member
Thanks for your review @glozow and @TheCharlatan
Forced pushed from 6d676c69195dc9032c4558987fb88f4c4b71092c to 09436b21e84cc6cd979fe4942ba1c415c4bc91beCompare changes
Rework the commits in the order of this review comment which is more cleaner and less verbose.
you’re changing how the fuzz inputs are processed here - maybe have m_has_no_mempool_parents = ConsumeBool() and make the others = false/false/true?
ismaelsadeeq
commented at 10:43 am on November 8, 2023:
Thanks, its now as you suggested
in
src/test/policyestimator_tests.cpp:167
in
ac82f58e45outdated
162+ LOCK2(cs_main, mpool.cs);
163+ mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now<NodeSeconds>()).Height(blocknum).FromTx(tx));
164+ // Since TransactionAddedToMempool callbacks are generated in ATMP,
165+ // not addUnchecked, we cheat and create one manually here
166+ const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx));
167+ const NewMempoolTransactionInfo tx_info = NewMempoolTransactionInfo(MakeTransactionRef(tx),
ismaelsadeeq
commented at 10:43 am on November 8, 2023:
Done
glozow
commented at 4:46 pm on November 7, 2023:
member
Maybe add a SyncWithValidationInterfaceQueue to the start of the fee estimation RPCs? To avoid spurious failures in feature_fee_estimation.py. Also, I can imagine scripts that grab fee estimates once per block as soon as the block arrives - should probably make sure they’re fresh.
ismaelsadeeq force-pushed
on Nov 8, 2023
ismaelsadeeq force-pushed
on Nov 8, 2023
ismaelsadeeq
commented at 3:39 pm on November 8, 2023:
member
Force pushed from 09436b21e84cc6cd979fe4942ba1c415c4bc91be to 969f5ec4a8
Added a commit that SyncWithValidationInterfaceQueue at the start of fee estimation RPC’s
The relationship between RemovedMempoolTransactionInfo, NewMempoolTransactionInfo, and TransactionInfo has been updated to use composition. TransactionInfo object is now a member of RemovedMempoolTransactionInfo and NewMempoolTransactionInfo.
DrahtBot added the label
Needs rebase
on Nov 13, 2023
ismaelsadeeq force-pushed
on Nov 13, 2023
DrahtBot removed the label
Needs rebase
on Nov 13, 2023
in
src/kernel/mempool_entry.h:234
in
a9d48d9a74outdated
TheCharlatan
commented at 2:46 pm on November 19, 2023:
Nit: Remove the unused argument names in the interface functions?
ismaelsadeeq
commented at 1:00 pm on November 20, 2023:
To override virtual functions from the CValidationInterface class, they must have same number of parameters and types to the corresponding virtual function in `CValidationInterface.
TheCharlatan
commented at 1:10 pm on November 20, 2023:
Yes, I was only suggesting to remove the names, e.g. change uint64_t mempool_sequence to uint64_t /*unused*/.
ismaelsadeeq
commented at 3:45 pm on November 20, 2023:
thanks I understand, updated
ismaelsadeeq force-pushed
on Nov 20, 2023
ismaelsadeeq force-pushed
on Nov 20, 2023
ismaelsadeeq
commented at 4:54 pm on November 20, 2023:
member
If the removal reason of a transaction is BLOCK, then the `removeTx`
boolean argument should be true.
Before this PR, `CBlockPolicyEstimator` have to complete updating the fee stats
before the mempool clears that's why having removeTx call outside reason!= `BLOCK`
in `addUnchecked` was not a bug.
But in a case where the `CBlockPolicyEstimator` update is asynchronous, the mempool might
clear before we update the `CBlockPolicyEstimator` fee stats.
Transactions that are removed for `BLOCK` reasons will also be incorrectly removed from
`CBlockPolicyEstimator` stats as failures.
a0e3eb7549
tx fees, policy: cast with static_cast instead of C-Style cast0889e07987
CValidationInterface, mempool: add new callback to `CValidationInterface`
This commit adds a new callback `MempoolTransactionsRemovedForBlock` which notify
its listeners of the transactions that are removed from the mempool because a new
block is connected, along with the block height the transactions were removed.
The transactions are in `RemovedMempoolTransactionInfo` format.
`CTransactionRef`, base fee, virtual size, and height which the transaction was added
to the mempool are all members of the struct called `RemovedMempoolTransactionInfo`.
A struct `NewMempoolTransactionInfo`, which has fields similar to `RemovedMempoolTransactionInfo`,
will be added in a later commit, create a struct `TransactionInfo` with all similar fields.
They can both have a member with type `TransactionInfo`.
Update `processBlock` parameter to reference to a vector of `RemovedMempoolTransactionInfo`.
91532bd382
CValidationInterface: modify the parameter of `TransactionAddedToMempool`
Create a new struct `NewMempoolTransactionInfo` that will be used as the new parameter of
`TransactionAddedToMempool` callback.
dff5ad3b99
tx fees, policy: CBlockPolicyEstimator update from `CValidationInterface` notifications
`CBlockPolicyEstimator` will implement `CValidationInterface` and
subscribe to its notification to process transactions added and removed
from the mempool.
Re-delegate calculation of `validForFeeEstimation` from validation to fee estimator.
Also clean up the validForFeeEstimation arg thats no longer needed in `CTxMempool`.
Co-authored-by: Matt Corallo <git@bluematt.me>
714523918b
rpc: `SyncWithValidationInterfaceQueue` on fee estimation RPC's
This ensures that the most recent fee estimation data is used for the
fee estimation with `estimateSmartfee` and `estimaterawfee` RPC's.
91504cbe0d
ismaelsadeeq force-pushed
on Nov 22, 2023
ismaelsadeeq
commented at 10:53 am on November 22, 2023:
member
willcl-ark
commented at 9:53 am on November 24, 2023:
In dff5ad3b9944cbb56126ba37a8da180d1327ba39
Worth adding a doxygen comment here? Even something simple like:
0/**
1 * Holds information about new transactions added to the mempool.
2 * In addition to TransactionInfo includes limit bypass, package, chain and parent info.
3 */
in
src/kernel/mempool_entry.h:214
in
dff5ad3b99outdated
willcl-ark
commented at 10:03 am on November 24, 2023:
In dff5ad3b9944cbb56126ba37a8da180d1327ba39
You’ve called this m_from_disconnected_block, but described it as whether the tx was added without enforcing mempool fee limits. This seems incorrect or confusing.
SubmitPackage() is calling this using args.m_bypass_limits so i think the comment is correct, and the variable should be renamed?
in
src/init.cpp:288
in
714523918boutdated
284@@ -285,8 +285,12 @@ void Shutdown(NodeContext& node)
285 DumpMempool(*node.mempool, MempoolPath(*node.args));
286 }
287288- // Drop transactions we were still watching, and record fee estimations.
289- if (node.fee_estimator) node.fee_estimator->Flush();
290+ // Drop transactions we were still watching, record fee estimations and Unregister
willcl-ark
commented at 10:08 am on November 24, 2023:
micro nit, if you end up touching 714523918ba2b853fc69bee6b04a33ba0c828bf5 again:
0 // Drop transactions we were still watching, record fee estimations and unregister
in
src/test/fuzz/policy_estimator.cpp:51
in
91504cbe0d
willcl-ark
commented at 12:31 pm on November 24, 2023:
In 714523918ba2b853fc69bee6b04a33ba0c828bf5
Would we want to use fuzzed_data_provider.ConsumeBool() for m_submitted_in_package?
willcl-ark approved
willcl-ark
commented at 12:56 pm on November 24, 2023:
member
ACK91504cbe0de2b74ef1aa2709761aaf0597ec66a2
Glad to see the fee estimator being decoupled in this way and keen to see more development of additional estimators in the future e.g. as per #27995
I gave the code a review and it seems well thought-out to me at this stage; left a few nits which can be addressed in the case that you re-touch things.
DrahtBot removed review request from hernanmarino
on Nov 24, 2023
DrahtBot removed review request from vincenzopalazzo
on Nov 24, 2023
DrahtBot requested review from vincenzopalazzo
on Nov 24, 2023
DrahtBot requested review from hernanmarino
on Nov 24, 2023
DrahtBot removed review request from vincenzopalazzo
on Nov 24, 2023
DrahtBot removed review request from hernanmarino
on Nov 24, 2023
DrahtBot requested review from hernanmarino
on Nov 24, 2023
DrahtBot requested review from vincenzopalazzo
on Nov 24, 2023
ismaelsadeeq
commented at 7:30 pm on November 24, 2023:
member
Thanks for your review @willcl-ark will address nits if there is need to retouch.
DrahtBot removed review request from hernanmarino
on Nov 24, 2023
DrahtBot removed review request from vincenzopalazzo
on Nov 24, 2023
DrahtBot requested review from vincenzopalazzo
on Nov 24, 2023
DrahtBot requested review from hernanmarino
on Nov 24, 2023
achow101
commented at 7:57 pm on December 1, 2023:
member
ACK91504cbe0de2b74ef1aa2709761aaf0597ec66a2
DrahtBot removed review request from vincenzopalazzo
on Dec 1, 2023
DrahtBot removed review request from hernanmarino
on Dec 1, 2023
DrahtBot requested review from hernanmarino
on Dec 1, 2023
DrahtBot requested review from vincenzopalazzo
on Dec 1, 2023
achow101 merged this
on Dec 1, 2023
achow101 closed this
on Dec 1, 2023
pablomartin4btc
commented at 2:15 am on December 5, 2023:
member
post merge tACK91504cbe0de2b74ef1aa2709761aaf0597ec66a2
Tested manually using RPC commands estimatesmartfee and estimaterawfee (following cases from /test/functional/feature_fee_estimation.py).
glozow referenced this in commit
65c05db660
on Jan 3, 2024
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-02-25 15:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me