Fee Estimator updates from Validation Interface/CScheduler thread

ismaelsadeeq commented at 10:45 am on August 30, 2023: member

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 commented at 10:45 am on August 30, 2023: contributor

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.

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

in src/init.cpp:352 in a7250f6597 outdated

347@@ -348,6 +348,10 @@ void Shutdown(NodeContext& node)
348     node.chainman.reset();
349     node.scheduler.reset();
350 
351+    if (node.fee_estimator) {
352+        UnregisterValidationInterface(node.fee_estimator.get());

vincenzopalazzo commented at 3:22 pm on August 30, 2023:

0        UnregisterSharedValidationInterface(node.fee_estimator);

if you want you can also do the check of null pointer inside the UnregisterValidationInterface

ismaelsadeeq commented at 10:59 am on September 1, 2023:

Thanks, I will leave this as is since there is a check above.

in src/policy/fees.cpp:621 in a7250f6597 outdated

625-    feeStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK());
626-    shortStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK());
627-    longStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK());
628+    feeStats->Record(blocksToConfirm, (double)feerate);
629+    shortStats->Record(blocksToConfirm, (double)feerate);
630+    longStats->Record(blocksToConfirm, (double)feerate);

vincenzopalazzo commented at 3:23 pm on August 30, 2023:

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 a7250f6597 outdated

610@@ -618,33 +611,25 @@ void CTxMemPool::removeConflicts(const CTransaction &tx)
611 }
612 
613 /**
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`  .

vincenzopalazzo commented at 3:24 pm on August 30, 2023:

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.

in src/txmempool.cpp:405 in a7250f6597 outdated

386@@ -387,7 +387,6 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee,
387 
388 CTxMemPool::CTxMemPool(const Options& opts)
389     : m_check_ratio{opts.check_ratio},
390-      minerPolicyEstimator{opts.estimator},

glozow commented at 4:32 pm on August 31, 2023:

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 5896c742ca outdated

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;

glozow commented at 4:46 pm on August 31, 2023:

5896c742ca6542ac997ca4ac693d429193c22e57 reserve for size equal to block.vtx.size()?

ismaelsadeeq commented at 10:37 am on September 1, 2023:

Fixed thank you

in src/test/fuzz/tx_pool.cpp:62 in c343bb37eb outdated

59@@ -59,9 +60,9 @@ struct TransactionsDelta final : public CValidationInterface {
60     explicit TransactionsDelta(std::set<CTransactionRef>& r, std::set<CTransactionRef>& a)
61         : m_removed{r}, m_added{a} {}
62 
63-    void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t /* mempool_sequence */) override
64+    void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx_info, uint64_t /* mempool_sequence */) override

glozow commented at 4:47 pm on August 31, 2023:

c343bb37ebe15219ac840de2deef1111a5d9be31 nit: would slightly prefer keeping tx outside of the struct

ismaelsadeeq commented at 10:37 am on September 1, 2023:

I remove it out of the struct, thank you.

in src/validation.cpp:1200 in c343bb37eb outdated

1195@@ -1196,7 +1196,14 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
1196             results.emplace(ws.m_ptx->GetWitnessHash(),
1197                 MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize,
1198                                              ws.m_base_fees, effective_feerate, effective_feerate_wtxids));
1199-            GetMainSignals().TransactionAddedToMempool(ws.m_ptx, m_pool.GetAndIncrementSequence());
1200+            const CTransaction& tx = *ws.m_ptx;
1201+            bool validForFeeEstimation = !args.m_bypass_limits && !args.m_package_submission && IsCurrentForFeeEstimation(m_active_chainstate) && m_pool.HasNoInputsOf(tx);

glozow commented at 4:52 pm on August 31, 2023:

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:

0bool m_from_disconnected_block;
1bool m_submitted_in_package;
2bool m_chainstate_is_current;
3bool m_has_mempool_parents;

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 bbfe01fa9f outdated

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-

glozow commented at 4:57 pm on August 31, 2023:

bbfe01fa9fabec3982b7cfc25c08b94653fb5b52

can you provide rationale why it’s ok to remove this? Could be worth splitting into its own commit.

ismaelsadeeq commented at 10:52 am on September 1, 2023:

This was from rebase in a734d1d166f9936e453b3c1f65b7a569af894082 I think it has to do with Handle reorgs much better in fee estimator.

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 4f22c0da09 outdated

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+        }

glozow commented at 5:05 pm on August 31, 2023:

