AppInitMain goes from ~650 lines to ~500. This also replaces constructs like
while(false) and using break vs return with a simple bool result for more
explicit operation.
Prompted by looking into #13577
Suggest: git diff --color-moved=dimmed_zebra --color-moved-ws=allow-indentation-change head^ to aid review.
Both Windows builds failed with:
0In file included from /home/travis/build/bitcoin/bitcoin/depends/i686-w64-mingw32/share/../include/boost/thread/win32/condition_variable.hpp:9:0,
1 from /home/travis/build/bitcoin/bitcoin/depends/i686-w64-mingw32/share/../include/boost/thread/condition_variable.hpp:14,
2 from ./util.h:35,
3 from ./init.h:11,
4 from init.cpp:10:
5/home/travis/build/bitcoin/bitcoin/depends/i686-w64-mingw32/share/../include/boost/thread/win32/thread_primitives.hpp:177:55: warning: conflicts with previous declaration 'void* boost::detail::win32::GetModuleHandleA(const char*)'
6 __declspec(dllimport) void* __stdcall GetModuleHandleA(char const*);
7 ^~~~~~~~~~~~~~~~
8In file included from /usr/share/mingw-w64/include/windows.h:71:0,
9 from /usr/share/mingw-w64/include/winsock2.h:23,
10 from ./compat.h:39,
11 from ./util.h:17,
12 from ./init.h:11,
13 from init.cpp:10:
14init.cpp:1245:5: error: expected identifier before numeric constant
15 ERROR = 2,
16 ^
17init.cpp:1245:5: error: expected '}' before numeric constant
18init.cpp:1245:5: error: expected unqualified-id before numeric constant
19init.cpp:1246:1: error: expected declaration before '}' token
20 };
21 ^
22make[2]: *** [libbitcoin_server_a-init.o] Error 1
23Makefile:5820: recipe for target 'libbitcoin_server_a-init.o' failed
ERROR to FATAL to avoid windows conflict. Note this was in an enum class so seems Windows is applying an ERROR macro or the like.
utACK 67ed865.
Nice refactor. Personally, I find the enum a bit heavy and think it would be simpler to return a bool and an additional bool fatal_err outparam, but I don’t feel too strongly. Check in the false return case could just be
0if (fShutdownRequested) {
1} else if (success) {
2} else if (fReset || fatal_err) {
3 return InitError( );
4} else {
5}
fReset an out arg as it was already being used to trigger the retry behavior, so now it can be toggled within the function to disable retry. Lmk if you have thoughts on that.
utACK fb28c7f5cf903c0c21f823b2e179a39bd69c9549
Yes, I like this approach more.
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.
1225@@ -1226,6 +1226,136 @@ bool AppInitLockDataDirectory()
1226 return true;
1227 }
1228
1229+static bool AppInitLoadBlockIndex(const CChainParams& chainparams, int64_t nCoinDBCache, int64_t nBlockTreeDBCache, bool fReindexChainState, bool& fReset, std::string& strLoadError)
1230+{
1231+ try {
I really like this refactor, but I wanted to comment briefly on the try block.
I suspect that you kept the try block to keep things simple, but alternatively you can also use a function try-block if you like the notational convenience that it provides.
A function try-block will save one level of indentation in the function body, which could be nice with a function body of this size.
Function try-blocks are not commonly used, but it is perfectly fine to use them (at least according to Stroustrup).
With a function-try block this function would look something like this:
0static bool AppInitLockBlockIndex(...) try
1{
2 ...
3 return true;
4} catch (std::exception& e)
5{
6 ...
7 return false;
8}
Of course this comment is not critical, feel free to ignore it.
Travis failed with unrelated error, restarted.
Regarding function-try-block I’m -0 if the reasoning is just to save one level of indentation in the function body.
Locally I get:
0 CXX libbitcoin_server_a-init.o
1init.cpp:1252:14: warning: calling function 'LoadBlockIndex' requires holding mutex 'cs_main' exclusively [-Wthread-safety-analysis]
2 if (!LoadBlockIndex(chainparams)) {
3 ^
41 warning generated.
Let’s see what travis says
AppInitLoadBlockIndex
git diff --color-moved=dimmed_zebra --color-moved-ws=allow-indentation-change head^. Now is a minimal extraction.
AppInitMain goes from ~650 lines to ~500. This also replaces
constructs like `while(false)` and using `break` vs `return` with
more explicit operation.
1269+ }
1270+
1271+ // If the loaded chain has a wrong genesis, bail out immediately
1272+ // (we're likely using a testnet datadir, or the other way around).
1273+ if (!mapBlockIndex.empty() && !LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) {
1274+ fReset = true; // don't retry
break and return. This is special handling for the one return case.