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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
Concept ACK
Thanks for working on these circular dependencies!
198 | @@ -198,8 +199,10 @@ class CBlockPolicyEstimator 199 | /** Process a transaction accepted to the mempool*/ 200 | void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate); 201 | 202 | - /** Remove a transaction from the mempool tracking stats*/ 203 | - bool removeTx(uint256 hash, bool inBlock); 204 | + /** Remove a transaction from the mempool tracking stats 205 | + * Removal is not inBlock as in-block removals are handled internally. 206 | + */ 207 | + 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;
198 | @@ -198,8 +199,10 @@ class CBlockPolicyEstimator 199 | /** Process a transaction accepted to the mempool*/ 200 | void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate); 201 | 202 | - /** Remove a transaction from the mempool tracking stats*/ 203 | - bool removeTx(uint256 hash, bool inBlock); 204 | + /** 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);
Marked methods override and made the comments more appropriate
Made the removeTx return type void and added interface arg comments.
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.
Thanks for calling out the readme. Fixed, rebased, and squashed.
yw, nice refactor! utACK f58e17e
508 | @@ -509,6 +509,12 @@ void TxConfirmStats::removeTx(unsigned int entryHeight, unsigned int nBestSeenHe 509 | // tracked. Txs that were part of a block have already been removed in 510 | // processBlockTx to ensure they are never double tracked, but it is 511 | // of no harm to try to remove them again. 512 | +void CBlockPolicyEstimator::removeTx(uint256 hash)
hash should be passed by const reference?
Changed removeTx and processBlock to accept const hash references.
copy-paste from #15638 (review) 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:
fees and mempool files.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.
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.
This refactoring change is +80-17. Generally I'd prefer if refactoring simplified things, not made them harder
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.
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
<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase