Remove mempool global #19556

maflcko · 2020-07-19T16:20:05Z

This refactor unlocks some nice potential features, such as, but not limited to:

Removing the fee estimates global (would avoid slightly fragile workarounds such as Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766)
Making the mempool optional for a "blocksonly" operation mode

Even absent those features, the new code without the global should be easier to maintain, read and write tests for.

hebasto · 2020-07-19T16:25:54Z

Concept ACK.

jonatack · 2020-07-19T16:57:32Z

Concept/approach ACK.

JeremyRubin · 2020-07-19T17:24:11Z

Concept ACK on this PR.

lite approach NACK on "Making the mempool optional for a "blocksonly" operation mode", it should be in it's own flag as there are some other reasons to keep a mempool around even in blocksonly.

DrahtBot · 2020-07-20T00:02: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:

Avoid locking CTxMemPool::cs recursively in some cases #19872 (Avoid locking CTxMemPool::cs recursively in some cases by hebasto)
scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion #19865 (scripted-diff: Restore AssertLockHeld after Do not hide compile-time thread safety warnings #19668, remove LockAssertion by ryanofsky)
Remove mempool global from interfaces #19848 (Remove mempool global from interfaces by MarcoFalke)
[net processing] Move Misbehaving() to PeerManager #19791 ([net processing] Move Misbehaving() to PeerManager by jnewbery)
Switch BlockMap to use an unordered_set under the hood #19677 (Switch BlockMap to use an unordered_set under the hood by JeremyRubin)
ZMQ: Create "sequence" notifier, enabling client-side mempool tracking #19572 (ZMQ: Create "sequence" notifier, enabling client-side mempool tracking by instagibbs)
Wallet passive startup #15719 (Wallet passive startup by ryanofsky)
RPC: Add ancestor{count,size,fees} to listunspent output #12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)
Add fee_est tool for debugging fee estimation code #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.

naumenkogs · 2020-07-20T07:16:19Z

Concept ACK.

jnewbery · 2020-07-20T10:51:05Z

Concept ACK

ariard

Concept ACK

As it has been raised above you may want to keep an empty mempool to test correctness of your transactions wrt to policy (testmempoolaccept). If user is willingly to run in blocksonly, turning on another flag doesn't seem a blocker if you're also willingly to disable fee-estimation.

ariard · 2020-07-20T22:21:29Z

src/policy/rbf.cpp

@@ -18,16 +19,16 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)

    // If this transaction is not in our mempool, then we can't be sure
    // we will know about all its inputs.
