Move fee estimator into validationinterface/cscheduler thread

TheBlueMatt commented at 7:30 pm on November 27, 2017: contributor

This does a few things in a PR, so I’m happy to split it up if people prefer.

a) It clarifies the validationinterface stuff into a mempoolinterface and the validationinterface, splitting the two up into separate systems with the mempool creating events for mempoolinterface and validationinterface functions coming from validation. They are conjoined in the backend to provide ordering guarantees for listeners which are both mempool interfaces and validation interfaces, but are otherwise fully separated. There are a few cleanups that are enabled here, which I went ahead and did.

b) fee estimator becomes a listener to the mempool interface which provides a bit of additional info about transactions being added more than just the tx (eg feerate, etc)

c) a few minor edge cases in the fee estimator are handled (witness malleation changing feerate, and handling reorgs better, specifically).

TheBlueMatt force-pushed on Nov 27, 2017

TheBlueMatt commented at 8:29 pm on November 27, 2017: contributor

Obviously this cleans up some of the interface gook from #10286, and adds some more docs that probably could have been there. Next up after this one is moving alll the remaining events all into the background thread, with associated net_processing cleanups in the process.

fanquake added the label TX fees and policy on Nov 27, 2017

fanquake added the label Validation on Nov 27, 2017

in src/txmempool.h:515 in 188179e008 outdated

491@@ -492,6 +492,8 @@ class CTxMemPool
492 
493     std::vector<indexed_transaction_set::const_iterator> GetSortedDepthAndScore() const;
494 
495+    void calculateRemoveRecursive(const CTransaction &tx, setEntries &stage);

jimpo commented at 3:31 am on November 29, 2017:

commit: Split removeRecursive into calculate/remove steps

style: Capitalize function name.

TheBlueMatt commented at 6:02 pm on April 5, 2018:

This PR was created long before the introduction of that part of the style guide, so it matches the capitalization of removeRecursive.

TheBlueMatt force-pushed on Dec 13, 2017

TheBlueMatt commented at 10:34 pm on December 13, 2017: contributor

Rebased.

TheBlueMatt force-pushed on Dec 15, 2017

theuni commented at 0:13 am on December 19, 2017: member

Concept ACK.

Though I’m nervous about backgrounding more stuff before safeguarding against outside (unsynchronized) access. For example, the mempool/feeestimator may now be significantly ahead of what the wallet has processed. So calling CreateTransaction when we’re in the middle of connecting 10 blocks seems… dangerous, even with the BlockUntilSyncedToCurrentChain hack.

TheBlueMatt commented at 0:34 am on December 19, 2017: contributor

Hmm? If anything the fee estimator should be at much lower risk there than for wallet…at worst you’ll get out of date fee estimates…something you’d get anyway because you’re calling the fee estimator in the middle of syncing stuff. Note that #11824 also keeps the queue depth at no more than 10, though I dont really want to rely on that and would prefer to implement the dump/reload-from-disk proposal suggested at https://github.com/bitcoin/bitcoin/pull/11775/files#diff-e8d9e22d9683f73a9fb8399be0dab640R145

TheBlueMatt force-pushed on Mar 27, 2018

TheBlueMatt commented at 9:13 pm on March 27, 2018: contributor

Rebased.

TheBlueMatt force-pushed on Mar 28, 2018

in src/txmempool.cpp:475 in f0d6d0bb6b outdated

469@@ -470,11 +470,8 @@ void CTxMemPool::CalculateDescendants(txiter entryit, setEntries &setDescendants
470     }
471 }
472 
473-void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReason reason)
474+void CTxMemPool::calculateRemoveRecursive(const CTransaction &origTx, setEntries &setAllRemoves)

jimpo commented at 5:23 pm on April 2, 2018:

commit: Split removeRecursive into calculate/remove steps

Should this AssertLockHeld?

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Done (bonus: also fixed the whitespace issues in that commit).