4f22c0da092e1c2287e4bc2d84e646d56abbe133 Why move this into the reason != BLOCK case?

ismaelsadeeq commented at 10:57 am on September 1, 2023:

Any transaction whose removal reason is BLOCK will be removed by the fee estimator from the processBlock call in removeForBlock.

Edit Having removeTx(hash, false); call outside reason != BLOCK is incorrect.

If the reason for tx removal is BLOCK the removeTx boolean argument should be true.

The reason why this passed before is that the fee estimator must have finish removing all txs whose reason is BLOCK before the mempool clears.

It suppose to be in the reason != BLOCK case.

glozow commented at 3:36 pm on November 2, 2023:

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 db4849c354 outdated

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:

Thank you for your review. I updated the name

in src/init.cpp:351 in db4849c354 outdated

347@@ -348,6 +348,10 @@ void Shutdown(NodeContext& node)
348     node.chainman.reset();
349     node.scheduler.reset();
350 
351+    if (node.fee_estimator) {

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 a34aaf88c1 outdated

 6@@ -7,6 +7,7 @@
 7 #define BITCOIN_VALIDATIONINTERFACE_H
 8 
 9 #include <kernel/cs_main.h>
10+#include <kernel/mempool_entry.h>

TheCharlatan commented at 9:46 am on September 3, 2023:

Nit: Could forward-declare NewMempoolTransactionInfo instead (and in other places where this is included in a header).

ismaelsadeeq commented at 5:36 pm on September 3, 2023:

Thanks fixed

in src/policy/fees.cpp:624 in 3b17461f74 outdated

620-    int blocksToConfirm = nBlockHeight - entry->GetHeight();
621+    int blocksToConfirm = nBlockHeight - pos->second.blockHeight;
622+
623+    // Feerates are stored and reported as BTC-per-kb:
624+    CAmount feerate = pos->second.m_fee_per_k;
625+    _removeTx(tx->GetHash(), true);

TheCharlatan commented at 10:15 am on September 3, 2023:

Why is this line moved?

ismaelsadeeq commented at 5:48 pm on September 3, 2023:

Because _removeTx clear the tx from mapMemPoolTxs. So using

0 _removeTx(tx->GetHash(), true);
1 std::map<uint256, TxStatsInfo>::iterator pos = mapMemPoolTxs.find(tx->GetHash());

pos will always be an empty iterator.

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

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

in src/kernel/mempool_entry.h:191 in 2d4b66b3b9 outdated

186 };
187 
188+struct NewMempoolTransactionInfo {
189+    CTransactionRef m_tx;
190+    std::vector<CTransactionRef> m_parents;
191+    //! The fee the added transaction paid

vincenzopalazzo commented at 9:30 am on October 14, 2023:

why not

0    /* The fee the added transaction paid */

vincenzopalazzo commented at 9:31 am on October 14, 2023: none

Sorry if this get me so long

Concept ACK https://github.com/bitcoin/bitcoin/pull/28368/commits/2d4b66b3b9fd907387fe96676c025b54d9925ebb

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 79bcc5ca06 outdated

19@@ -20,6 +20,8 @@
20 
21 using node::NodeContext;
22 
23+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.

in src/policy/fees.cpp:572 in 576de764e0 outdated

570+void CBlockPolicyEstimator::processTransaction(const NewMempoolTransactionInfo& tx_info, bool validForFeeEstimation)
571 {
572     LOCK(m_cs_fee_estimator);
573-    unsigned int txHeight = entry.GetHeight();
574-    uint256 hash = entry.GetTx().GetHash();
575+    const CTransactionRef& tx = tx_info.m_tx;

TheCharlatan commented at 6:10 am on October 27, 2023:

Nit: Why introduce this temporary?

ismaelsadeeq commented at 11:38 am on October 30, 2023:

Removed, since I can get the hash directly with tx_info.m_tx->GetHash();

in src/test/policyestimator_tests.cpp:71 in 79bcc5ca06 outdated

76-                         false,
77-                         true,
78-                         true},
79-                        mpool.GetAndIncrementSequence());
80+                    NewMempoolTransactionInfo tx_info;
81+                    tx_info.m_tx = MakeTransactionRef(tx);

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 79bcc5ca06 outdated

169@@ -170,6 +170,15 @@ class CTxMemPoolEntry
170     const Parents& GetMemPoolParentsConst() const { return m_parents; }

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