-    if (!pool.exists(tx.GetHash())) {
+    if (!pool || !pool->exists(tx.GetHash())) {


In fact I think you can do better than returning an UNKNOWN state if you check that all parents are part of coin cache and transaction doesn't explicitly signal you can return a FINAL state ?

maflcko · 2020-07-21T07:34:20Z

... empty mempool to test correctness of your transactions wrt to policy (testmempoolaccept)

I don't modify testmempoolaccept behavior in this refactoring pull at all. If you think it should change, I suggest opening a separate behavior-changing pull reqeust. Though, with an empty mempool, a lot of the benefits of testmempoolaccept will be lost. So if the behavior is changed, I recommend that users will have to opt-in to the less-useful empty-mempool behavior.

In fact I think you can do better than returning an UNKNOWN state if you check that all parents are part of coin cache and transaction doesn't explicitly signal you can return a FINAL state ?

This function is only called by the wallet and I think it is not feasible to operate a wallet without a mempool, so accommodating that use case seems a bit overkill. Regardless, I suggest to handle this in a separate pull request from this refactor.

ariard · 2020-07-21T14:31:38Z

I don't modify testmempoolaccept behavior in this refactoring pull at all.

I was aware of this PR not changing behavior , my comment was more an opinion on whether or not we should tight blocksonly and withoutmempool features under same flag in future works.

jnewbery

Code review ACK faab597

src/init.cpp

src/validation.h

src/rpc/rawtransaction.cpp

hebasto

ACK faab597, tested on Linux Mint 20 (x86_64).

hebasto · 2020-07-22T03:39:21Z

src/policy/rbf.cpp

@@ -5,9 +5,10 @@
 #include <policy/rbf.h>
 #include <util/rbf.h>

-RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
+namespace {
+RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool* const pool) NO_THREAD_SAFETY_ANALYSIS


fad153a
Could this function differ from

bitcoin/src/policy/rbf.cpp

Line 42 in fad153a

RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) { return IsRBFOptIn(tx, &pool); }

by name, not only by signature?

why? And, any suggestions I could take?

fa0572d Pass mempool reference to chainstate constructor (MarcoFalke) Pull request description: Next step toward bitcoin#19556 Instead of relying on the mempool global, each chainstate is given a reference to a mempool to keep up to date with the tip (block connections, disconnections, reorgs, ...) ACKs for top commit: promag: Code review ACK fa0572d. darosior: ACK fa0572d hebasto: ACK fa0572d, reviewed and tested on Linux Mint 20 (x86_64). Tree-SHA512: 12184d33ae5797438d03efd012a07ba3e4ffa0d817c7a0877743f3d7a7656fe279280c751554fc035ccd0058166153b6c6c308a98b2d6b13998922617ad95c4c

fa9ee52 doc: Add doxygen comment to IsRBFOptIn (MarcoFalke) faef4fc Remove mempool global from interfaces (MarcoFalke) fa83168 refactor: Add IsRBFOptInEmptyMempool (MarcoFalke) Pull request description: The chain interface has an `m_node` member, which has a pointer to the mempool global. Use the pointer instead of the global to prepare the removal of the mempool global. See bitcoin#19556 ACKs for top commit: jnewbery: utACK fa9ee52 darosior: ACK fa9ee52 hebasto: re-ACK fa9ee52, since my [previous](bitcoin#19848 (review)) review: Tree-SHA512: 11b4c1446f0860a743fdaa67f95c52bf0262d0a4f888be0eaf07ee497448965d32be414111bf016bd568f2989cde923430e3a3889e224057b73c499f06de7199

Can be reviewed with the git diff options --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --ignore-all-space

maflcko · 2020-09-05T17:39:06Z

This is now ready for review

hebasto

ACK fafb381, I have reviewed the code and it looks OK, I agree it can be merged.

Hope these changes will make work on transit CTxMemPool::cs from RecursiveMutex to Mutex easier and more straightforward.

src/validation.cpp

darosior

ACK fafb381

jnewbery · 2020-09-06T20:40:26Z

utACK fafb381

jnewbery · 2020-09-06T20:45:10Z

src/init.cpp

@@ -1368,10 +1360,18 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
    node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
    assert(!node.connman);
    node.connman = MakeUnique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), args.GetBoolArg("-networkactive", true));
+
    // Make mempool generally available in the node context. For example the connection manager, wallet, or RPC threads,


This comment is outdated (the mempool is being constructed, not just made available). If you retouch the PR, can you update the comment?

Good point. The comment should probably be removed because now that it is no longer a global, it is clear from the code where it is passed in.

promag · 2020-09-07T07:56:53Z

Code review ACK fafb381.

