Introduce MempoolObserver interface to break "policy/fees -> txmempool -> policy/fees" circular dependency #13949

Empact · 2018-08-13T01:09:11Z

A simple interface which make the implementations independent of one another.

Note I also removed the inBlock argument to the public CBlockPolicyEstimator::removeTx, as all block-related removal calls are internal to the class. This simplifies the extracted interface and reduces CBlockPolicyEstimator surface area.

DrahtBot · 2018-08-13T02:55:02Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16066 (mempool: Skip estimator if block is older than X by promag)
#15946 (Allow maintaining the blockfilterindex when using prune by jonasschnelli)
#14193 (validation: Add missing mempool locks by MarcoFalke)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

practicalswift · 2018-08-13T08:28:26Z

Concept ACK

Thanks for working on these circular dependencies!

l2a5b1

Concept ACK. Thanks @Empact!

l2a5b1 · 2018-08-14T09:37:38Z

src/policy/fees.h

+    /** Remove a transaction from the mempool tracking stats
+     * Removal is not inBlock as in-block removals are handled internally.
+     */
+    bool removeTx(uint256 hash);


You can mark the declarations of interfaces::MempoolObserver methods in CBlockPolicyEstimator with override:

void processBlock(unsigned int nBlockHeight, std::vector<const CTxMemPoolEntry*>& entries) override; void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate) override; bool removeTx(uint256 hash) override;

l2a5b1 · 2018-08-14T09:59:05Z

src/policy/fees.h

@@ -198,8 +199,10 @@ class CBlockPolicyEstimator
    /** Process a transaction accepted to the mempool*/
    void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate);

-    /** Remove a transaction from the mempool tracking stats*/
-    bool removeTx(uint256 hash, bool inBlock);
+    /** Remove a transaction from the mempool tracking stats


To prevent redundant documentation, maybe you can declare the interfaces::MempoolObserver methods in CBlockPolicyEstimator without copying the documentation from the method declarations in interfaces::MempoolObserver.

void processBlock(unsigned int nBlockHeight, std::vector<const CTxMemPoolEntry*>& entries); void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate); /** * Removal is not inBlock as in-block removals are handled internally. */ bool removeTx(uint256 hash);

Empact · 2018-08-14T22:08:10Z

Marked methods override and made the comments more appropriate

Empact · 2018-08-15T05:42:19Z

Made the removeTx return type void and added interface arg comments.

l2a5b1 · 2018-08-15T12:07:36Z

utACK 56ec018 when squashed.

One nit: I was wondering about the location of mempool_observer.h. If the file belongs in src/interfaces, where it currently resides, you may want to add an entry for the MempoolObserver interface to https://github.com/bitcoin/bitcoin/blob/master/src/interfaces/README.md, which contains a list of headers.

Empact · 2018-08-15T17:14:18Z

Thanks for calling out the readme. Fixed, rebased, and squashed.

l2a5b1 · 2018-08-15T18:21:09Z

yw, nice refactor! utACK f58e17e

practicalswift · 2018-09-05T07:12:19Z

src/policy/fees.cpp

