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

glozow · 2021-08-10T16:30:33Z

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.

JeremyRubin · 2021-08-10T17:48:48Z

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)

DrahtBot · 2021-08-10T18:51:03Z

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

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.

fanquake · 2021-08-11T02:44:17Z

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.

glozow · 2021-08-11T10:06:39Z

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?

glozow · 2021-08-18T20:34:28Z

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?

JeremyRubin · 2021-08-18T21:30:28Z

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?

glozow · 2021-08-19T08:43:18Z

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

glozow · 2021-08-19T14:01:00Z

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.

JeremyRubin · 2021-08-19T21:24:32Z

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

A circular dependency is added because policy now depends on txmempool and txmempool depends on validation. It is natural for [mempool] policy to rely on mempool; the problem is caused by txmempool depending on validation. bitcoin#22677 will resolve this.

f293c68 MOVEONLY: getting mempool conflicts to policy/rbf (glozow) 8d71796 [validation] quit RBF logic earlier and separate loops (glozow) badb9b1 call SignalsOptInRBF instead of checking all inputs (glozow) e0df41d [validation] default conflicting fees and size to 0 (glozow) b001b9f MOVEONLY: BIP125 max conflicts limit to policy/rbf.h (glozow) Pull request description: See #22675 for motivation, this is one chunk of it. It extracts some BIP125 logic into policy/rbf: - Defines a constant for specifying the maximum number of mempool entries we'd consider replacing by RBF - Calls the available `SignalsOptInRBF` function instead of manually iterating through inputs - Moves the logic for getting the list of conflicting mempool entries to a helper function - Also does a bit of preparation for future moves - moving declarations around, etc Also see #22677 for addressing the circular dependency. ACKs for top commit: jnewbery: Code review ACK f293c68 theStack: Code-review ACK f293c68 📔 ariard: ACK f293c68 Tree-SHA512: a60370994569cfc91d4b2ad5e94542d4855a48927ae8b174880216074e4fa50d4523dd4ee36efdd6edf2bf7adb87a8beff9c3aaaf6dd323b286b287233e63790

michaelfolkson · 2021-09-02T16:05:58Z

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.

A circular dependency is added because policy now depends on txmempool and txmempool depends on validation. It is natural for [mempool] policy to rely on mempool; the problem is caused by txmempool depending on validation. bitcoin#22677 will resolve this.

No behavior change. Parameterize removeForReorg using a CChain and callable that encapsulates validation logic. The mempool shouldn't need to know a bunch of details about coinbase maturity and lock finality. Instead, just pass in a callable function that says true/false. Breaks circular dependency by removing txmempool's dependency on validation.

glozow · 2021-11-30T12:30:59Z

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

laanwj · 2021-11-30T17:52:13Z

Code review ACK a64078e

theStack

re-ACK a64078e

theStack · 2021-12-01T10:27:25Z

src/txmempool.h

@@ -14,6 +14,7 @@
 #include <utility>
 #include <vector>

+#include <chain.h>


nit: could use the forward-declaration class CChain; instead to reduce header dependencies

taken in #23649

mjdietzx · 2021-12-01T14:53:58Z

reACK a64078e

maflcko

left comments

src/txmempool.cpp

src/validation.cpp

…ency 2/2

ddd74ff clean up txmempool includes (glozow) c4efc4d change TestLockPointValidity to take a const reference (glozow) b01784f remove unnecessary casts and use braced initialization (glozow) Pull request description: Followups from #22677 + clean up `TestLockPointValidity` ACKs for top commit: theStack: Code-review ACK ddd74ff Tree-SHA512: 0f7f26535b7301e2fb379e676310bdc7cfb2c5e232a6657f41dc6d3bc91583ec452eb2359ad2f2416ea12dd856f7fab3fa507a391ccf80f14de96da989281d96

maflcko · 2021-12-03T15:40:24Z

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

faf2614 style: Use 4 spaces for indendation, not 5 (MarcoFalke) fada66f Disallow copies of CChain (MarcoFalke) Pull request description: Creating a copy of the chain is not a valid use case in normal operation. Also, it massively degrades performance. However, it seems to be a mistake that no one looks out for during review: * #22677 (comment) Fix this by disallowing it. ACKs for top commit: jamesob: ACK faf2614 ([`jamesob/ackr/23769.1.MarcoFalke.disallow_copies_of_cchai`](https://github.com/jamesob/bitcoin/tree/ackr/23769.1.MarcoFalke.disallow_copies_of_cchai)) glozow: utACK faf2614, nice. prusnak: utACK faf2614 Tree-SHA512: 27b908c78842e4700e118adb876c09c3d1ec04662310e983309e2cd6fa8ad38c9359ff45f36a804359b9f117e351c4739e651b3e6754c14e6c6fcd7ae5e68342