ACK 79bcc5ca0679daf6e57fc4d7ce2244262a7cfd13

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 59db4d1739 outdated

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.

glozow commented at 3:20 pm on October 30, 2023:

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:

Thank you removed

in src/txmempool.cpp:647 in 59db4d1739 outdated

657+                .m_fee = it->GetFee(),
658+                .m_virtual_transaction_size = it->GetTxSize(),
659+                .txHeight = it->GetHeight(),
660+                .nSizeWithAncestors = it->GetSizeWithAncestors(),
661+                .nModFeesWithAncestors = it->GetModFeesWithAncestors()};
662+            txs_removed_for_block.push_back(tx_info);

glozow commented at 3:22 pm on October 30, 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 59db4d1739 outdated

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) {}

glozow commented at 3:26 pm on October 30, 2023:

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 59db4d1739 outdated

81+                        .m_parents = mempool_entry.GetMemPoolParentsCopy(),
82+                        .m_fee = mempool_entry.GetFee(),
83+                        .m_virtual_transaction_size = mempool_entry.GetTxSize(),
84+                        .txHeight = mempool_entry.GetHeight(),
85+                        .nSizeWithAncestors = mempool_entry.GetSizeWithAncestors(),
86+                        .nModFeesWithAncestors = mempool_entry.GetModFeesWithAncestors(),

TheCharlatan commented at 9:52 pm on October 30, 2023:

Nitty nit: Inconsistent with closing bracket placement.

in src/test/fuzz/policy_estimator.cpp:46 in 59db4d1739 outdated

41@@ -41,7 +42,20 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator)
42                     return;
43                 }
44                 const CTransaction tx{*mtx};
45-                block_policy_estimator.processTransaction(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx), fuzzed_data_provider.ConsumeBool());
46+                const CTxMemPoolEntry entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, tx);
47+                NewMempoolTransactionInfo tx_info = {

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-ACK 59db4d1739de1a008e6f34d0f03c3e8e8da60a93

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 4986edb99f outdated

213@@ -214,6 +214,16 @@ void CMainSignals::TransactionAddedToMempool(const CTransactionRef& tx, uint64_t
214                           tx->GetWitnessHash().ToString());
215 }
216 
217+void CMainSignals::MempoolTransactionsRemovedForConnectedBlock(const std::vector<CTransactionRef>& txs_removed_for_block, unsigned int nBlockHeight)
218+{
219+    auto event = [txs_removed_for_block, nBlockHeight, this] {
220+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.MempoolTransactionsRemovedForConnectedBlock(txs_removed_for_block, nBlockHeight); });
221+    };
222+    ENQUEUE_AND_LOG_EVENT(event, "%s: block height=%s txs=%s", __func__,

glozow commented at 2:45 pm on November 2, 2023:

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 bb22a5148a outdated

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;

glozow commented at 2:49 pm on November 2, 2023:

bb22a5148af0f8aad3d8e7b242cc65fe6dd4ced9 this is missing a doxygen comment

ismaelsadeeq commented at 7:23 pm on November 3, 2023:

Added

in src/policy/fees.h:287 in f03720ee99 outdated

273@@ -272,6 +274,7 @@ class CBlockPolicyEstimator
274     {
275         unsigned int blockHeight{0};
276         unsigned int bucketIndex{0};
277+        CAmount m_fee_per_k;

glozow commented at 2:51 pm on November 2, 2023:

f03720ee9962b3c4657cd59ce1ea2125ec9b1cbe

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:

Thank you, this is removed in latest push

ismaelsadeeq force-pushed on Nov 2, 2023

in src/policy/fees.cpp:640 in 9b0843042a outdated

636@@ -634,7 +637,7 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTra
637 }
638 
639 void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight,
640-                                         std::vector<CTransactionRef>& txs_removed_for_block)
641+                                         const std::vector<CTransactionRef>& txs_removed_for_block)

glozow commented at 2:58 pm on November 2, 2023:

You’re making this const in 9b0843042ab5c30e6ef4753d590c69c4811d9a70 - why not just introduce it as const?

ismaelsadeeq commented at 7:23 pm on November 3, 2023:

Fixed

in src/kernel/mempool_entry.h:197 in 9b0843042a outdated

