Fee Estimator updates from Validation Interface/CScheduler thread by ismaelsadeeq · Pull Request #28368 · bitcoin/bitcoin

ismaelsadeeq · 2023-08-30T10:45:22Z

This is an attempt to #11775

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.

Also left out some commits from #11775

Some refactoring which are no longer needed.
Handle reorgs much better in fee estimator.
Track witness hash malleation in fee estimator

I believe they are a separate change that can come in a follow-up after this.

DrahtBot · 2023-08-30T10:45:25Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, willcl-ark, achow101
Concept ACK	darosior, vincenzopalazzo, pablomartin4btc
Approach ACK	hernanmarino, glozow

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.

willcl-ark · 2023-08-30T10:52:57Z

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 · 2023-08-30T14:54:35Z

I haven't spent much/any time on the interfaces themselves, will defer to others

src/init.cpp

src/policy/fees.cpp

vincenzopalazzo

I did a first pass on this but I should take a look in a more deep way

src/txmempool.cpp

glozow · 2023-08-31T08:36:27Z

Concept ACK! Background fee estimator would be really nice (fwiw it seems like this was always the plan: #10199 (comment) 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 · 2023-08-31T12:56:36Z

Concept ACK. Not sure i'll be able to review anytime soon though.

glozow

Did a first pass

src/txmempool.cpp

src/test/fuzz/tx_pool.cpp

src/validation.cpp

src/policy/fees.cpp

src/txmempool.cpp

src/validationinterface.h

src/init.cpp

src/validationinterface.h

src/policy/fees.cpp

ismaelsadeeq · 2023-11-20T16:54:21Z

CI failure is unrelated see #28905

sedited

ACK 7268aaf36bdcc6b466fd16bbf544782571e680bb

src/policy/fees.cpp

hebasto · 2023-11-22T07:48:32Z

CI restarted.

UPD. See #28925

…tion 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.

…ace` 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`.

…ool` Create a new struct `NewMempoolTransactionInfo` that will be used as the new parameter of `TransactionAddedToMempool` callback.

…ace` 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 <[email protected]>

This ensures that the most recent fee estimation data is used for the fee estimation with `estimateSmartfee` and `estimaterawfee` RPC's.

ismaelsadeeq · 2023-11-22T10:53:05Z

Thanks for your review @TheCharlatan
Update from 969f5ec to 91504cb
Compare 969f5ec to 91504cb

Using explicit constructors and list initialization for NewMempoolTransactionInfo and RemovedMempoolTransactionInfo struct
Removed redundant boolean argument in CBlockPolicyEstimator::removeTx
Removed unused argument names in the overridden interface CValidationInterface functions
Update CBlockPolicyEstimator::processTransaction txHeight and feeRate to const's
Rebased after ci: Avoid toolset ambiguity that MSVC can't handle #28905 and ci: Update apt cache #28925 to fix CI failure

sedited

Re-ACK 91504cb

willcl-ark

ACK 91504cb

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.

willcl-ark · 2023-11-23T15:42:37Z

src/validationinterface.h

    virtual void BlockConnected(ChainstateRole role, const std::shared_ptr<const CBlock> &block, const CBlockIndex *pindex) {}
    /**
     * Notifies listeners of a block being disconnected
+     *  Provides the block that was connected.


nit if you retouch bfcd401:

Suggested change

* Provides the block that was connected.

* Provides the block that was disconnected.

willcl-ark · 2023-11-24T09:53:41Z

src/kernel/mempool_entry.h

        : info{entry.GetSharedTx(), entry.GetFee(), entry.GetTxSize(), entry.GetHeight()} {}
 };

+struct NewMempoolTransactionInfo {


In dff5ad3

Worth adding a doxygen comment here? Even something simple like:

/** * Holds information about new transactions added to the mempool. * In addition to TransactionInfo includes limit bypass, package, chain and parent info. */

willcl-ark · 2023-11-24T10:03:04Z

src/kernel/mempool_entry.h

+struct NewMempoolTransactionInfo {
+    TransactionInfo info;
+    /*
+     * This boolean indicates whether the transaction was added


In dff5ad3

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?

willcl-ark · 2023-11-24T10:08:30Z

src/init.cpp


-    // Drop transactions we were still watching, and record fee estimations.
-    if (node.fee_estimator) node.fee_estimator->Flush();
+    // Drop transactions we were still watching, record fee estimations and Unregister


micro nit, if you end up touching 7145239 again:

Suggested change

// Drop transactions we were still watching, record fee estimations and Unregister

// Drop transactions we were still watching, record fee estimations and unregister

willcl-ark · 2023-11-24T12:31:27Z

src/test/fuzz/policy_estimator.cpp

+                const auto tx_info = NewMempoolTransactionInfo(entry.GetSharedTx(), entry.GetFee(),
+                                                               entry.GetTxSize(), entry.GetHeight(),
+                                                               /* m_from_disconnected_block */ false,
+                                                               /* m_submitted_in_package */ false,


In 7145239

Would we want to use fuzzed_data_provider.ConsumeBool() for m_submitted_in_package?

ismaelsadeeq · 2023-11-24T19:30:30Z

Thanks for your review @willcl-ark will address nits if there is need to retouch.

achow101 · 2023-12-01T19:57:57Z

ACK 91504cb

pablomartin4btc

post merge tACK 91504cb

Tested manually using RPC commands estimatesmartfee and estimaterawfee (following cases from /test/functional/feature_fee_estimation.py).

ismaelsadeeq marked this pull request as ready for review August 30, 2023 10:50

ismaelsadeeq marked this pull request as draft August 30, 2023 10:51

DrahtBot mentioned this pull request Aug 30, 2023

validate package transactions with their in-package ancestor sets #26711

Closed

vincenzopalazzo reviewed Aug 30, 2023

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

vincenzopalazzo reviewed Aug 30, 2023

View reviewed changes

src/policy/fees.cpp Outdated Show resolved Hide resolved

vincenzopalazzo reviewed Aug 30, 2023

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 30, 2023

policy: nVersion=3 and Package RBF #25038

Closed

glozow added the TX fees and policy label Aug 31, 2023

glozow reviewed Aug 31, 2023

View reviewed changes

ismaelsadeeq force-pushed the 08-2023-fee-estimator-updates-from-validation-interface-signal branch from a7250f6 to b45345d Compare September 1, 2023 10:35

ismaelsadeeq marked this pull request as ready for review September 1, 2023 10:35

ismaelsadeeq force-pushed the 08-2023-fee-estimator-updates-from-validation-interface-signal branch from b45345d to 2f6d2b1 Compare September 1, 2023 10:45

DrahtBot added the CI failed label Sep 1, 2023

ismaelsadeeq force-pushed the 08-2023-fee-estimator-updates-from-validation-interface-signal branch from 2f6d2b1 to db4849c Compare September 1, 2023 10:55

DrahtBot removed the CI failed label Sep 1, 2023

glozow mentioned this pull request Sep 1, 2023

Macros/mempool interface vincenzopalazzo/bitcoin#1

Open

DrahtBot mentioned this pull request Sep 2, 2023

refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access #28391

Merged

sedited reviewed Sep 3, 2023

View reviewed changes

src/validationinterface.h Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

src/validationinterface.h Outdated Show resolved Hide resolved

src/policy/fees.cpp Outdated Show resolved Hide resolved

DrahtBot added the CI failed label Sep 3, 2023

ismaelsadeeq force-pushed the 08-2023-fee-estimator-updates-from-validation-interface-signal branch from db4849c to 9ae8482 Compare September 3, 2023 17:35

sedited approved these changes Nov 20, 2023

View reviewed changes

src/policy/fees.cpp Outdated Show resolved Hide resolved

DrahtBot requested review from glozow, hernanmarino and vincenzopalazzo and removed request for hernanmarino and vincenzopalazzo November 20, 2023 22:06

ismaelsadeeq force-pushed the 08-2023-fee-estimator-updates-from-validation-interface-signal branch from 7268aaf to 781c60f Compare November 22, 2023 00:25

ismaelsadeeq and others added 7 commits November 22, 2023 11:48

tx fees, policy: cast with static_cast instead of C-Style cast

0889e07

tx fees, policy: update CBlockPolicyEstimator::processBlock parameter

91532bd

Update `processBlock` parameter to reference to a vector of `RemovedMempoolTransactionInfo`.

CValidationInterface: modify the parameter of `TransactionAddedToMemp…

dff5ad3

…ool` Create a new struct `NewMempoolTransactionInfo` that will be used as the new parameter of `TransactionAddedToMempool` callback.

rpc: SyncWithValidationInterfaceQueue on fee estimation RPC's

91504cb

This ensures that the most recent fee estimation data is used for the fee estimation with `estimateSmartfee` and `estimaterawfee` RPC's.

sedited approved these changes Nov 22, 2023

View reviewed changes

willcl-ark approved these changes Nov 24, 2023

View reviewed changes

DrahtBot mentioned this pull request Nov 28, 2023

kernel: Remove dependency on CScheduler #28960

Merged

pablomartin4btc reviewed Dec 5, 2023

View reviewed changes

This was referenced Dec 6, 2023

Test policyestimator_tests/BlockPolicyEstimates failure #29000

Closed

test: doc: follow-up #28368 #29013

Merged

ismaelsadeeq mentioned this pull request Jul 4, 2024

Fee Estimation via Fee rate Forecasters tracking issue #30392

Closed

7 tasks

ismaelsadeeq mentioned this pull request Sep 14, 2024

Mempool + Validation Code Organization #25584

Closed

	* Provides the block that was connected.
	* Provides the block that was disconnected.

	// Drop transactions we were still watching, record fee estimations and Unregister
	// Drop transactions we were still watching, record fee estimations and unregister

Conversation

ismaelsadeeq commented Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

willcl-ark commented Aug 30, 2023

Uh oh!

instagibbs commented Aug 30, 2023

Uh oh!

Uh oh!

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glozow commented Aug 31, 2023

Uh oh!

darosior commented Aug 31, 2023

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ismaelsadeeq commented Nov 20, 2023

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ismaelsadeeq commented Nov 22, 2023

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

willcl-ark Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

willcl-ark Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

willcl-ark Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

willcl-ark Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

willcl-ark Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq commented Nov 24, 2023

Uh oh!

achow101 commented Dec 1, 2023

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

ismaelsadeeq commented Aug 30, 2023 •

edited

Loading

DrahtBot commented Aug 30, 2023 •

edited

Loading

hebasto commented Nov 22, 2023 •

edited

Loading