…r dependency 2/2 3a20371 Break validation <-> txmempool circular dependency (glozow) 3582864 [mempool] always assert coin spent (glozow) 20498d0 [refactor] put finality and maturity checking into a lambda (glozow) 9ca73bf [mempool] only update lockpoints for non-removed entries (glozow) 0af46ea MOVEONLY: TestLockPointValidity to txmempool (glozow) Pull request description: 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. ACKs for top commit: laanwj: Code review ACK 3a20371 mjdietzx: reACK 3a20371 theStack: re-ACK 3a20371 Tree-SHA512: f75995200569c09dfb8ddc09729da66ddb32167ff1e8a7e72f105ec062d2d6a9a390e6b4a2a115e7ad8ad3525f891ee1503f3cd2bed11773abcaf7c3230b1136

…reorg b4adc5a [bugfix] update lockpoints correctly during reorg (glozow) b6002b0 MOVEONLY: update_lock_points to txmempool.h (glozow) Pull request description: I introduced a bug in bitcoin#22677 (sorry! 😅) Mempool entries cache `LockPoints`, containing the first height/blockhash/`CBlockIndex*` at which the transaction becomes valid. During a reorg, we re-check timelocks on all mempool entries using `CheckSequenceLocks(useExistingLockPoints=false)` and remove any now-invalid entries. `CheckSequenceLocks()` also mutates the `LockPoints` passed in, and we update valid entries' `LockPoints` using `update_lock_points`. Thus, `update_lock_points(lp)` needs to be called right after `CheckSequenceLocks(lp)`, otherwise we lose the data in `lp`. I incorrectly assumed they could be called in separate loops. The incorrect behavior introduced is: if we have a reorg in which a timelocked mempool transaction is still valid but becomes valid at a different block, the cached `LockPoints` will be incorrect. This PR fixes the bug, adds a test, and adds an assertion at the end of `removeForReorg()` to check that all mempool entries' lockpoints are valid. You can reproduce the bug by running the test added in the [test] commit on the code before the [bugfix] commit. ACKs for top commit: jnewbery: ACK b4adc5a vasild: ACK b4adc5a mzumsande: Code Review ACK b4adc5a hebasto: ACK b4adc5a MarcoFalke: re-ACK b4adc5a 🏁 Tree-SHA512: 16b59f6ff8140d0229079ca1c6b04f2f4a00a2e49931275150e4f3fe5ac4ec109698b083fa6b223ba9511f328271cc1ab081263669d5da020af7fee83c13e401

