test: call VerifyLoadedChainstate during ChainTestingSetup #23736

pull jamesob wants to merge 1 commits into bitcoin:master from jamesob:2021-12-verify-loaded-cs-test changing 1 files +10 −0

jamesob commented at 4:48 PM on December 10, 2021: member

for additional coverage and similarity to actual init process.

Followup to #23280.
DrahtBot added the label Tests on Dec 10, 2021

in src/test/util/setup_common.cpp:187 in 1c6f4e6fe0 outdated

 186 | @@ -187,6 +187,16 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
 187 |          throw std::runtime_error("LoadGenesisBlock failed.");

MarcoFalke commented at 8:27 AM on December 11, 2021:

please rebase this on master, otherwise it is needlessly hard to review. I suspect this doesn't even compile?

jamesob commented at 1:24 PM on December 13, 2021:

Not sure I follow, but it's been rebased.

MarcoFalke commented at 1:40 PM on December 13, 2021:

To see if a commit compiles locally, you can use make. For example this commit indeed fails:

$ git checkout 1c6f4e6fe07bb111dd40e6ebfb8936f49a51c2e3 && make 
HEAD is now at 1c6f4e6fe0 test: call VerifyLoadedChainstate during ChainTestingSetup
Making all in src
make[1]: Entering directory '/home/marco/workspace/btc_bitcoin_core/src'
make[2]: Entering directory '/home/marco/workspace/btc_bitcoin_core/src'
make[3]: Entering directory '/home/marco/workspace/btc_bitcoin_core'
make[3]: Leaving directory '/home/marco/workspace/btc_bitcoin_core'
  CXX      test/util/libtest_util_a-setup_common.o
test/util/setup_common.cpp:192:9: error: use of undeclared identifier 'fReindex'
        fReindex.load(),
        ^
1 error generated.
make[2]: *** [Makefile:11221: test/util/libtest_util_a-setup_common.o] Error 1
make[2]: Leaving directory '/home/marco/workspace/btc_bitcoin_core/src'
make[1]: *** [Makefile:17227: all-recursive] Error 1
make[1]: Leaving directory '/home/marco/workspace/btc_bitcoin_core/src'
make: *** [Makefile:817: all-recursive] Error 1

jamesob force-pushed on Dec 13, 2021

in src/test/util/setup_common.cpp:198 in ad0cf78f76 outdated

 194 | @@ -195,6 +195,16 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
 195 |                               true);
 196 |      assert(!rv.has_value());
 197 |  
 198 | +    auto rv2 = VerifyLoadedChainstate(

MarcoFalke commented at 1:23 PM on December 13, 2021:

any reason to call this rv instead of something that makes sense?

in src/test/util/setup_common.cpp:199 in ad0cf78f76 outdated

 194 | @@ -195,6 +195,16 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
 195 |                               true);
 196 |      assert(!rv.has_value());
 197 |  
 198 | +    auto rv2 = VerifyLoadedChainstate(
 199 | +        *Assert(m_node.chainman.get()),

MarcoFalke commented at 1:25 PM on December 13, 2021:

        *Assert(m_node.chainman),

I think this should also compile?

in src/test/util/setup_common.cpp:205 in ad0cf78f76 outdated

 200 | +        fReindex.load(),
 201 | +        m_args.GetBoolArg("-reindex-chainstate", false),
 202 | +        chainparams.GetConsensus(),
 203 | +        m_args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS),
 204 | +        m_args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL),
 205 | +        GetAdjustedTime);

MarcoFalke commented at 1:28 PM on December 13, 2021:

What is the reason to use this time?

MarcoFalke commented at 1:28 PM on December 13, 2021: member

some q
jamesob force-pushed on Dec 13, 2021
jamesob commented at 2:03 PM on December 13, 2021: member

@MarcoFalke all fixed, thanks.
test: call VerifyLoadedChainstate during ChainTestingSetup
for additional coverage and similarity to actual init process.
826e12b010
jamesob force-pushed on Dec 13, 2021
jamesob commented at 2:18 PM on December 20, 2021: member

Should be a pretty easy review... @dongcarl maybe?
ryanofsky approved
ryanofsky commented at 2:40 PM on December 20, 2021: member

Code review ACK 826e12b010eda4238f9e8cd875e8915a405bed0d
dongcarl commented at 11:08 PM on December 20, 2021: member

Code Review ACK 826e12b010eda4238f9e8cd875e8915a405bed0d
jamesob commented at 7:43 PM on December 22, 2021: member

Ready for merge?
fanquake merged this on Dec 23, 2021
fanquake closed this on Dec 23, 2021
sidhujag referenced this in commit 99d9e47d60 on Dec 27, 2021
DrahtBot locked this on Dec 23, 2022

Contributors

Labels

Linked (view graph)

#23280 init: Coalesce Chainstate loading sequence between {,non-}unittest codepaths