This refactor unlocks some nice potential features, such as, but not limited to:
Even absent those features, the new code without the global should be easier to maintain, read and write tests for.
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
18@@ -18,16 +19,16 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
19
20 // If this transaction is not in our mempool, then we can't be sure
21 // we will know about all its inputs.
22- if (!pool.exists(tx.GetHash())) {
23+ if (!pool || !pool->exists(tx.GetHash())) {
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.
… 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.
679@@ -680,7 +680,7 @@ static void CleanupBlockRevFiles()
680 }
681 }
682
683-static void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFiles)
684+static void ThreadImport(ChainstateManager& chainman, CTxMemPool& mempool, std::vector<fs::path> vImportFiles)
fa571a9d36fe6f54ff15e5bc9b14df6317fdf53d
ATM the caller has the mempool so this looks fine to me.
164+void UnloadBlockIndex(CTxMemPool& mempool);
165 /** Run an instance of the script checking thread */
166 void ThreadScriptCheck(int worker_num);
167 /** Retrieve a transaction (from memory pool, or from disk, if possible) */
168-bool GetTransaction(const uint256& hash, CTransactionRef& tx, const Consensus::Params& params, uint256& hashBlock, const CBlockIndex* const blockIndex = nullptr);
169+bool GetTransaction(const CTxMemPool& mempool, const uint256& hash, CTransactionRef& tx, const Consensus::Params& params, uint256& hashBlock, const CBlockIndex* const blockIndex = nullptr);
239@@ -238,6 +240,8 @@ static UniValue gettxoutproof(const JSONRPCRequest& request)
240 RPCExamples{""},
241 }.Check(request);
242
243+ const CTxMemPool& mempool = EnsureMemPool(request.context);
GetTransaction() interface is cleaned up, or when mempool becomes optional? Would it be worth having a comment here for now?
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.
4@@ -5,9 +5,10 @@
5 #include <policy/rbf.h>
6 #include <util/rbf.h>
7
8-RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
9+namespace {
10+RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool* const pool) NO_THREAD_SAFETY_ANALYSIS
Can be reviewed with the git diff options
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --ignore-all-space
ACK fafb381af8279b2d2ca768df0bf68d7eb036a2f9, 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.
4226@@ -4227,6 +4227,14 @@ bool static LoadBlockIndexDB(ChainstateManager& chainman, const CChainParams& ch
4227 return true;
4228 }
4229
4230+void CChainState::LoadMempool(const ArgsManager& args)
4231+{
4232+ if (args.GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) {
persistentMempool bool parameter to try to contain the configuration/command line argument logic in init.cpp ? (Even if there is gArgs logic elsewhere in validation.cpp ..)
4226@@ -4227,6 +4227,14 @@ bool static LoadBlockIndexDB(ChainstateManager& chainman, const CChainParams& ch
4227 return true;
4228 }
4229
4230+void CChainState::LoadMempool(const ArgsManager& args)
4231+{
4232+ if (args.GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) {
4233+ ::LoadMempool(m_mempool);
LoadMempool function, so –in a follow-up– its body could be moved here (or as a CTxMempool method) ?
1359@@ -1368,10 +1360,18 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1360 node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
1361 assert(!node.connman);
1362 node.connman = MakeUnique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), args.GetBoolArg("-networkactive", true));
1363+
1364 // Make mempool generally available in the node context. For example the connection manager, wallet, or RPC threads,