cut the validation <-> txmempool circular dependency 2/2 #22677

Remove 2 circular dependencies: validation - txmempool and validation - policy/rbf - txmempool

Validation should depend on txmempool (e.g. CChainstateManager has a mempool and we often need to know what's in our mempool to validate transactions), but txmempool is a data structure that shouldn't really need to know about chain state.

• Changes removeForReorg() to be parameterized by a callable that returns true/false (i.e. whether the transaction should be removed due to being now immature or nonfinal) instead of a CChainState. The mempool really shouldn't need to know about coinbase maturity or lockpoints, it just needs to know which entries to remove.

Hmm I'm mildly concept NACK on this refactoring; removeForReorg really feels like internal mempool functionality.

Another tactic to remove the module links would be to make removeForReorg generic to any T ChainState class. Then you don't need to have the specifics of the implementation of T known within mempool.

(Circular deps within .cpp are not that bad IMO)

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22282 (refactor: CheckFinalTx pass by reference instead of pointer by klementtan)
• #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.

validation.cpp:338:25: error: calling function 'TestLockPointValidity' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
        bool validLP =  TestLockPointValidity(m_chain, &lp);
                        ^
validation.cpp:339:41: error: calling function 'CoinsTip' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
        CCoinsViewMemPool view_mempool(&CoinsTip(), pool);
                                        ^
validation.cpp:340:14: error: calling function 'CheckFinalTx' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
        if (!CheckFinalTx(m_chain.Tip(), tx, flags)
             ^
validation.cpp:350:36: error: calling function 'CoinsTip' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
                const Coin &coin = CoinsTip().AccessCoin(txin.prevout);
                                   ^
validation.cpp:5113:5: error: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::__1::recursive_mutex> >' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
    AssertLockHeld(::cs_main);
    ^
./sync.h:81:28: note: expanded from macro 'AssertLockHeld'
#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
                           ^
validation.cpp:5117:10: error: calling function 'check' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
    pool.check();
         ^
validation.cpp:5119:41: error: calling function 'CoinsTip' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
    CCoinsViewCache& active_coins_tip = CoinsTip();
                                        ^
7 errors generated.

removeForReorg really feels like internal mempool functionality. Another tactic to remove the module links would be to make removeForReorg generic to any T ChainState class. Then you don't need to have the specifics of the implementation of T known within mempool.

Yeah, I see this. Perhaps the function could instead be parametrized by tip, coins cache, etc?

I've pushed a version that splits out the finality checking part of removeForReorg (parts that require validation functions CheckSequenceLocks and CheckFinalTx) into validation, but leaves most of it in txmempool. Perhaps a better separation?

i don't think so :/

edit: it still feels like an abstraction layer leak, the for loop is inner functionality. Have you given parametrizing by CChainState. a try?

@JeremyRubin CChainState is what it's currently parametrized by - it's what causes the dependency on validation.h

Ok new approach. I added something similar to the CConnman.ForEachNode so we can pass in a lambda that captures what we need from chainstate. Validation isn't looking through mapTx anymore. The mempool gets a callable object and applies it to the entries internally.

i meant template paramterized by a generic ChainStateProvider type, that way it's clean depedency wise.. will check out the new approach later.

What's the latest on this PR? Am I right in thinking that an approach hasn't been settled on to avoid the circular dependency and that #22675 should be reviewed instead?

edit: I guess this is just a RFC and draft PR so it is here in case an alternative approach is sketched out.

Fixed the CI issue. Ready for review.

Concept ACK!

I've left a few comments inline. Another couple of comments on the git history:

Commit 3 ([mempool] split CTxMemPool::check into contextual and internal parts) claims to be no behaviour change, but the new internal check() method is not called by external check(CChainState& active_chainstate) method. That is restored in a later commit, but it should be included in commit 3 to make the commits hygenic (and the later commit MOVEONLY: contextual mempool check to validation would be more move-only)

It'd be nice if the MOVEONLY: contextual mempool check to validation commit had fewer non-move changes so that using git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space would obviously show that the code really is just moving. Are you able to rename variables inside check(), so that the changes are minimal in the move. Same for the MOVEONLY: filtering nonfinal/immature transactions to validation commit.

I've changed the circular dependency-breaking to be as minimal as possible. The check() function is now parametrized by a coins cache and spend height rather than chainstate. The removeForReorg() function now takes a callable object to apply to each mempool entry, removing the ones for which it returns true.

I've also written a bench for these 2 functions and taken the opportunity to speed check() up by eliminating the need to check mempool entries more than once. My bench results show a 35% speedup. Hopefully the performance improvement + circular dependency break is good enough to motivate these changes.

Bench at commit when the benchmark is first introduced:

Bench at HEAD of this PR:

Rebased, addressed @jnewbery's comment, and separated the benches because it's a bit misleading to group them.

Split the PR up - first half is in #23157.

Rebased

Code review ACK b5de6cc10bfbc8174936c3e8ae9418dedfb60000

Very nicely done as usual. I also did some light testing - built locally, unit tests pass, also ran the functional tests I added in #22867 and those still pass ✅

Code-review ACK b5de6cc10bfbc8174936c3e8ae9418dedfb60000

TIL that lambda captures can also be used to define new variables/constant, rather than just capturing existing ones (as done for flags).

Needs rebase?

Thanks @MarcoFalke! silent merge conflict with #23211. Need to rethink approach since it's no longer possible to modify lockpoints from outside txmempool...

That was a move-only, so can be reverted if needed. Maybe a struct update_lock_points; forward decl is enough?

That was a move-only, so can be reverted if needed. Maybe a struct update_lock_points; forward decl is enough?

Forward decl isn't enough because we need the constructor in order to call update_lock_points(lp).

Currently, I'm leaning towards splitting removeForReorg into 2 loops: CheckLockPoints (leave inside txmempool) and RemoveNonFinalAndPremature (move to validation). There shouldn't be a performance penalty.

Reverting is also an option. I'm currently trying to figure out whether that's better. AFAICT, the update_* structs help provide an interface to modifying mempool entries while keeping the txiter type const (txmempool passes txiters and setEntriess to outside callers pretty freely). But since all modifications to entries should be done through txmempool's public member functions, perhaps it's better for them to be private. Will investigate further.

Rebased. If you have time, please take another look @mjdietzx @theStack @MarcoFalke?

Code review ACK a64078e38563ef3ac5e5ec20c07569441c87eeee

re-ACK a64078e38563ef3ac5e5ec20c07569441c87eeee

reACK a64078e38563ef3ac5e5ec20c07569441c87eeee

left comments

This is not refactoring, but introducing a bug: #22677 (review)