Summary: This is a partial backport of [[bitcoin/bitcoin#22677 | core#22677]] and [[bitcoin/bitcoin#23649 | core#23649]] bitcoin/bitcoin@1b3a11e bitcoin/bitcoin@ddd74ff Depends on D12442 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12440

Summary: This is a partial backport of [[bitcoin/bitcoin#22677 | core#22677]], [[bitcoin/bitcoin#23649 | core#23649]] and [[bitcoin/bitcoin#23683 | core#23683]] bitcoin/bitcoin@bedf246 bitcoin/bitcoin@b01784f bitcoin/bitcoin@b4adc5a Note that [[bitcoin/bitcoin#23683 | core#23683]] mostly reverts the commit from [[bitcoin/bitcoin#22677 | core#22677]]. The remaining diff after applying both commits consists of just an additional assertion and comment. Also note that core#23683 was applied out of sequence to avoid introducing a bug. In the source material, the assertion is added to the code after a piece of it was moved to validation.cpp. Depends on D12440 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12441

Summary: No behavior change. This is a partial backport of [[bitcoin/bitcoin#22677 | core#22677]] bitcoin/bitcoin@bb9078e Depends on D12441 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12443

Summary: This is an extremely cheap function (just checks that the coin CTxOut isn't null) that doesn't need to be gated on check_ratio. Run coin.IsSpent only once in a row This is a partial backport of [[bitcoin/bitcoin#22677 | core#22677]] and [[bitcoin/bitcoin#24102 | core#24102]] bitcoin/bitcoin@64e4963 bitcoin/bitcoin@fa2bcc4 Depends on D12443 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12446

… DisconnectedBlockTransactions::updateMempoolForReorg Summary: Original commit: > Break validation <-> txmempool circular dependency > > No behavior change. > > Parameterize removeForReorg using a CChain and callable that > encapsulates validation logic. The mempool shouldn't need to know a > bunch of details about coinbase maturity and lock finality. Instead, > just pass in a callable function that says true/false. Breaks circular > dependency by removing txmempool's dependency on validation. We cannot break the circular dependency because `updateMempoolForReorg` is not a `CChainstate` member function in Bitcoin ABC, but a member of a different class that depends both on `validation` and `txmempool` for now. See D1667. Breaking the circular dependency is not a good enough reason to undo the change from D1667, because it would mean exposing the `DisconnectedBlockTransactions` internals to validation. This concludes backport of [[bitcoin/bitcoin#22677 | core#22677]] bitcoin/bitcoin@a64078e Depends on D12446 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12448

glozow mentioned this pull request Aug 10, 2021

RBF move 2/3: extract RBF logic into policy/rbf #22675

Merged

DrahtBot added Mempool P2P Validation labels Aug 10, 2021

This was referenced Aug 10, 2021

refactor: CheckFinalTx pass by reference instead of pointer #22282

Closed

net_processing: lock clean up #21527

Closed

rpc, test: Improve getblockstats for unspendables #19888

Merged

DrahtBot mentioned this pull request Aug 11, 2021

assumeutxo #15606

Closed

18 tasks

glozow force-pushed the 2021-08-circular-dep branch from ccde0ad to 41485f6 Compare August 18, 2021 14:14

glozow force-pushed the 2021-08-circular-dep branch from 41485f6 to bfea36f Compare August 19, 2021 13:56

glozow force-pushed the 2021-08-circular-dep branch from bfea36f to 6c6f52a Compare August 19, 2021 14:12

This was referenced Aug 19, 2021

Implement RBF inherited signaling and fix getmempoolentry returned bip125-replaceable status #22698

Closed

policy/rbf: don't return "incorrect" replaceability status #22665

Closed

Package Mempool Submission with Package Fee-Bumping #22290

Closed

glozow mentioned this pull request Aug 25, 2021

RBF move (1/3): extract BIP125 Rule 5 into policy/rbf #22796

Merged

glozow force-pushed the 2021-08-circular-dep branch from 6c6f52a to 6dce1fa Compare September 10, 2021 09:30

glozow force-pushed the 2021-08-circular-dep branch from b5de6cc to a64078e Compare November 30, 2021 12:26

theStack approved these changes Dec 1, 2021

View reviewed changes

maflcko merged commit 4633199 into bitcoin:master Dec 1, 2021

maflcko reviewed Dec 1, 2021

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

src/txmempool.cpp Show resolved Hide resolved

src/txmempool.cpp Outdated Show resolved Hide resolved

src/validation.cpp Show resolved Hide resolved

src/validation.cpp Show resolved Hide resolved

mjdietzx mentioned this pull request Dec 1, 2021

test: Extend test coverage of BIP125 and document confusing/inconsistent behavior #22867

Closed

glozow deleted the 2021-08-circular-dep branch December 2, 2021 11:44

glozow mentioned this pull request Dec 2, 2021

circular dependency followups #23649

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 2, 2021

Merge bitcoin#22677: cut the validation <-> txmempool circular depend…

5409965

…ency 2/2

maflcko added Bug and removed Refactoring labels Dec 3, 2021

glozow mentioned this pull request Dec 6, 2021

bug fix: valid but different LockPoints after a reorg #23683

Merged

maflcko mentioned this pull request Dec 14, 2021

Disallow copies of CChain #23769

Merged

hebasto mentioned this pull request Dec 29, 2021

refactor: Move calculation logic out from CheckSequenceLocksAtTip() #23897

Merged

glozow mentioned this pull request Jul 11, 2022

Mempool + Validation Code Organization #25584

Closed

bitcoin locked and limited conversation to collaborators Dec 29, 2022

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

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

Uh oh!

Conversation

glozow commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin commented Aug 10, 2021

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

fanquake commented Aug 11, 2021

Uh oh!

glozow commented Aug 11, 2021

Uh oh!

glozow commented Aug 18, 2021

Uh oh!

JeremyRubin commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Aug 19, 2021

Uh oh!

glozow commented Aug 19, 2021

Uh oh!

JeremyRubin commented Aug 19, 2021

Uh oh!

michaelfolkson commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Nov 30, 2021

Uh oh!

laanwj commented Nov 30, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

theStack Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

glozow Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

mjdietzx commented Dec 1, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Dec 3, 2021

Uh oh!

Uh oh!

glozow commented Aug 10, 2021 •

edited

Loading

DrahtBot commented Aug 10, 2021 •

edited

Loading

JeremyRubin commented Aug 18, 2021 •

edited

Loading

michaelfolkson commented Sep 2, 2021 •

edited

Loading