Fire TransactionRemovedFromMempool callbacks from mempool

l2a5b1 commented at 8:43 pm on October 3, 2018: contributor

This pull request fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code.

It also resolves the txmempool -> validation -> validationinterface -> txmempool circular dependency.

Ideally, validationinterface is a dumb component that doesn’t have any knowledge of the sub-systems it sends its notifications to. The commit that aims to resolve this circular dependency by moving txmempool specific code out of validationinterface to txmempool where it belongs.

l2a5b1 force-pushed on Oct 3, 2018

DrahtBot commented at 10:11 pm on October 3, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16688 (log: Add validation interface logging by jkczyz)

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.

fanquake added the label Refactoring on Oct 3, 2018

l2a5b1 force-pushed on Oct 4, 2018

in src/txmempool.cpp:341 in a60a187158 outdated

344+}
345+
346+void CTxMemPool::MempoolEntryRemoved(CTransactionRef ptx, MemPoolRemovalReason reason)
347+{
348+    if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) {
349+        GetMainSignals().TransactionRemovedFromMempool(ptx);

practicalswift commented at 9:35 am on October 4, 2018:

Could ptx be moved to avoid unnecessary copies?

l2a5b1 commented at 1:15 pm on April 11, 2019:

Thanks @practicalswift. Good point. When ptx was passed by value I suspect we could have done that. However, per your suggestion #14384 (review) ptx is now passed by reference.

in src/validationinterface.h:176 in a60a187158 outdated

175-    /** Unregister with mempool */
176-    void UnregisterWithMempoolSignals(CTxMemPool& pool);
177 
178     void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload);
179     void TransactionAddedToMempool(const CTransactionRef &);
180+    void TransactionRemovedFromMempool(const CTransactionRef tx);

practicalswift commented at 9:36 am on October 4, 2018:

Should be named ptx to match definition :-)

l2a5b1 commented at 8:10 pm on October 4, 2018:

Thanks @practicalswift fixed!

in src/validationinterface.cpp:134 in a60a187158 outdated

130@@ -148,6 +131,12 @@ void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) {
131     });
132 }
133 
134+void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef ptx) {

practicalswift commented at 9:36 am on October 4, 2018:

Could be made const reference :-)

practicalswift commented at 9:37 am on October 4, 2018: contributor

Concept ACK

Let’s get rid of these ugly circular dependencies! :-)

l2a5b1 force-pushed on Oct 4, 2018

DrahtBot added the label Needs rebase on Nov 5, 2018

l2a5b1 force-pushed on Nov 5, 2018

DrahtBot removed the label Needs rebase on Nov 5, 2018

l2a5b1 force-pushed on Nov 5, 2018

DrahtBot added the label Needs rebase on Nov 9, 2018

l2a5b1 force-pushed on Nov 13, 2018

DrahtBot removed the label Needs rebase on Nov 13, 2018

l2a5b1 force-pushed on Nov 13, 2018

l2a5b1 commented at 10:26 pm on November 13, 2018: contributor

Rebased b58bbc9

DrahtBot added the label Needs rebase on Dec 29, 2018

l2a5b1 force-pushed on Apr 10, 2019

DrahtBot removed the label Needs rebase on Apr 10, 2019

l2a5b1 commented at 10:26 am on April 10, 2019: contributor

rebased fe6d863

thanks @practicalswift for the review - fe6d863 addresses your feedback.

To answer your question #14384 (review): I don’t think it is necessary to move the shared pointer as it is now passed by reference as per your suggestion: #14384 (review) .

DrahtBot added the label Needs rebase on Jun 6, 2019

l2a5b1 force-pushed on Jun 6, 2019

DrahtBot removed the label Needs rebase on Jun 6, 2019

l2a5b1 commented at 7:44 pm on June 6, 2019: contributor

Rebased 35a63b2.

Updated the pull request description, since the validation -> validationinterface -> validation circular dependency was resolved in #16129 and removed from this pull request.

Travis build error seems unrelated.

DrahtBot added the label Needs rebase on Jun 21, 2019

l2a5b1 force-pushed on Jun 23, 2019

DrahtBot removed the label Needs rebase on Jun 23, 2019

l2a5b1 commented at 11:39 am on June 23, 2019: contributor

Rebased 80c3659

in src/txmempool.cpp:24 in 80c365973f outdated

15@@ -16,6 +16,12 @@
16 #include <util/system.h>
17 #include <util/moneystr.h>
18 #include <util/time.h>
19+#include <validationinterface.h>
20+
21+// This map has to a separate global instead of a member of MainSignalsInstance,
22+// because RegisterWithMempoolSignals is currently called before RegisterBackgroundSignalScheduler,
23+// so MainSignalsInstance hasn't been created yet.
24+static std::unordered_map<CTxMemPool*, boost::signals2::scoped_connection> g_connNotifyEntryRemoved;

MarcoFalke commented at 0:12 am on July 23, 2019:

Why is this not a CTxMemPool member?

MarcoFalke commented at 1:17 pm on July 23, 2019:

