Skip to content

Commit a3586d5

Browse files
author
MarcoFalke
committed
Merge #20323: tests: Create or use existing properly initialized NodeContexts
81137c6 test: Add new ChainTestingSetup and use it (Carl Dong) 7e9e7fe qt/test: [FIX] Add forgotten Context setting in RPCNestedTests (Carl Dong) Pull request description: This is part 1/n of the effort to [de-globalize `ChainstateManager`](bitcoin/bitcoin#20158) Reviewers: Looking for tested/Code-Review/plain-ACKs ### Context In many of our tests, we manually instantiate `NodeContext`s or `ChainstateManager`s in the test code, which is error prone. Instead, we should create or use existing references because: 1. Before we [de-globalize `ChainstateManager`](bitcoin/bitcoin#20158), much of our code still acts on `g_chainman` (our global `ChainstateManager`), sometimes even when you're calling a method on a specific instance of `ChainstateManager`! This means that we may act on two instances of `ChainstateManager`, which is most likely not what we want. 2. Using existing references (initialized by the `{Basic,}TestingSetup` constructors) means that you're acting on objects which are properly initialized, instead of "just initialized enough for this dang test to pass". Also, they're already there! It's free! 3. By acting on the right object, we also allow the review-only assertions in future commits of [de-globalize `ChainstateManager`](bitcoin/bitcoin#20158) to work and demonstrate correctness. Some more detailed debugging notes can be found in the first commit, reproduced below: ``` Previously, the validation_chainstatemanager_tests test suite instantiated its own duplicate ChainstateManager on which tests were performed. This wasn't a problem for the specific actions performed in that suite. However, the existence of this duplicate ChainstateManager and the fact that many of our validation static functions reach for g_chainman, ::Chain(state|)Active means we may end up acting on two different CChainStates should we write more extensive tests in the future. This change adds a new ChainTestingSetup which performs all initialization previously done by TestingSetup except: 1. Mempool sanity check frequency setting 2. ChainState initialization 3. Genesis Activation 4. {Ban,Conn,Peer}Man initialization Means that we will no longer need to initialize a duplicate ChainstateManger in order to test the initialization codepaths of CChainState and ChainstateManager. Lastly, this change has the additional benefit of allowing for review-only assertions meant to show correctness to work in future work de-globalizing g_chainman. In the test chainstatemanager_rebalance_caches, an additional LoadGenesisBlock call is added as MaybeReblanaceCaches eventually calls FlushBlockFile, which tries to access vinfoBlockFile[nLastBlockFile], which is out of bounds when LoadGenesisBlock hasn't been called yet. ----- Note for the future: In a previous version of this change, I put ChainTestingSetup between BasicTestingSetup and TestingSetup such that TestingSetup inherited from ChainTestingSetup. This was suboptimal, and showed how the class con/destructor inheritance structure we have for these TestingSetup classes is probably not the most suitable abstraction. In particular, for both TestingSetup and ChainTestingSetup, we need to stop the scheduler first before anything else. Otherwise classes depending on the scheduler may be referenced by the scheduler after said classes are freed. This means that there's no clear parallel between our teardown code and C++'s destructuring order for class hierarchies. Future work should strive to coalesce (as much as possible) test and non-test init codepaths and perhaps structure it in a more fail-proof way. ``` ACKs for top commit: MarcoFalke: ACK 81137c6 looking excellent now 🐩 jnewbery: ACK 81137c6 ryanofsky: Code review ACK 81137c6. This change is simpler after the rebase because wallet & bench commits are dropped. Tree-SHA512: a8d84f08f2db6428b0b88449bdc814c9db35b7559156d536dfebd3225c2707dba65959e76d2152e3f8c96eacbf1e0b0000f745edf1e196deddb97ff1ef360953
2 parents 90ef622 + 81137c6 commit a3586d5

File tree

4 files changed

+88
-73
lines changed

4 files changed

+88
-73
lines changed

‎src/qt/test/rpcnestedtests.cpp

Lines changed: 32 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -43,41 +43,41 @@ void RPCNestedTests::rpcNestedTests()
4343
tableRPC.appendCommand("rpcNestedTest", &vRPCCommands[0]);
4444

4545
TestingSetup test;
46+
m_node.setContext(&test.m_node);
4647

4748
if (RPCIsInWarmup(nullptr)) SetRPCWarmupFinished();
4849

4950
std::string result;
5051
std::string result2;
5152
std::string filtered;
52-
interfaces::Node* node = &m_node;
53-
RPCConsole::RPCExecuteCommandLine(*node, result, "getblockchaininfo()[chain]", &filtered); //simple result filtering with path
53+
RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo()[chain]", &filtered); //simple result filtering with path
5454
QVERIFY(result=="main");
5555
QVERIFY(filtered == "getblockchaininfo()[chain]");
5656

57-
RPCConsole::RPCExecuteCommandLine(*node, result, "getblock(getbestblockhash())"); //simple 2 level nesting
58-
RPCConsole::RPCExecuteCommandLine(*node, result, "getblock(getblock(getbestblockhash())[hash], true)");
57+
RPCConsole::RPCExecuteCommandLine(m_node, result, "getblock(getbestblockhash())"); //simple 2 level nesting
58+
RPCConsole::RPCExecuteCommandLine(m_node, result, "getblock(getblock(getbestblockhash())[hash], true)");
5959

60-
RPCConsole::RPCExecuteCommandLine(*node, result, "getblock( getblock( getblock(getbestblockhash())[hash] )[hash], true)"); //4 level nesting with whitespace, filtering path and boolean parameter
60+
RPCConsole::RPCExecuteCommandLine(m_node, result, "getblock( getblock( getblock(getbestblockhash())[hash] )[hash], true)"); //4 level nesting with whitespace, filtering path and boolean parameter
6161

62-
RPCConsole::RPCExecuteCommandLine(*node, result, "getblockchaininfo");
62+
RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo");
6363
QVERIFY(result.substr(0,1) == "{");
6464

65-
RPCConsole::RPCExecuteCommandLine(*node, result, "getblockchaininfo()");
65+
RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo()");
6666
QVERIFY(result.substr(0,1) == "{");
6767

68-
RPCConsole::RPCExecuteCommandLine(*node, result, "getblockchaininfo "); //whitespace at the end will be tolerated
68+
RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo "); //whitespace at the end will be tolerated
6969
QVERIFY(result.substr(0,1) == "{");
7070

71-
(RPCConsole::RPCExecuteCommandLine(*node, result, "getblockchaininfo()[\"chain\"]")); //Quote path identifier are allowed, but look after a child containing the quotes in the key
71+
(RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo()[\"chain\"]")); //Quote path identifier are allowed, but look after a child containing the quotes in the key
7272
QVERIFY(result == "null");
7373

74-
(RPCConsole::RPCExecuteCommandLine(*node, result, "createrawtransaction [] {} 0")); //parameter not in brackets are allowed
75-
(RPCConsole::RPCExecuteCommandLine(*node, result2, "createrawtransaction([],{},0)")); //parameter in brackets are allowed
74+
(RPCConsole::RPCExecuteCommandLine(m_node, result, "createrawtransaction [] {} 0")); //parameter not in brackets are allowed
75+
(RPCConsole::RPCExecuteCommandLine(m_node, result2, "createrawtransaction([],{},0)")); //parameter in brackets are allowed
7676
QVERIFY(result == result2);
77-
(RPCConsole::RPCExecuteCommandLine(*node, result2, "createrawtransaction( [], {} , 0 )")); //whitespace between parameters is allowed
77+
(RPCConsole::RPCExecuteCommandLine(m_node, result2, "createrawtransaction( [], {} , 0 )")); //whitespace between parameters is allowed
7878
QVERIFY(result == result2);
7979

80-
RPCConsole::RPCExecuteCommandLine(*node, result, "getblock(getbestblockhash())[tx][0]", &filtered);
80+
RPCConsole::RPCExecuteCommandLine(m_node, result, "getblock(getbestblockhash())[tx][0]", &filtered);
8181
QVERIFY(result == "4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b");
8282
QVERIFY(filtered == "getblock(getbestblockhash())[tx][0]");
8383

@@ -102,35 +102,35 @@ void RPCNestedTests::rpcNestedTests()
102102
RPCConsole::RPCParseCommandLine(nullptr, result, "help(importprivkey(abc), walletpassphrase(def))", false, &filtered);
103103
QVERIFY(filtered == "help(importprivkey(…), walletpassphrase(…))");
104104

105-
RPCConsole::RPCExecuteCommandLine(*node, result, "rpcNestedTest");
105+
RPCConsole::RPCExecuteCommandLine(m_node, result, "rpcNestedTest");
106106
QVERIFY(result == "[]");
107-
RPCConsole::RPCExecuteCommandLine(*node, result, "rpcNestedTest ''");
107+
RPCConsole::RPCExecuteCommandLine(m_node, result, "rpcNestedTest ''");
108108
QVERIFY(result == "[\"\"]");
109-
RPCConsole::RPCExecuteCommandLine(*node, result, "rpcNestedTest \"\"");
109+
RPCConsole::RPCExecuteCommandLine(m_node, result, "rpcNestedTest \"\"");
110110
QVERIFY(result == "[\"\"]");
111-
RPCConsole::RPCExecuteCommandLine(*node, result, "rpcNestedTest '' abc");
111+
RPCConsole::RPCExecuteCommandLine(m_node, result, "rpcNestedTest '' abc");
112112
QVERIFY(result == "[\"\",\"abc\"]");
113-
RPCConsole::RPCExecuteCommandLine(*node, result, "rpcNestedTest abc '' abc");
113+
RPCConsole::RPCExecuteCommandLine(m_node, result, "rpcNestedTest abc '' abc");
114114
QVERIFY(result == "[\"abc\",\"\",\"abc\"]");
115-
RPCConsole::RPCExecuteCommandLine(*node, result, "rpcNestedTest abc abc");
115+
RPCConsole::RPCExecuteCommandLine(m_node, result, "rpcNestedTest abc abc");
116116
QVERIFY(result == "[\"abc\",\"abc\"]");
117-
RPCConsole::RPCExecuteCommandLine(*node, result, "rpcNestedTest abc\t\tabc");
117+
RPCConsole::RPCExecuteCommandLine(m_node, result, "rpcNestedTest abc\t\tabc");
118118
QVERIFY(result == "[\"abc\",\"abc\"]");
119-
RPCConsole::RPCExecuteCommandLine(*node, result, "rpcNestedTest(abc )");
119+
RPCConsole::RPCExecuteCommandLine(m_node, result, "rpcNestedTest(abc )");
120120
QVERIFY(result == "[\"abc\"]");
121-
RPCConsole::RPCExecuteCommandLine(*node, result, "rpcNestedTest( abc )");
121+
RPCConsole::RPCExecuteCommandLine(m_node, result, "rpcNestedTest( abc )");
122122
QVERIFY(result == "[\"abc\"]");
123-
RPCConsole::RPCExecuteCommandLine(*node, result, "rpcNestedTest( abc , cba )");
123+
RPCConsole::RPCExecuteCommandLine(m_node, result, "rpcNestedTest( abc , cba )");
124124
QVERIFY(result == "[\"abc\",\"cba\"]");
125125

126126
// do the QVERIFY_EXCEPTION_THROWN checks only with Qt5.3 and higher (QVERIFY_EXCEPTION_THROWN was introduced in Qt5.3)
127-
QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(*node, result, "getblockchaininfo() .\n"), std::runtime_error); //invalid syntax
128-
QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(*node, result, "getblockchaininfo() getblockchaininfo()"), std::runtime_error); //invalid syntax
129-
(RPCConsole::RPCExecuteCommandLine(*node, result, "getblockchaininfo(")); //tolerate non closing brackets if we have no arguments
130-
(RPCConsole::RPCExecuteCommandLine(*node, result, "getblockchaininfo()()()")); //tolerate non command brackts
131-
QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(*node, result, "getblockchaininfo(True)"), UniValue); //invalid argument
132-
QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(*node, result, "a(getblockchaininfo(True))"), UniValue); //method not found
133-
QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(*node, result, "rpcNestedTest abc,,abc"), std::runtime_error); //don't tollerate empty arguments when using ,
134-
QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(*node, result, "rpcNestedTest(abc,,abc)"), std::runtime_error); //don't tollerate empty arguments when using ,
135-
QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(*node, result, "rpcNestedTest(abc,,)"), std::runtime_error); //don't tollerate empty arguments when using ,
127+
QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo() .\n"), std::runtime_error); //invalid syntax
128+
QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo() getblockchaininfo()"), std::runtime_error); //invalid syntax
129+
(RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo(")); //tolerate non closing brackets if we have no arguments
130+
(RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo()()()")); //tolerate non command brackts
131+
QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(m_node, result, "getblockchaininfo(True)"), UniValue); //invalid argument
132+
QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(m_node, result, "a(getblockchaininfo(True))"), UniValue); //method not found
133+
QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(m_node, result, "rpcNestedTest abc,,abc"), std::runtime_error); //don't tollerate empty arguments when using ,
134+
QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(m_node, result, "rpcNestedTest(abc,,abc)"), std::runtime_error); //don't tollerate empty arguments when using ,
135+
QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(m_node, result, "rpcNestedTest(abc,,)"), std::runtime_error); //don't tollerate empty arguments when using ,
136136
}