@@ -509,6 +509,12 @@ void TxConfirmStats::removeTx(unsigned int entryHeight, unsigned int nBestSeenHe
 // tracked. Txs that were part of a block have already been removed in
 // processBlockTx to ensure they are never double tracked, but it is
 // of no harm to try to remove them again.
+void CBlockPolicyEstimator::removeTx(uint256 hash)


hash should be passed by const reference?

Empact · 2018-09-11T03:58:19Z

Changed removeTx and processBlock to accept const hash references.

maflcko · 2019-03-25T18:26:51Z

copy-paste from #15638 (comment) by @ryanofsky:

While I think in some cases fixing lint-circular-dependencies errors can make code organization better, it can just as easily make it worse.

An example would be the circular dependency between txmempool and policy/fees. The fee estimation code needs access to the mempool entry struct, and the mempool code needs to call simple fee estimator add/remove functions when mempool entries get added and removed.

The simple and obvious way to write this code is to declare the mempool entry struct in txmempool.h and the add/remove functions in fees.h and have a circular dependency between the two object files. As long as both object files are part of the same static library, this is perfectly fine and causes no problems. But if you want to appease the lint checker, you only have bad and mediocre options:

Combining files that shouldn't be combined: merging fees and mempool files.
Splitting and scattering code around unnecessarily: declaring mempool entry struct in a separate file from the mempool struct.
Introducing unnecessary complexity: Registering fee estimator add/remove functions with the mempool at runtime, affecting runtime performance, and potentially introducing initialization bugs.

It's possible changes like these would be good for other reasons, but in general I think cleaning up spew from lint-circular-dependencies is not a good reason to make a code change if you can't point to another actual reason for the change.

ryanofsky · 2019-03-25T19:08:45Z

re: #13949 (comment)

Wow, I didn't know about (or maybe forgot about?) this PR. Looking over this change, it may not be a bad idea if there are justifications for it other than fixing circular dependencies. In the pasted comment, I was actually complaining circular dependency errors in the context of #10443, not this PR.

maflcko · 2019-03-25T19:16:02Z

This refactoring change is +80-17. Generally I'd prefer if refactoring simplified things, not made them harder

Empact · 2019-04-03T04:33:57Z

Now +70 −16, with +43 coming from the interface header itself.

I would argue this does make things simpler, by making CTxMemPool independent of CBlockPolicyEstimator. Both objects are now simpler because they both relate only to an abstract interface, which means there are no demands placed on either class apart from the capabilities and limitations of the interface.

A practical benefit is that this makes CTxMemPool more flexible, for example it introduces the possibility of testing it indirectly via an observer object.

l2a5b1 · 2019-04-03T11:31:16Z

utACK 8501bdc. The introduction of the MempoolObserver interface seems an elegant way to break the circular dependency.

This is an abstract interface which presents removes the dependency between CBlockPolicyEstimator and CTxMemPool

DrahtBot · 2019-07-03T14:26:36Z

Needs rebase

Empact · 2020-02-20T19:41:37Z

Closing in favor of #17786

fanquake added Refactoring Mempool labels Aug 13, 2018

Empact force-pushed the mempool-observer branch 2 times, most recently from 45ef07c to 2a44b30 Compare August 13, 2018 01:50

This was referenced Aug 13, 2018

refactor: Removal of circular dependency between index/txindex, validation and index/base #13942

Closed

WIP: Transaction Pool Layer #13804

Closed

l2a5b1 reviewed Aug 14, 2018

View reviewed changes

Empact force-pushed the mempool-observer branch from 2a44b30 to 2ec0a07 Compare August 14, 2018 22:07

Empact force-pushed the mempool-observer branch from 2ec0a07 to 56ec018 Compare August 15, 2018 05:41

Empact force-pushed the mempool-observer branch 2 times, most recently from d8c390b to 7b31ea2 Compare August 15, 2018 17:14

Empact force-pushed the mempool-observer branch from 7b31ea2 to f58e17e Compare August 15, 2018 17:16

DrahtBot mentioned this pull request Aug 23, 2018

Utxoscriptindex #14035

Closed

practicalswift reviewed Sep 5, 2018

View reviewed changes

DrahtBot mentioned this pull request Sep 7, 2018

Remove 2nd mapTx lookup in CTxMemPool::removeForBlock #13189

Closed

Empact force-pushed the mempool-observer branch from f58e17e to e44968a Compare September 11, 2018 03:57

This was referenced Sep 15, 2018

Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations #14224

Closed

Log early messages with -printtoconsole #13088

Closed

Add fee_est tool for debugging fee estimation code #10443

Closed

DrahtBot mentioned this pull request Sep 27, 2018

Multiprocess bitcoin #10102

Draft

Empact force-pushed the mempool-observer branch 2 times, most recently from e87926c to 8501bdc Compare April 3, 2019 04:28

DrahtBot added the Needs rebase label Apr 10, 2019

Empact force-pushed the mempool-observer branch from 8501bdc to bf35f86 Compare June 5, 2019 12:00

DrahtBot added Needs rebase and removed Needs rebase labels Jun 5, 2019

Extract MempoolObserver interface

e3e9114

This is an abstract interface which presents removes the dependency between CBlockPolicyEstimator and CTxMemPool

Empact force-pushed the mempool-observer branch from bf35f86 to e3e9114 Compare June 7, 2019 13:37

DrahtBot removed the Needs rebase label Jun 7, 2019

DrahtBot added the Needs rebase label Jul 3, 2019

hebasto mentioned this pull request Dec 21, 2019

refactor: Nuke policy/fees->mempool circular dependencies #17786

Merged

Empact closed this Feb 20, 2020

Empact deleted the mempool-observer branch September 7, 2020 23:37

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Introduce MempoolObserver interface to break "policy/fees -> txmempool -> policy/fees" circular dependency #13949

Introduce MempoolObserver interface to break "policy/fees -> txmempool -> policy/fees" circular dependency #13949

Uh oh!

Conversation

Empact commented Aug 13, 2018

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

practicalswift commented Aug 13, 2018

Uh oh!

l2a5b1 left a comment

Choose a reason for hiding this comment

Uh oh!

l2a5b1 Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

l2a5b1 Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

Empact commented Aug 14, 2018

Uh oh!

Empact commented Aug 15, 2018

Uh oh!

l2a5b1 commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Aug 15, 2018

Uh oh!

l2a5b1 commented Aug 15, 2018

Uh oh!

practicalswift Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

Empact commented Sep 11, 2018

Uh oh!

maflcko commented Mar 25, 2019

Uh oh!

ryanofsky commented Mar 25, 2019

Uh oh!

maflcko commented Mar 25, 2019

Uh oh!

Empact commented Apr 3, 2019

Uh oh!

l2a5b1 commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 3, 2019

Uh oh!

Empact commented Feb 20, 2020

Uh oh!

Uh oh!

DrahtBot commented Aug 13, 2018 •

edited

Loading

l2a5b1 commented Aug 15, 2018 •

edited

Loading

l2a5b1 commented Apr 3, 2019 •

edited

Loading