Actually, this can be removed in one go:

07c8420f4ddddde7103d8a564f918389357904c9

l2a5b1 commented at 6:42 pm on July 23, 2019:

Thanks @MarcoFalke! Really appreciate your feedback. Please see 04390b2.

l2a5b1 commented at 6:55 pm on July 23, 2019:

nit: Does it makes sense to remove CTxMemPool::EntryRemoved’s method and move its implementation to CTxMemPool::removeUnchecked ?

0void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
1{
2    NotifyEntryRemoved(ptx, reason);
3    if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) {
4        GetMainSignals().TransactionRemovedFromMempool(ptx);
5    }
6    ...
7}

MarcoFalke commented at 7:34 pm on July 23, 2019:

nit: Does it makes sense to remove CTxMemPool::EntryRemoved’s method and move its implementation to CTxMemPool::removeUnchecked ?

Sure, if you wish so

in src/txmempool.cpp:342 in 80c365973f outdated

338@@ -333,6 +339,24 @@ CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator) :
339     nCheckFrequency = 0;
340 }
341 
342+void CTxMemPool::RegisterSignals() {

MarcoFalke commented at 0:12 am on July 23, 2019:

Why can’t this be called in the mempool constructor?

MarcoFalke deleted a comment on Jul 23, 2019

l2a5b1 force-pushed on Jul 23, 2019

l2a5b1 commented at 6:42 pm on July 23, 2019: contributor

04390b2 addresses feedback from @MarcoFalke.

MarcoFalke renamed this:
~~Resolve validationinterface circular dependencies~~
Fire TransactionRemovedFromMempool callbacks from mempool, remove RegisterWithMempoolSignals
on Jul 23, 2019

MarcoFalke renamed this:
~~Fire TransactionRemovedFromMempool callbacks from mempool, remove RegisterWithMempoolSignals~~
Fire TransactionRemovedFromMempool callbacks from mempool
on Jul 23, 2019

MarcoFalke commented at 7:36 pm on July 23, 2019: member

Concept ACK.

TransactionRemovedFromMempool is used to notify of removed tx, excluding txs that were already notified via BlockConnected. However, we will miss those notifications, if we forgot to call RegisterWithMempoolSignals. This happened in the unit tests (setup_common).

This pull fixes that and cleans up a bunch of code on the way.

l2a5b1 force-pushed on Jul 23, 2019

l2a5b1 commented at 9:52 pm on July 23, 2019: contributor

28403ac addresses a nit (https://github.com/bitcoin/bitcoin/pull/14384#discussion_r306480219) and updates the commit message.

l2a5b1 commented at 1:13 pm on October 3, 2019: contributor

Happy 1y anniversary! 🥳🎂

DrahtBot added the label Needs rebase on Oct 29, 2019

MarcoFalke commented at 5:12 pm on November 8, 2019: member

Can we get this for 0.20, pls? Needs rebase

l2a5b1 force-pushed on Nov 8, 2019

DrahtBot removed the label Needs rebase on Nov 8, 2019

l2a5b1 commented at 8:01 pm on November 8, 2019: contributor

@MarcoFalke sure, rebased 37fd92ba96224cfe11bacea069df02cead0c34e2

MarcoFalke commented at 8:17 pm on November 8, 2019: member

ACK 37fd92ba96224cfe11bacea069df02cead0c34e2

0xB10C commented at 9:29 am on November 10, 2019: member

Concept ACK.

jnewbery commented at 6:38 pm on November 10, 2019: member

Concept ACK. I’ve skimmed the code and it looks good. Will fully review and test next week.

I think a follow-up to this would be to remove the pvtxConflicted vector in BlockConnected and instead just call TransactionRemovedFromMempool() for conflicted transactions directly from removeUnchecked(). I don’t see anywhere in the client code that requires the conflicted txs to be reported in the same notification as the block.

Doing that would allow us to remove the NotifyEntryRemoved signal from CTxMempool, the conflictedTxs from the ConnectTrace class, and some other code clean-up in validation.cpp.

in src/validationinterface.cpp:130 in 37fd92ba96 outdated

126@@ -150,6 +127,12 @@ void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) {
127     });
128 }
129 
130+void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx) {

jnewbery commented at 0:02 am on November 11, 2019:

Can you keep the MemPoolRemovealReason reason as an argument to this function please? #16688 adds validation interface logging, including logging the reason for the mempool removal.

I also think it makes sense for the client to be notified of the reason for the mempool reason.

jnewbery commented at 5:14 pm on November 11, 2019:

Can be done in a follow-up, or in #16688 when this gets merged.

jnewbery commented at 5:13 pm on November 11, 2019: member

utACK 37fd92ba96224cfe11bacea069df02cead0c34e2

re #14384 (review): On second thoughts, there’s no need to change TransationRemovedFromMempool() to take a MemPoolRemovalReason as an argument. If this PR gets merged before #16688 , then that PR should just add the argument there.

I think a follow-up to this would be to remove the pvtxConflicted vector in BlockConnected

I have a branch that does that here: https://github.com/jnewbery/bitcoin/tree/2019-11-remove-mempool-signals2. Will PR once this is merged.

l2a5b1 commented at 7:44 pm on November 11, 2019: contributor

@jnewbery thanks for the review! 😃

ariard approved

ariard commented at 7:48 pm on November 14, 2019: member

Tested ACK 37fd92b

jamesob commented at 6:27 pm on November 18, 2019: member

ACK https://github.com/bitcoin/bitcoin/pull/14384/commits/37fd92ba96224cfe11bacea069df02cead0c34e2

This is basically some helpful code-shuffling; the change removes unneeded code to absorb the TransactionRemovedFromMempool signal into the existing CValidationInterface callback registration code, and avoids including mempool-specific header files into the more general validationinterface.

jonatack commented at 6:43 pm on November 18, 2019: member

Concept ACK, kudos @l2a5b1 for staying with this.

Built 37fd92ba96224cfe11bacea069df02cead0c34e2 rebased onto current master at 30521302f90e4856a7516867b32a4576fa6d98b3, tests pass, bitcoind running.

Now reviewing the code.

jonatack commented at 7:31 pm on November 18, 2019: member

ACK 37fd92ba96224cfe11bacea069df02cead0c34e2

Nice cleanup that improves separation of concerns and enables removing MempoolEntryRemoved, RegisterWithMempoolSignals, and UnregisterWithMempoolSignals.

TheBlueMatt commented at 8:31 pm on November 18, 2019: member

This isn’t strictly worse, but its still super awkward….CTxMemPool is (mostly) a “dumb datastructure”, with all the adding/removing happening in validation.cpp. Currently its double awkward cause they’re signals that go back to validation so that validation can fire appropriate events, but just firing the events inside a datastructure is also really awkward. Is there an equally-easy way to move the event-firing back into validation.cpp?

jnewbery commented at 9:14 pm on November 18, 2019: member

Is there an equally-easy way to move the event-firing back into validation.cpp?

I don’t think there is. That would require that every site that validation calls a CTxMemPool method that could result in transactions being removed would need to be passed back a vector of transactions that it should fire the events for. A quick search shows at least the following sites would need to be updated:

validation’s UpdateMempoolForReorg() calling into mempool.removeRecursive()
validation’s LimitMempoolSize() calling into pool.Expire()
validation’s LimitMempoolSize() calling into pool.TrimToSize()

There are probably more. I don’t think those call sites have ever handled firing the events, and adding that functionality would be new data structures, function signatures and logic.

This seems to be a strict improvement to me. The events are being fired from exactly the same place as before, except that there’s no weird indirection in the validation interface.

laanwj added this to the "Blockers" column in a project

MarcoFalke commented at 7:30 pm on November 19, 2019: member

Re-run ci

MarcoFalke closed this on Nov 19, 2019

MarcoFalke reopened this on Nov 19, 2019

TheBlueMatt commented at 2:46 am on November 21, 2019: member

Right, certainly don’t interpret my comment as a NACK, then. Longer-term it would be nice, indeed, to move the event-firing into those callsites, cause the responsibility right now is very awkwardly split (hence why we’re checking the reason in the first place - validation fires some similar things), but this isn’t worse than master and if other PR’s need this, do it!

laanwj commented at 6:42 pm on November 21, 2019: member

argh, needs trivial rebase for the linter test/lint/lint-circular-dependencies.sh

DrahtBot added the label Needs rebase on Nov 21, 2019

Fire TransactionRemovedFromMempool from mempool

This commit fires TransactionRemovedFromMempool callbacks from the
mempool and cleans up a bunch of code.

e20c72f9f0

l2a5b1 force-pushed on Nov 21, 2019

DrahtBot removed the label Needs rebase on Nov 21, 2019

l2a5b1 commented at 6:43 am on November 22, 2019: contributor

argh @laanwj lol, no worries, rebased e20c72f9f076681def325b5b5fa53bccda2b0eab

jnewbery commented at 2:59 pm on November 22, 2019: member

ACK e20c72f9f076681def325b5b5fa53bccda2b0eab

laanwj referenced this in commit bb862d7864 on Nov 22, 2019

laanwj merged this on Nov 22, 2019

laanwj closed this on Nov 22, 2019

ariard commented at 4:16 pm on November 22, 2019: member

ACK e20c72f, only rebased.

jonatack commented at 5:13 pm on November 22, 2019: member

Posthumous ACK e20c72f9f076681def325b5b5fa53bccda2b0eab

sidhujag referenced this in commit 25b2afe377 on Nov 22, 2019

jkczyz referenced this in commit 34d6486de3 on Nov 22, 2019

meshcollider removed this from the "Blockers" column in a project

deadalnix referenced this in commit d66d5d7efd on Jun 8, 2020

sidhujag referenced this in commit 59b9555630 on Nov 10, 2020

random-zebra referenced this in commit e0350eda5b on Apr 14, 2021

DrahtBot locked this on Dec 16, 2021

Fire TransactionRemovedFromMempool callbacks from mempool #14384

Conflicts