190@@ -191,6 +191,10 @@ struct NewMempoolTransactionInfo {
191      */
192     int64_t m_virtual_transaction_size;
193     unsigned int txHeight;
194+    bool m_from_disconnected_block;
195+    bool m_submitted_in_package;
196+    bool m_chainstate_is_current;
197+    bool m_has_no_mempool_parents;

glozow commented at 3:02 pm on November 2, 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.

in src/validation.cpp:1226 in 9b0843042a outdated

1222-            .txHeight = ws.m_entry->GetHeight()};
1223+            .txHeight = ws.m_entry->GetHeight(),
1224+            .m_from_disconnected_block = args.m_bypass_limits,
1225+            .m_submitted_in_package = args.m_package_submission,
1226+            .m_chainstate_is_current = IsCurrentForFeeEstimation(m_active_chainstate),
1227+            .m_has_no_mempool_parents = m_pool.HasNoInputsOf(tx)};

glozow commented at 3:12 pm on November 2, 2023:

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 4986edb99f outdated

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) {}

glozow commented at 3:16 pm on November 2, 2023:

This function is newly added in 4986edb99f8aa73f72e87f3bdc09387c3e516197 and then changed in 863d46751900a0c69c4f7fbd92d4d8a651b2cc7b

ismaelsadeeq commented at 7:33 pm on November 3, 2023:

Fixed

in src/test/fuzz/policy_estimator.cpp:73 in 863d467519 outdated

69@@ -68,10 +70,18 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator)
70                     const CTransaction tx{*mtx};
71                     mempool_entries.push_back(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx));
72                 }
73-                std::vector<CTransactionRef> txs;
74+                std::vector<const NewMempoolTransactionInfo> txs;

glozow commented at 3:18 pm on November 2, 2023:

863d46751900a0c69c4f7fbd92d4d8a651b2cc7b why are you making the element type const?

ismaelsadeeq commented at 4:13 pm on November 2, 2023:

This is fixed in latest push.

ismaelsadeeq force-pushed on Nov 2, 2023

DrahtBot added the label CI failed on Nov 2, 2023

in src/kernel/mempool_entry.h:205 in 53bbc791d4 outdated