‎src/test/util/setup_common.cpp

Lines changed: 36 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -125,18 +125,12 @@ BasicTestingSetup::~BasicTestingSetup()
125125
ECC_Stop();
126126
}
127127

128-
TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const char*>& extra_args)
128+
ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::vector<const char*>& extra_args)
129129
: BasicTestingSetup(chainName, extra_args)
130130
{
131-
const CChainParams& chainparams = Params();
132-
// Ideally we'd move all the RPC tests to the functional testing framework
133-
// instead of unit tests, but for now we need these here.
134-
RegisterAllCoreRPCCommands(tableRPC);
135-
136-
m_node.scheduler = MakeUnique<CScheduler>();
137-
138131
// We have to run a scheduler thread to prevent ActivateBestChain
139132
// from blocking due to queue overrun.
133+
m_node.scheduler = MakeUnique<CScheduler>();
140134
threadGroup.create_thread([&] { TraceThread("scheduler", [&] { m_node.scheduler->serviceQueue(); }); });
141135
GetMainSignals().RegisterBackgroundSignalScheduler(*m_node.scheduler);
142136

@@ -146,39 +140,16 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
146140
m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1);
147141

148142
m_node.chainman = &::g_chainman;
149-
m_node.chainman->InitializeChainstate(*m_node.mempool);
150-
::ChainstateActive().InitCoinsDB(
151-
/* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
152-
assert(!::ChainstateActive().CanFlushToDisk());
153-
::ChainstateActive().InitCoinsCache(1 << 23);
154-
assert(::ChainstateActive().CanFlushToDisk());
155-
if (!LoadGenesisBlock(chainparams)) {
156-
throw std::runtime_error("LoadGenesisBlock failed.");
157-
}
158-
159-
BlockValidationState state;
160-
if (!ActivateBestChain(state, chainparams)) {
161-
throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
162-
}
163143

164144
// Start script-checking threads. Set g_parallel_script_checks to true so they are used.
165145
constexpr int script_check_threads = 2;
166146
for (int i = 0; i < script_check_threads; ++i) {
167147
threadGroup.create_thread([i]() { return ThreadScriptCheck(i); });
168148
}
169149
g_parallel_script_checks = true;
170-
171-
m_node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
172-
m_node.connman = MakeUnique<CConnman>(0x1337, 0x1337); // Deterministic randomness for tests.
173-
m_node.peerman = MakeUnique<PeerManager>(chainparams, *m_node.connman, m_node.banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool);
174-
{
175-
CConnman::Options options;
176-
options.m_msgproc = m_node.peerman.get();
177-
m_node.connman->Init(options);
178-
}
179150
}
180151

181-
TestingSetup::~TestingSetup()
152+
ChainTestingSetup::~ChainTestingSetup()
182153
{
183154
if (m_node.scheduler) m_node.scheduler->stop();
184155
threadGroup.interrupt_all();
@@ -196,6 +167,39 @@ TestingSetup::~TestingSetup()
196167
pblocktree.reset();
197168
}
198169

170+
TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const char*>& extra_args)
171+
: ChainTestingSetup(chainName, extra_args)
172+
{
173+
const CChainParams& chainparams = Params();
174+
// Ideally we'd move all the RPC tests to the functional testing framework
175+
// instead of unit tests, but for now we need these here.
176+
RegisterAllCoreRPCCommands(tableRPC);
177+
178+
m_node.chainman->InitializeChainstate(*m_node.mempool);
179+
::ChainstateActive().InitCoinsDB(
180+
/* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
181+
assert(!::ChainstateActive().CanFlushToDisk());
182+
::ChainstateActive().InitCoinsCache(1 << 23);
183+
assert(::ChainstateActive().CanFlushToDisk());
184+
if (!LoadGenesisBlock(chainparams)) {
185+
throw std::runtime_error("LoadGenesisBlock failed.");
186+
}
187+
188+
BlockValidationState state;
189+
if (!ActivateBestChain(state, chainparams)) {
190+
throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
191+
}
192+
193+
m_node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
194+
m_node.connman = MakeUnique<CConnman>(0x1337, 0x1337); // Deterministic randomness for tests.
195+
m_node.peerman = MakeUnique<PeerManager>(chainparams, *m_node.connman, m_node.banman.get(), *m_node.scheduler, *m_node.chainman, *m_node.mempool);
196+
{
197+
CConnman::Options options;
198+
options.m_msgproc = m_node.peerman.get();
199+
m_node.connman->Init(options);
200+
}
201+
}
202+
199203
TestChain100Setup::TestChain100Setup()
200204
{
201205
// Generate a 100-block chain:

‎src/test/util/setup_common.h

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,14 +83,21 @@ struct BasicTestingSetup {
8383
const fs::path m_path_root;
8484
};
8585

86-
/** Testing setup that configures a complete environment.
87-
* Included are coins database, script check threads setup.
86+
/** Testing setup that performs all steps up until right before
87+
* ChainstateManager gets initialized. Meant for testing ChainstateManager
88+
* initialization behaviour.
8889
*/
89-
struct TestingSetup : public BasicTestingSetup {
90+
struct ChainTestingSetup : public BasicTestingSetup {
9091
boost::thread_group threadGroup;
9192

93+
explicit ChainTestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::vector<const char*>& extra_args = {});
94+
~ChainTestingSetup();
95+
};
96+
97+
/** Testing setup that configures a complete environment.
98+
*/
99+
struct TestingSetup : public ChainTestingSetup {
92100
explicit TestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::vector<const char*>& extra_args = {});
93-
~TestingSetup();
94101
};
95102

