Fee Estimator updates from Validation Interface/CScheduler thread #28368

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

TheCharlatan

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

@@ -140,6 +150,7 @@ class CValidationInterface {
    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

@@ -208,4 +208,33 @@ struct RemovedMempoolTransactionInfo {
        : 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

@@ -285,8 +285,12 @@ void Shutdown(NodeContext& node)
        DumpMempool(*node.mempool, MempoolPath(*node.args));
    }

-    // 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).

b1318dc test: change `m_submitted_in_package` input to fuzz data provider boolean (ismaelsadeeq) 5615e16 tx fees: update `m_from_disconnected_block` to `m_mempool_limit_bypassed` (ismaelsadeeq) fcd4296 doc: fix typo and update incorrect comment (ismaelsadeeq) 562664d test: wait for fee estimator to catch up before estimating fees (ismaelsadeeq) Pull request description: This is a simple PR that does two things 1. Fixes #29000 by waiting for the fee estimator to catch up after `removeForBlock` calls before calling `estimateFee` in the `BlockPolicyEstimates` unit test. 2. Addressed some outstanding review comments from #28368 - Updated `NewMempoolTransactionInfo::m_from_disconnected_block` to `NewMempoolTransactionInfo::m_mempool_limit_bypassed` which now correctly indicates what the boolean does. - Changed input of `processTransaction`'s tx_info `m_submitted_in_package` input from false to fuzz data provider boolean. - Fixed some typos, and update incorrect comment ACKs for top commit: martinus: re-ACK b1318dc glozow: utACK b1318dc Tree-SHA512: 45268729bc044da4748fe004524e0df696d2ec92c5bd053db9aad6e15675f3838429b2a7b9061a6b694be4dc319d1782a876b44df506ddd439d62ad07252d0e1

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

TheCharlatan 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

TheCharlatan approved these changes Nov 22, 2023

View reviewed changes

DrahtBot requested review from hernanmarino and vincenzopalazzo and removed request for hernanmarino and vincenzopalazzo November 22, 2023 12:39

willcl-ark approved these changes Nov 24, 2023

View reviewed changes

DrahtBot requested review from vincenzopalazzo and hernanmarino and removed request for hernanmarino and vincenzopalazzo November 24, 2023 12:56

DrahtBot requested review from vincenzopalazzo and hernanmarino and removed request for hernanmarino and vincenzopalazzo November 24, 2023 19:30

DrahtBot mentioned this pull request Nov 28, 2023

kernel: Remove dependency on CScheduler #28960

Merged

DrahtBot requested review from hernanmarino and vincenzopalazzo and removed request for vincenzopalazzo and hernanmarino December 1, 2023 19:58

achow101 merged commit a97a892 into bitcoin:master Dec 1, 2023
16 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fee Estimator updates from Validation Interface/CScheduler thread #28368

Fee Estimator updates from Validation Interface/CScheduler thread #28368

ismaelsadeeq commented Aug 30, 2023 •

edited

DrahtBot commented Aug 30, 2023 •

edited

willcl-ark commented Aug 30, 2023

instagibbs commented Aug 30, 2023

vincenzopalazzo left a comment

glozow commented Aug 31, 2023

darosior commented Aug 31, 2023

glozow left a comment

TheCharlatan left a comment

willcl-ark left a comment

willcl-ark Nov 23, 2023

willcl-ark Nov 24, 2023

willcl-ark Nov 24, 2023

willcl-ark Nov 24, 2023

willcl-ark Nov 24, 2023

ismaelsadeeq commented Nov 24, 2023

achow101 commented Dec 1, 2023

pablomartin4btc left a comment

	* 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

Fee Estimator updates from Validation Interface/CScheduler thread #28368

Fee Estimator updates from Validation Interface/CScheduler thread #28368

Conversation

ismaelsadeeq commented Aug 30, 2023 • edited

DrahtBot commented Aug 30, 2023 • edited

Code Coverage

Reviews

Conflicts

willcl-ark commented Aug 30, 2023

instagibbs commented Aug 30, 2023

vincenzopalazzo left a comment

Choose a reason for hiding this comment

glozow commented Aug 31, 2023

darosior commented Aug 31, 2023

glozow left a comment

Choose a reason for hiding this comment

TheCharlatan left a comment

Choose a reason for hiding this comment

willcl-ark left a comment

Choose a reason for hiding this comment

willcl-ark Nov 23, 2023

Choose a reason for hiding this comment

willcl-ark Nov 24, 2023

Choose a reason for hiding this comment

willcl-ark Nov 24, 2023

Choose a reason for hiding this comment

willcl-ark Nov 24, 2023

Choose a reason for hiding this comment

willcl-ark Nov 24, 2023

Choose a reason for hiding this comment

ismaelsadeeq commented Nov 24, 2023

achow101 commented Dec 1, 2023

pablomartin4btc left a comment

Choose a reason for hiding this comment

ismaelsadeeq commented Aug 30, 2023 •

edited

DrahtBot commented Aug 30, 2023 •

edited