200@@ -191,6 +201,8 @@ struct NewMempoolTransactionInfo {
201      */
202     int64_t m_virtual_transaction_size;
203     unsigned int txHeight;
204+    int64_t nSizeWithAncestors;
205+    CAmount nModFeesWithAncestors;

glozow commented at 3:25 pm on November 2, 2023:

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 09436b21e84cc6cd979fe4942ba1c415c4bc91be Compare changes

Rework the commits in the order of this review comment which is more cleaner and less verbose.
Addressed @glozow review comments.
Cherry picked https://github.com/TheCharlatan/bitcoin/commit/32846141df1a2e110f84f3244d5e98626d379185 commit which removes policy/fees from kernel library.

in src/kernel/mempool_entry.h:207 in 79803d3912 outdated

199+          m_virtual_transaction_size(vsize),
200+          txHeight(height) {}
201+};
202+
203+struct RemovedMempoolTransactionInfo : public TransactionInfo {
204+    RemovedMempoolTransactionInfo(const CTxMemPoolEntry& entry)

TheCharlatan commented at 9:24 pm on November 3, 2023:

Mark constructors as explicit.

ismaelsadeeq commented at 12:55 pm on November 20, 2023:

Fixed

in src/txmempool.cpp:493 in 6f40c32fa8 outdated

495@@ -496,12 +496,16 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
496     // even if not directly reported below.

TheCharlatan commented at 9:29 pm on November 3, 2023:

In commit 80ea259b0b8bb08637d1cb75f65ec342d03eae7a, I find the second sentence of the commit message confusing. Is there something missing?

ismaelsadeeq commented at 12:55 pm on November 20, 2023:

I updated the commit message 👍🏾

in src/Makefile.am:957 in 09436b21e8 outdated

950@@ -951,7 +951,6 @@ libbitcoinkernel_la_SOURCES = \
951   node/chainstate.cpp \
952   node/utxo_snapshot.cpp \
953   policy/feerate.cpp \
954-  policy/fees.cpp \

glozow commented at 4:13 pm on November 7, 2023:

09436b21e84cc6cd979fe4942ba1c415c4bc91be can be squashed

ismaelsadeeq commented at 10:42 am on November 8, 2023:

Done

in src/txmempool.cpp:499 in 6f40c32fa8 outdated

495@@ -496,12 +496,16 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
496     // even if not directly reported below.
497     uint64_t mempool_sequence = GetAndIncrementSequence();
498 
499+    const uint256 hash = it->GetTx().GetHash();

glozow commented at 4:26 pm on November 7, 2023:

6f40c32fa8dfc742cf5ca7daa28615e066872aec

maybe stop making a copy, and let it be a Txid

0    const auto& hash = it->GetTx().GetHash();

ismaelsadeeq commented at 10:42 am on November 8, 2023:

👍🏾 Done

in src/policy/fees.cpp:583 in ac82f58e45 outdated

581 {
582     LOCK(m_cs_fee_estimator);
583-    unsigned int txHeight = entry.GetHeight();
584-    uint256 hash = entry.GetTx().GetHash();
585+    unsigned int txHeight = tx_info.txHeight;
586+    uint256 hash = tx_info.m_tx->GetHash();

glozow commented at 4:31 pm on November 7, 2023:

ac82f58e45421aa5aec1d3e6e992d1c369472a10

0    const auto& hash = tx_info.m_tx->GetHash();

ismaelsadeeq commented at 10:43 am on November 8, 2023:

Updated, thank you

in src/test/fuzz/policy_estimator.cpp:53 in ac82f58e45 outdated

46+                const auto tx_info = NewMempoolTransactionInfo(entry.GetSharedTx(), entry.GetFee(),
47+                                                               entry.GetTxSize(), entry.GetHeight(),
48+                                                               /* m_from_disconnected_block */ fuzzed_data_provider.ConsumeBool(),
49+                                                               /* m_submitted_in_package */ fuzzed_data_provider.ConsumeBool(),
50+                                                               /* m_chainstate_is_current */ fuzzed_data_provider.ConsumeBool(),
51+                                                               /* m_has_no_mempool_parents */ fuzzed_data_provider.ConsumeBool());

glozow commented at 4:35 pm on November 7, 2023:

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 ac82f58e45 outdated

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),

glozow commented at 4:38 pm on November 7, 2023:

ac82f58e45421aa5aec1d3e6e992d1c369472a10 a bit less verbose

0                    const NewMempoolTransactionInfo tx_info{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 commented at 3:39 pm on November 8, 2023: member

Force pushed from 09436b21e84cc6cd979fe4942ba1c415c4bc91be to 969f5ec4a8

Addressed @glozow review comments
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 a9d48d9a74 outdated

229+                              const int64_t vsize, const unsigned int height,
230+                              const bool from_disconnected_block, const bool submitted_in_package,
231+                              const bool chainstate_is_current,
232+                              const bool has_no_mempool_parents)
233+        : info(tx, fee, vsize, height),
234+          m_from_disconnected_block(from_disconnected_block),

TheCharlatan commented at 1:58 pm on November 19, 2023:

Nit: Doesn’t really matter here, but could use list initialization.

ismaelsadeeq commented at 12:56 pm on November 20, 2023:

Updated with your suggestion

in src/policy/fees.cpp:584 in 4ae756d3ae outdated

580+    processTransaction(tx);
581+}
582+
583+void CBlockPolicyEstimator::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence)
584+{
585+    removeTx(tx->GetHash(), false);

TheCharlatan commented at 2:24 pm on November 19, 2023:

The bool in removeTx is always false now, maybe remove it entirely?

ismaelsadeeq commented at 12:57 pm on November 20, 2023:

This will change the input of the CBlockPolicyEstimator fuzzing test, I added a docstring /*inBlock*/.

TheCharlatan commented at 1:13 pm on November 20, 2023:

Keeping around a dead branch for the purpose of fuzzing seems weird to me. Besides the inBlock case is still exercised from processBlock.

ismaelsadeeq commented at 3:45 pm on November 20, 2023:

I agree with this point, fixed. Thank you

in src/policy/fees.cpp:577 in 4ae756d3ae outdated

573@@ -579,11 +574,26 @@ CBlockPolicyEstimator::CBlockPolicyEstimator(const fs::path& estimation_filepath
574 
575 CBlockPolicyEstimator::~CBlockPolicyEstimator() = default;
576 
577-void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate)
578+void CBlockPolicyEstimator::TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence)

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 commented at 4:54 pm on November 20, 2023: member

CI failure is unrelated see https://github.com/bitcoin/bitcoin/pull/28905

in src/policy/fees.cpp:595 in 93a19d7151 outdated

