rpc: Remove mempool global from miner #17781

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:1912-mempoolMiner changing 13 files +95 −56

MarcoFalke commented at 3:25 pm on December 20, 2019: member

The miner needs read-only access to the mempool. Instead of using the mutable global ::mempool, keep a immutable reference to a mempool that is passed to the miner. Apart from the obvious benefits of removing a global and making things immutable, this might also simplify testing with multiple mempools.
test: Properly document blockinfo size in miner_tests
This fixes a typo in the test documentation
6666ef13f1
fanquake added the label RPC/REST/ZMQ on Dec 20, 2019
MarcoFalke added the label Mining on Dec 20, 2019
MarcoFalke added the label Refactoring on Dec 20, 2019
practicalswift commented at 3:35 pm on December 20, 2019: contributor

Strong concept ACK

Also welcome from a fuzz testing perspective :)
DrahtBot commented at 5:56 pm on December 20, 2019: member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:
- #17693 (rpc: Add generateblock to mine a custom set of transactions by andrewtoth)
- #17375 (Add asymptotes for benchmarking framework by JeremyRubin)
- #17268 (Epoch Mempool by JeremyRubin)
- #16975 (test: Show debug log on unit test failure by MarcoFalke)
- #16411 (BIP-325: Signet support by kallewoof)
- #16333 (test: Set BIP34Height = 2 for regtest by MarcoFalke)
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.
jnewbery commented at 6:01 pm on December 20, 2019: member

Concept ACK
JeremyRubin commented at 7:58 pm on December 20, 2019: contributor

Concept ACK, but this will make me sad from a rebasing perspective ;)
promag commented at 8:16 pm on December 20, 2019: member

Concept ACK.
in src/bench/bench.h:17 in fa9c739808 outdated
13@@ -14,6 +14,9 @@ 14 #include <boost/preprocessor/cat.hpp> 15 #include <boost/preprocessor/stringize.hpp> 16 17+struct RegTestingSetup;
emilengler commented at 2:43 pm on December 21, 2019:

Maybe I oversaw it but is it really necessary to include this variable into the header? As far as I know it should work just fine if just the pointer is included in the header file.

sipa commented at 2:52 pm on December 21, 2019:

This is not declaring a variable, it’s forward declaring the type RegTestingSetup.

emilengler commented at 7:24 pm on December 21, 2019:

But is it still required?

promag commented at 9:25 pm on December 22, 2019:

Yes, it’s defined in src/test/util/setup_common.h which isn’t included at this point.

in src/bench/bench.cpp:119 in fa9c739808 outdated

114@@ -113,6 +115,7 @@ void benchmark::BenchRunner::RunAll(Printer& printer, uint64_t num_evals, double
115 
116     for (const auto& p : benchmarks()) {
117         RegTestingSetup test{};
118+        g_testing_setup = &test;

promag commented at 9:29 pm on December 22, 2019:

nit, assert(g_testing_setup == nullptr) before

promag commented at 9:41 pm on December 22, 2019: member

Code review ACK fa9c7398087b821ccebf522ee5184eca7fa81445.
rpc: Remove mempool global from miner faa92a2297
MarcoFalke force-pushed on Dec 22, 2019
MarcoFalke commented at 11:14 pm on December 22, 2019: member

Addressed feedback by @promag
promag commented at 11:45 pm on December 22, 2019: member

ACK faa92a2297b4a6aebdd58d1818c428f1c0346078.
fjahr commented at 3:34 pm on January 2, 2020: member

ACK faa92a2297b4a6aebdd58d1818c428f1c0346078

Reviewed code, tested locally.

in src/miner.cpp:49 in faa92a2297

44@@ -45,7 +45,9 @@ BlockAssembler::Options::Options() {
45     nBlockMaxWeight = DEFAULT_BLOCK_MAX_WEIGHT;
46 }
47 
48-BlockAssembler::BlockAssembler(const CChainParams& params, const Options& options) : chainparams(params)
49+BlockAssembler::BlockAssembler(const CTxMemPool& mempool, const CChainParams& params, const Options& options)
50+    : chainparams(params),

jnewbery commented at 5:26 pm on January 2, 2020:

nit: order member initialization list the same as the parameter list (and preferably on one line)

MarcoFalke commented at 10:48 pm on January 2, 2020:

I prefer multiple lines

in src/rpc/mining.cpp:182 in faa92a2297
178@@ -179,9 +179,11 @@ static UniValue generatetodescriptor(const JSONRPCRequest& request) 179 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Cannot derive script without private keys")); 180 } 181 182+ const CTxMemPool& mempool = EnsureMemPool();
jnewbery commented at 5:30 pm on January 2, 2020:

nit: place this either at the start of the function (so it exits early if there is no mempool), or immediately before the call to generateBlocks(). Placing it between the code that creates coinbase_script and the coinbase_script CHECK seems random.

Same comment for generatetoaddress()

MarcoFalke commented at 10:50 pm on January 2, 2020:

Might do in a follow-up to not invalidate the three reviews
jnewbery commented at 5:41 pm on January 2, 2020: member

Code review ACK faa92a2297b4a6aebdd58d1818c428f1c0346078

A couple of nits inline. Feel free to ignore.
MarcoFalke referenced this in commit 17e14ac92f on Jan 2, 2020
MarcoFalke merged this on Jan 2, 2020
MarcoFalke closed this on Jan 2, 2020
MarcoFalke deleted the branch on Jan 2, 2020
sidhujag referenced this in commit d6bb0c9ca8 on Jan 4, 2020
MarcoFalke referenced this in commit fe48ac8580 on Jan 28, 2020
deadalnix referenced this in commit 97808ad068 on Oct 20, 2020
sidhujag referenced this in commit 9c047b4d37 on Nov 10, 2020
Fabcien referenced this in commit 8d80549973 on Dec 18, 2020
PastaPastaPasta referenced this in commit f473cf0c4f on Jun 27, 2021
PastaPastaPasta referenced this in commit 74efdbc95d on Jun 28, 2021
PastaPastaPasta referenced this in commit a80f20b943 on Jun 29, 2021
DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke practicalswift DrahtBot jnewbery JeremyRubin promag emilengler sipa fjahr

Labels
Refactoring RPC/REST/ZMQ Mining

rpc: Remove mempool global from miner #17781

Conflicts