fafb381 Remove mempool global (MarcoFalke) fa0359c Remove mempool global from p2p (MarcoFalke) eeee110 Remove mempool global from init (MarcoFalke) Pull request description: This refactor unlocks some nice potential features, such as, but not limited to: * Removing the fee estimates global (would avoid slightly fragile workarounds such as bitcoin#18766) * Making the mempool optional for a "blocksonly" operation mode Even absent those features, the new code without the global should be easier to maintain, read and write tests for. ACKs for top commit: jnewbery: utACK fafb381 hebasto: ACK fafb381, I have reviewed the code and it looks OK, I agree it can be merged. darosior: ACK fafb381 Tree-SHA512: a2e696dc377e2e81eaf9c389e6d13dde4a48d81f3538df88f4da502d3012dd61078495140ab5a5854f360a06249fe0e1f6a094c4e006d8b5cc2552a946becf26

Summary: PR description: Instead of relying on the implicit mempool global, pass a mempool pointer (which can be 0). This helps with testing, code clarity and unlocks the features described in [[bitcoin/bitcoin#19556 | #19556]] This is a backport of [[bitcoin/bitcoin#19604 | core#19604]] [2/3] bitcoin/bitcoin@fac674d Depends on D9756 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9757

Summary: > Next step toward [[bitcoin/bitcoin#19556 | core#19556]] > > Instead of relying on the mempool global, each chainstate is given a reference to a mempool to keep up to date with the tip (block connections, disconnections, reorgs, ...) This is a backport of [[bitcoin/bitcoin#19826 | core#19826]] Most differences from Core in validation.cpp are explained by D1667, D7203 & D7439. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9780

Summary: > GetTransaction implicitly and unconditionally asks the mempool global for a transaction. This is problematic for several reasons: > > - gettxoutproof is for on-chain txs only and asking the mempool for on-chain txs is confusing and minimally wasteful > - Globals are confusing and make code harder to test with unit tests > > Fix both issues by passing in an optional mempool. This also helps with [[bitcoin/bitcoin#19556 | core#19556]] This is a backport of [[bitcoin/bitcoin#19589 | core#19589]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D9783

Summary: > The chain interface has an m_node member, which has a pointer to the mempool global. Use the pointer instead of the global to prepare the removal of the mempool global. See [[bitcoin/bitcoin#19556 | core#19556]] This is a backport of [[bitcoin/bitcoin#19848 | core#19848]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9784

Summary: PR description: > This refactor unlocks some nice potential features, such as, but not limited to: > > - Removing the fee estimates global (would avoid slightly fragile workarounds such as > Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766) > - Making the mempool optional for a "blocksonly" operation mode > > Even absent those features, the new code without the global should be easier to maintain, read and write tests for. This is a backport of [[bitcoin/bitcoin#19556 | core#19556]] [1/3] bitcoin/bitcoin@eeee110 Depends on D9780, D9783 and D9784 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9788

Summary: PR description: > This refactor unlocks some nice potential features, such as, but not limited to: > > - Removing the fee estimates global (would avoid slightly fragile workarounds such as > Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766) > - Making the mempool optional for a "blocksonly" operation mode > > Even absent those features, the new code without the global should be easier to maintain, read and write tests for. This is a backport of [[bitcoin/bitcoin#19556 | core#19556]] [2/3] bitcoin/bitcoin@fa0359c Depends on D9788 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9789

Summary: PR description: > This refactor unlocks some nice potential features, such as, but not limited to: > > - Removing the fee estimates global (would avoid slightly fragile workarounds such as > Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766) > - Making the mempool optional for a "blocksonly" operation mode > > Even absent those features, the new code without the global should be easier to maintain, read and write tests for. This is a backport of [[bitcoin/bitcoin#19556 | core#19556]] [3/3] bitcoin/bitcoin@fafb381 Depends on D9789 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9790

maflcko added the Refactoring label Jul 19, 2020

maflcko force-pushed the Mf1808-txpoolGlobal branch from fa3a019 to faab597 Compare July 19, 2020 18:26

ariard reviewed Jul 20, 2020

View reviewed changes

jnewbery reviewed Jul 21, 2020

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

hebasto approved these changes Jul 22, 2020

View reviewed changes

DrahtBot mentioned this pull request Aug 31, 2020

Switch BlockMap to use an unordered_set under the hood #19677

Closed

This was referenced Sep 1, 2020

ZMQ: Create "sequence" notifier, enabling client-side mempool tracking #19572

Merged

scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion #19865

Closed

Avoid locking CTxMemPool::cs recursively in some cases #19872

Closed

MarcoFalke added 3 commits September 5, 2020 16:24

Remove mempool global from init

eeee110

Can be reviewed with the git diff options --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --ignore-all-space

Remove mempool global from p2p

fa0359c

Remove mempool global

fafb381

maflcko force-pushed the Mf1808-txpoolGlobal branch from a43bc0a to fafb381 Compare September 5, 2020 14:27

maflcko marked this pull request as ready for review September 5, 2020 14:28

hebasto approved these changes Sep 6, 2020

View reviewed changes

darosior reviewed Sep 6, 2020

View reviewed changes

src/validation.cpp Show resolved Hide resolved

src/validation.cpp Show resolved Hide resolved

darosior approved these changes Sep 6, 2020

View reviewed changes

jnewbery reviewed Sep 6, 2020

View reviewed changes

maflcko merged commit 0708705 into bitcoin:master Sep 7, 2020

maflcko deleted the Mf1808-txpoolGlobal branch September 7, 2020 08:07

jnewbery mentioned this pull request Apr 29, 2021

Mempool Update Cut-Through Optimization #21464

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove mempool global #19556

Remove mempool global #19556

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Remove mempool global #19556

Remove mempool global #19556

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!