593+void CBlockPolicyEstimator::processTransaction(const NewMempoolTransactionInfo& tx)
594 {
595     LOCK(m_cs_fee_estimator);
596-    unsigned int txHeight = entry.GetHeight();
597-    uint256 hash = entry.GetTx().GetHash();
598+    unsigned int txHeight = tx.info.txHeight;

TheCharlatan commented at 9:50 pm on November 20, 2023:

Nit: Since you’re already making hash const, maybe make txHeight and feeRate const too?

ismaelsadeeq commented at 10:30 am on November 22, 2023:

Fixed Thanks

TheCharlatan approved

TheCharlatan commented at 10:06 pm on November 20, 2023: contributor

ACK 7268aaf36bdcc6b466fd16bbf544782571e680bb

DrahtBot removed review request from vincenzopalazzo on Nov 20, 2023

DrahtBot requested review from vincenzopalazzo on Nov 20, 2023

DrahtBot requested review from glozow on Nov 20, 2023

DrahtBot requested review from hernanmarino on Nov 20, 2023

DrahtBot removed review request from vincenzopalazzo on Nov 20, 2023

DrahtBot removed review request from hernanmarino on Nov 20, 2023

DrahtBot requested review from hernanmarino on Nov 20, 2023

DrahtBot requested review from vincenzopalazzo on Nov 20, 2023

ismaelsadeeq force-pushed on Nov 22, 2023

hebasto commented at 7:48 am on November 22, 2023: member

CI restarted.

UPD. See https://github.com/bitcoin/bitcoin/pull/28925

tx fees, policy: bugfix: move `removeTx` into reason != `BLOCK` condition

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 cast 0889e07987

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

bfcd401368

tx fees, policy: update `CBlockPolicyEstimator::processBlock` parameter

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

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 #28905 and #28925 to fix CI failure

TheCharlatan approved

TheCharlatan commented at 12:39 pm on November 22, 2023: contributor

Re-ACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2

DrahtBot removed review request from hernanmarino on Nov 22, 2023

DrahtBot removed review request from vincenzopalazzo on Nov 22, 2023

DrahtBot requested review from hernanmarino on Nov 22, 2023

DrahtBot requested review from vincenzopalazzo on Nov 22, 2023

in src/validationinterface.h:153 in 91504cbe0d

149@@ -140,6 +150,7 @@ class CValidationInterface {
150     virtual void BlockConnected(ChainstateRole role, const std::shared_ptr<const CBlock> &block, const CBlockIndex *pindex) {}
151     /**
152      * Notifies listeners of a block being disconnected
153+     *  Provides the block that was connected.

willcl-ark commented at 3:42 pm on November 23, 2023:

nit if you retouch bfcd401368fc0dc43827a8969a37b7e038d5ca79:

0     * Provides the block that was disconnected.

in src/kernel/mempool_entry.h:211 in dff5ad3b99 outdated

207@@ -208,4 +208,33 @@ struct RemovedMempoolTransactionInfo {
208         : info{entry.GetSharedTx(), entry.GetFee(), entry.GetTxSize(), entry.GetHeight()} {}
209 };
210 
211+struct NewMempoolTransactionInfo {

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 dff5ad3b99 outdated

207@@ -208,4 +208,33 @@ struct RemovedMempoolTransactionInfo {
208         : info{entry.GetSharedTx(), entry.GetFee(), entry.GetTxSize(), entry.GetHeight()} {}
209 };
210 
211+struct NewMempoolTransactionInfo {
212+    TransactionInfo info;
213+    /*
214+     * This boolean indicates whether the transaction was added

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 714523918b outdated

284@@ -285,8 +285,12 @@ void Shutdown(NodeContext& node)
285         DumpMempool(*node.mempool, MempoolPath(*node.args));
286     }
287 
288-    // 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

43@@ -44,9 +44,16 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator)
44                     return;
45                 }
46                 const CTransaction tx{*mtx};
47-                block_policy_estimator.processTransaction(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx), fuzzed_data_provider.ConsumeBool());
48+                const CTxMemPoolEntry& entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, tx);
49+                const auto tx_info = NewMempoolTransactionInfo(entry.GetSharedTx(), entry.GetFee(),
50+                                                               entry.GetTxSize(), entry.GetHeight(),
51+                                                               /* m_from_disconnected_block */ false,
52+                                                               /* m_submitted_in_package */ false,

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

ACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2

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

ACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2

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 tACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2

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

ismaelsadeeq deleted the branch on Jun 27, 2024

Fee Estimator updates from Validation Interface/CScheduler thread #28368

Code Coverage

Reviews

Conflicts