96103
/** Identical to TestingSetup, but chain set to regtest */

‎src/test/validation_chainstatemanager_tests.cpp

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,16 @@
1515

1616
#include <boost/test/unit_test.hpp>
1717

18-
BOOST_FIXTURE_TEST_SUITE(validation_chainstatemanager_tests, TestingSetup)
18+
BOOST_FIXTURE_TEST_SUITE(validation_chainstatemanager_tests, ChainTestingSetup)
1919

2020
//! Basic tests for ChainstateManager.
2121
//!
2222
//! First create a legacy (IBD) chainstate, then create a snapshot chainstate.
2323
BOOST_AUTO_TEST_CASE(chainstatemanager)
2424
{
25-
ChainstateManager manager;
26-
CTxMemPool mempool;
25+
ChainstateManager& manager = *m_node.chainman;
26+
CTxMemPool& mempool = *m_node.mempool;
27+
2728
std::vector<CChainState*> chainstates;
2829
const CChainParams& chainparams = Params();
2930

@@ -104,8 +105,9 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
104105
//! Test rebalancing the caches associated with each chainstate.
105106
BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
106107
{
107-
ChainstateManager manager;
108-
CTxMemPool mempool;
108+
ChainstateManager& manager = *m_node.chainman;
109+
CTxMemPool& mempool = *m_node.mempool;
110+
109111
size_t max_cache = 10000;
110112
manager.m_total_coinsdb_cache = max_cache;
111113
manager.m_total_coinstip_cache = max_cache;
@@ -122,6 +124,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
122124
{
123125
LOCK(::cs_main);
124126
c1.InitCoinsCache(1 << 23);
127+
BOOST_REQUIRE(c1.LoadGenesisBlock(Params()));
125128
c1.CoinsTip().SetBestBlock(InsecureRand256());
126129
manager.MaybeRebalanceCaches();
127130
}
@@ -139,6 +142,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
139142
{
140143
LOCK(::cs_main);
141144
c2.InitCoinsCache(1 << 23);
145+
BOOST_REQUIRE(c2.LoadGenesisBlock(Params()));
142146
c2.CoinsTip().SetBestBlock(InsecureRand256());
143147
manager.MaybeRebalanceCaches();
144148
}

0 commit comments

Comments
 (0)