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:
In file included from /home/travis/build/bitcoin/bitcoin/depends/i686-w64-mingw32/share/../include/boost/thread/win32/condition_variable.hpp:9:0,
from /home/travis/build/bitcoin/bitcoin/depends/i686-w64-mingw32/share/../include/boost/thread/condition_variable.hpp:14,
from ./util.h:35,
from ./init.h:11,
from init.cpp:10:
/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*)'
__declspec(dllimport) void* __stdcall GetModuleHandleA(char const*);
^~~~~~~~~~~~~~~~
In file included from /usr/share/mingw-w64/include/windows.h:71:0,
from /usr/share/mingw-w64/include/winsock2.h:23,
from ./compat.h:39,
from ./util.h:17,
from ./init.h:11,
from init.cpp:10:
init.cpp:1245:5: error: expected identifier before numeric constant
ERROR = 2,
^
init.cpp:1245:5: error: expected '}' before numeric constant
init.cpp:1245:5: error: expected unqualified-id before numeric constant
init.cpp:1246:1: error: expected declaration before '}' token
};
^
make[2]: *** [libbitcoin_server_a-init.o] Error 1
Makefile:5820: recipe for target 'libbitcoin_server_a-init.o' failed
Renamed 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
if (fShutdownRequested) {
} else if (success) {
} else if (fReset || fatal_err) {
return InitError( );
} else {
}
Thanks for the review @jimpo. I see what you mean - switched to a boolean return, and made 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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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:
static bool AppInitLockBlockIndex(...) try
{
...
return true;
} catch (std::exception& e)
{
...
return false;
}
Of course this comment is not critical, feel free to ignore it.
Wow, TIL. Read up on them and I'm into it. If this is accepted, this will be the first use of function-try-block in the codebase. :P
Switched back as per #13582 (comment)
Switched to using function-try-block. If people aren't into that, maybe we should discourage them in the developer-notes? https://en.cppreference.com/w/cpp/language/function-try-block
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.
Yeah, on second thought, switched back to regular try/catch. The framing of the function body and the consistent treatment help with the reading of the function IMO.
utACK a2dbe2a.
Locally I get:
CXX libbitcoin_server_a-init.o
init.cpp:1252:14: warning: calling function 'LoadBlockIndex' requires holding mutex 'cs_main' exclusively [-Wthread-safety-analysis]
if (!LoadBlockIndex(chainparams)) {
^
1 warning generated.
Let's see what travis says
@MarcoFalke rebased and added the lock declaration to AppInitLoadBlockIndex
Concept ACK
Appveyor failure looks unrelated
Reworked to minimize 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.
Rebased
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
was this added? I don't see it in the original block. not saying it's wrong, but just wondering.
It's that way because there were 2 ways to exit the prior codeblock: break and return. This is special handling for the one return case.
<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase
Closing as rebasing this is too much a chore. :P