in src/txmempool.cpp:556 in b3a9ecb568 outdated

550@@ -551,7 +551,13 @@ void CTxMemPool::removeConflicts(const CTransaction &tx)
551             if (txConflict != tx)
552             {
553                 ClearPrioritisation(txConflict.GetHash());
554-                removeRecursive(txConflict, MemPoolRemovalReason::CONFLICT);
555+                setEntries set_removes;
556+                calculateRemoveRecursive(txConflict, set_removes);
557+                txn_removed.reserve(txn_removed.size() + set_removes.size());

jimpo commented at 5:30 pm on April 2, 2018:

commit: Track the set of transactions removed/conflicted in removeForBlock

I think this reserve call could be less performant than skipping it if there are a sufficient number of conflicting inputs. https://stackoverflow.com/a/1742961

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Good catch. Removed.

in src/wallet/wallet.cpp:1258 in a897ff03ae outdated

1231+    LOCK2(cs_main, cs_wallet);
1232+    for (const CTransactionRef& ptx : tx_removed_conflicted) {
1233+        SyncTransaction(ptx);
1234+        TransactionRemovedFromMempool(ptx, MemPoolRemovalReason::CONFLICT);
1235+    }
1236+    // We can ignore tx_removed_in_block, we'll get it in BlockConnected

jimpo commented at 6:08 pm on April 2, 2018:

commit: BlockConnected(vtxConflicted) -> New MempoolInterface callback

Why not just move that logic here from BlockConnected as well?

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Because we still need it in BlockConnected - there may be txn that weren’t in our mempool that got confirmed, so copying it would be useless.

in src/init.cpp:201 in cd74b8f63a outdated

196@@ -197,7 +197,8 @@ void Shutdown()
197     // Because these depend on each-other, we make sure that neither can be
198     // using the other before destroying them.
199     if (peerLogic) UnregisterValidationInterface(peerLogic.get());
200-    if (g_connman) g_connman->Stop();
201+    if (peerLogic) UnregisterMempoolInterface(peerLogic.get());
202+    if(g_connman) g_connman->Stop();

jimpo commented at 6:14 pm on April 2, 2018:

commit: Add txn_replaced to Added callback, remove lReplaced ATMP arg

nit: Space after if

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Fixed.

in src/txmempool.cpp:424 in cd74b8f63a outdated

417@@ -418,8 +418,10 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry,
418 
419 void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
420 {
421-    if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) {
422+    if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT &&
423+        reason != MemPoolRemovalReason::REPLACED) {
424         // BLOCK and CONFLICT callbacks are generated in removeForBlock
425+        // REPLACED txn are included in TransactionAddedToMempool from ATMP

jimpo commented at 6:23 pm on April 2, 2018:

commit: Add txn_replaced to Added callback, remove lReplaced ATMP arg

It wasn’t obvious to me what ATMP stood for. I think it would be better to spell out AddToMemoryPool for clarity.

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Done.

in src/validationinterface.cpp:155 in cd74b8f63a outdated

151@@ -152,9 +152,9 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd
152     });
153 }
154 
155-void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) {
156-    m_internals->m_schedulerClient.AddToProcessQueue([ptx, this] {
157-        m_internals->TransactionAddedToMempool(ptx);
158+void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx, const std::shared_ptr<std::vector<CTransactionRef>>& txn_replaced) {

jimpo commented at 6:39 pm on April 2, 2018:

commit: Add txn_replaced to Added callback, remove lReplaced ATMP arg

This method takes a std::shared_ptr while MempoolUpdatedForBlockConnect takes rvalue refs and constructs the std::shared_ptrs itself. I think these should be consistent.

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Obviousy avoiding the move is a bit more performant, hence its use here. The use of the move in MempoolUpdatedForBlockConnect was to reduce diff size.

in src/validationinterface.h:80 in f54c193f92 outdated

75+     * Information about a newly-added-to-mempool transaction.
76+     */
77+    struct NewMempoolTransactionInfo {
78+        //!A shared pointer to the transaction which was added.
79+        CTransactionRef m_tx;
80+        //! The fee the added thransaction paid

jimpo commented at 6:46 pm on April 2, 2018:

commit: Make fee estimation not need CTxMemPoolEntry is block processing

typo: thransaction

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Fixed.

in src/validationinterface.h:78 in f54c193f92 outdated

69@@ -70,6 +70,28 @@ void SyncWithValidationInterfaceQueue();
70  * called on the thread generating the callbacks.
71  */
72 class MempoolInterface {
73+public:
74+    /**
75+     * Information about a newly-added-to-mempool transaction.
76+     */
77+    struct NewMempoolTransactionInfo {
78+        //!A shared pointer to the transaction which was added.

jimpo commented at 6:47 pm on April 2, 2018:

commit: Make fee estimation not need CTxMemPoolEntry is block processing

nit: space after bang

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Fixed.

in src/txmempool.cpp:592 in f54c193f92 outdated

588@@ -599,6 +589,7 @@ void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigne
589         removeConflicts(*tx, txn_conflicts);
590         ClearPrioritisation(tx->GetHash());
591     }
592+    if (minerPolicyEstimator) {minerPolicyEstimator->processBlock(nBlockHeight, txn_removed_in_block);}

jimpo commented at 6:49 pm on April 2, 2018:

commit: Make fee estimation not need CTxMemPoolEntry is block processing

nit: Remove braces or move statement body onto newline.

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Fixed.

in src/txmempool.cpp:447 in f54c193f92 outdated

442@@ -443,7 +443,8 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
443     mapLinks.erase(it);
444     mapTx.erase(it);
445     nTransactionsUpdated++;
446-    if (minerPolicyEstimator) {minerPolicyEstimator->removeTx(hash, false);}
447+    if (reason != MemPoolRemovalReason::BLOCK && minerPolicyEstimator) {
448+        minerPolicyEstimator->removeTx(hash);}

jimpo commented at 6:50 pm on April 2, 2018:

commit: Make fee estimation not need CTxMemPoolEntry is block processing

nit: Move closing brace onto newline.

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Fixed.

in src/policy/fees.cpp:508 in f54c193f92 outdated

504@@ -504,20 +505,24 @@ void TxConfirmStats::removeTx(unsigned int entryHeight, unsigned int nBestSeenHe
505     }
506 }
507 
508+void CBlockPolicyEstimator::removeTx(std::map<uint256, TxStatsInfo>::iterator pos, bool inBlock) {

jimpo commented at 6:53 pm on April 2, 2018:

commit: Make fee estimation not need CTxMemPoolEntry is block processing

style: s/inBlock/in_block/

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Fixed.

in src/policy/fees.h:191 in f54c193f92 outdated

185@@ -185,20 +186,30 @@ class CBlockPolicyEstimator
186      */
187     static constexpr double FEE_SPACING = 1.05;
188 
189+    struct TxStatsInfo

jimpo commented at 6:59 pm on April 2, 2018:

commit: Make fee estimation not need CTxMemPoolEntry is block processing

Any particular reason to move this up here rather than leave it in the second private section where it is now?

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Probably just a change in code between when I wrote this and now.

in src/policy/fees.cpp:616 in f54c193f92 outdated

622+    longStats->Record(blocksToConfirm, (double)pos->second.m_fee_per_k);
623 
624-    feeStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK());
625-    shortStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK());
626-    longStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK());
627+    mapMemPoolTxs.erase(pos);

jimpo commented at 7:06 pm on April 2, 2018:

commit: Make fee estimation not need CTxMemPoolEntry is block processing

I’d move this erase immediately after the removeTx above and define double fee_per_k = (double)pos->second.m_fee_per_k; as a variable up there as well.

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Would also need to copy blockHeight as well, at which point you’d just be copying the whole struct so that you can remove it.

jimpo commented at 0:45 am on April 6, 2018:

Yeah, I think copying the struct for the sake of simplifying the control flow (by only erasing in one place) is actually good. Also would just make it more readable to have a descriptive variable name like tx_info instead of pos->second.

in src/policy/fees.h:200 in f54c193f92 outdated

192+        CAmount m_fee_per_k;
193+        TxStatsInfo() : blockHeight(0), m_fee_per_k(0) {}
194+    };
195+
196+    /** Remove a transaction from the mempool tracking stats*/
197+    void removeTx(std::map<uint256, TxStatsInfo>::iterator pos, bool inBlock);

jimpo commented at 7:11 pm on April 2, 2018:

commit: Make fee estimation not need CTxMemPoolEntry is block processing

I don’t really like that this method has the same name as the other removeTx because it’s responsibilities are different.

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Happy to rename it, but honestly I dont remember what the different responsibilities are. Its been so long since I wrote this I dont know how it works anymore. Any suggestions?

jimpo commented at 0:41 am on April 6, 2018:

Actually, it’s fine leaving this one as is because in the later commit (Process fee estimates in the background via MempoolInterface), the other removeTx gets renamed to removeTxNotInBlock.

Not really necessary, but maybe rename the other function to removeTxNotInBlock in this commit instead of a later commit to avoid the confusion here.

in src/policy/fees.cpp:592 in 68d7445e23 outdated

589     if (pos == mapMemPoolTxs.end()) {
590         // This transaction wasn't being tracked for fee estimation
591         return false;
592     }
593+    if (pos->second.witness_hash != tx->GetWitnessHash()) {
594+        // In the case that we saw a tx which had its witness malleated,

jimpo commented at 8:10 pm on April 2, 2018:

commit: Track witness hash malleation in fee estimator

Why don’t we want to record the blocks to confirm in this case?

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Because we don’t know how long its been around/it waited.

jimpo commented at 10:08 pm on April 6, 2018:

I don’t understand. We have the record of when the tx entered the mempool (albeit with different witness data). How does the malleated witness change things?

TheBlueMatt commented at 3:16 pm on April 7, 2018:

Because clearly there was a propagation issue. We dont know whether the lower-feerate version (or some other version) was the one most miners saw or the higher-feerate version. Better to just ignore it.

in src/test/policyestimator_tests.cpp:87 in 9266c936b7 outdated

83@@ -77,6 +84,13 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates)
84         block.clear();
85         // Check after just a few txs that combining buckets works as expected
86         if (blocknum == 3) {
87+            // Wait for fee estimator to catch up

jimpo commented at 8:22 pm on April 2, 2018:

commit: Process fee estimates in the background via MempoolInterface

Use SyncWithValidationInterfaceQueue?

TheBlueMatt commented at 6:02 pm on April 5, 2018:

This was written before that existed. Fixed.

in src/policy/fees.h:207 in 9266c936b7 outdated

213-    void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate);
214-
215-    /** Remove a transaction from the mempool tracking stats*/
216-    bool removeTx(uint256 hash);
217+    // MempoolInterface functions:
218+    void TransactionAddedToMempool(const NewMempoolTransactionInfo& info, const std::vector<CTransactionRef>& txn_replaced) override;

jimpo commented at 8:23 pm on April 2, 2018:

commit: Process fee estimates in the background via MempoolInterface

Make the callbacks protected?

TheBlueMatt commented at 6:02 pm on April 5, 2018:

Fixed.

jimpo commented at 8:30 pm on April 2, 2018: contributor

Concept ACK. Overall, this looks awesome and simplifies a lot of code. I’m all for decoupling stuff through the ValidationInterface.

I did find it hard to review because it is so large and would prefer it be broken up into fee estimate stuff and non-fee estimate specific refactors.

Also, there’s a typo in the commit title of “Make fee estimation not need CTxMemPoolEntry is block processing”

TheBlueMatt force-pushed on Apr 5, 2018

sdaftuar commented at 7:42 pm on April 10, 2018: member

I’ve started reviewing and testing the first few commits in this PR, but I already agree with @jimpo above, that this would be easier to review if it was split up.

Perhaps we could first have a PR that changed the callback interface around, and then another PR that reworks the fee estimator to take advantage of the new interface?

(also this needs rebase)

ajtowns commented at 2:50 am on April 12, 2018: contributor

ConceptACK for what it’s worth, but definitely split this up – it overflows my mental stack, and both times I’ve tried to review it it’s already had conflicts with master that make a review not crazy useful…

Breaking out the trivial bits, at least “Clarify validationinterface notification ordering” and “Remove useless scoped (sic?) in AcceptToMemoryPool” seems like a good first move. Maybe “Split removeRecursive” as well. Otherwise just splitting the remaining patches into mempool/validationinterface vs fee-estimation focused seems plausible.

jnewbery commented at 2:20 pm on April 12, 2018: member

I haven’t looked at the changes but a +700/-600 diff is somewhat intimidating.

I’d love to help you make progress on this and will happily review PRs that are a subset of this.

promag commented at 2:38 pm on April 12, 2018: member

Tested ACK.

I’m kidding :trollface: please please split up 👍

MarcoFalke commented at 3:16 pm on April 12, 2018: member

@bugthebluematt ^

TheBlueMatt force-pushed on Apr 13, 2018

TheBlueMatt commented at 6:59 pm on April 13, 2018: contributor

Now based on #12979.

TheBlueMatt force-pushed on Apr 17, 2018

Clarify validationinterface notification ordering bad68e9a50

Add a parallel validation interface for mempool events.

Because many listeners (eg wallet) will want both types of events
to be well-ordered, they are parallel and connected on the backend.
However, they are exposed separately to clients to separate the
logic (and because, hopefully, eventually, they can be exposed to
external clients of Bitcoin Core via libconsensus or similar).

ae5e07196c

Pass MemPoolRemovalReason out through TransactionRemovedFromMempool 07095f3b45

Split removeRecursive into calculate/remove steps ec0f81f6be

Track the set of transactions removed/conflicted in removeForBlock 1ea56d14ae

TheBlueMatt force-pushed on May 2, 2018

BlockConnected(vtxConflicted) -> New MempoolInterface callback

This removes the whole ConnectTrace object, which may make it
slightly harder to remove the unbounded-memory-during-reorg bug
by throwing blocks out of memory and re-loading them from disk
later. Comments are added to validationinterface to note where
this should likely happen instead of ConnectTrace.

9550c830d7

Remove boost::signals from txmempool, call GetMainSignals() directly 4da0342753

Remove useless scope in AcceptToMemoryPoolWorker 15205068e6

Add txn_replaced to Added callback, remove lReplaced ATMP arg 53b90df75c

Pass new block height through MempoolUpdatedForBlock 49f8f57a1c

Make fee estimation not need CTxMemPoolEntry is block processing

This makes the processBlock function in CBlockPolicyEstimator much
more simlar to MempoolInterface's MempoolUpdatedForBlockConnect
callback.

6864a1529c

Process fee estimates in the background via MempoolInterface a734d1d166

Handle reorgs much better in fee estimator.

There are lots of better things we can do, but this is much better
than it was.

660190c8f5

Track witness hash malleation in fee estimator 6697c02f7d

TheBlueMatt force-pushed on May 2, 2018

MarcoFalke added the label Needs rebase on Jun 6, 2018

TheBlueMatt closed this on Jun 14, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

bitcoin locked this on Dec 16, 2021

glozow added the label Up for grabs on Nov 18, 2022

fanquake removed the label Up for grabs on Aug 30, 2023

Move fee estimator into validationinterface/cscheduler thread #11775