Extract AppInitLoadBlockIndex from AppInitMain #13582

Empact · 2018-07-01T20:23:07Z

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.

fanquake · 2018-07-02T07:11:15Z

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

Empact · 2018-07-02T07:29:13Z

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.

jimpo · 2018-07-02T18:12:55Z

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 {
}

Empact · 2018-07-02T20:10:09Z

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.

jimpo · 2018-07-02T22:36:16Z

utACK fb28c7f5cf903c0c21f823b2e179a39bd69c9549

Yes, I like this approach more.

Empact · 2018-07-05T15:50:40Z

Rebased for #13235

laanwj · 2018-07-05T16:13:59Z

Needs rebase for #13577

Empact · 2018-07-06T04:17:54Z

Rebased for #13577

DrahtBot · 2018-07-06T07:27:09Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15329 (Fix InitError() and InitWarning() content by hebasto)
#15218 (validation: Flush state after initial sync by andrewtoth)
#12980 (Allow quicker shutdowns during LoadBlockIndex() by jonasschnelli)

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.

src/init.cpp

Empact · 2018-07-09T03:44:48Z

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

promag · 2018-07-09T08:45:10Z

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.

Empact · 2018-07-09T14:59:38Z

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.

promag · 2018-07-16T22:54:34Z

utACK a2dbe2a.

Empact · 2018-11-10T19:16:43Z

Rebased for #14437

Empact · 2018-11-10T22:49:09Z

Appveyor failure looks unrelated

Empact · 2019-01-17T12:51:14Z

Reworked to minimize git diff --color-moved=dimmed_zebra --color-moved-ws=allow-indentation-change head^. Now is a minimal extraction.

Empact · 2019-03-09T15:30:34Z

Rebase for #15402

AppInitMain goes from ~650 lines to ~500. This also replaces constructs like `while(false)` and using `break` vs `return` with more explicit operation.

Empact · 2019-05-17T19:06:56Z

Rebased

jb55 · 2019-05-18T14:36:33Z

src/init.cpp

+        // If the loaded chain has a wrong genesis, bail out immediately
+        // (we're likely using a testnet datadir, or the other way around).
+        if (!mapBlockIndex.empty() && !LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) {
+            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.

DrahtBot · 2019-05-20T08:33:19Z

Needs rebase

Empact · 2019-05-28T21:12:34Z

Closing as rebasing this is too much a chore. :P

Empact mentioned this pull request Jul 1, 2018

logging: avoid nStart may be used uninitialized in AppInitMain warning #13577

Merged

Empact force-pushed the app-init-load-block-index branch 2 times, most recently from 8426636 to e93569a Compare July 1, 2018 20:30

fanquake added the Refactoring label Jul 1, 2018

Empact force-pushed the app-init-load-block-index branch 3 times, most recently from 6bb58a2 to c913c77 Compare July 2, 2018 06:13

Empact force-pushed the app-init-load-block-index branch from c913c77 to 67ed865 Compare July 2, 2018 07:26

Empact force-pushed the app-init-load-block-index branch from 67ed865 to fb28c7f Compare July 2, 2018 20:07

Empact force-pushed the app-init-load-block-index branch from fb28c7f to b3a01bd Compare July 5, 2018 15:50

Empact force-pushed the app-init-load-block-index branch from b3a01bd to 463994c Compare July 6, 2018 04:16

l2a5b1 reviewed Jul 8, 2018

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

Empact force-pushed the app-init-load-block-index branch 2 times, most recently from 262a3f2 to 4c0ea2e Compare July 9, 2018 03:39

Empact force-pushed the app-init-load-block-index branch from 4c0ea2e to a2dbe2a Compare July 9, 2018 14:57

Empact mentioned this pull request Jul 14, 2018

[moveonly] Extract RescanWallet to handle a simple rescan #13658

Merged

DrahtBot mentioned this pull request Aug 31, 2018

Minor style enhancement in documentation #14091

Closed

This was referenced Sep 21, 2018

Allow quicker shutdowns during LoadBlockIndex() #12980

Merged

Refactor: separate wallet from node #10973

Merged

Multiprocess bitcoin #10102

Draft

DrahtBot mentioned this pull request Sep 30, 2018

[logs] Fix a few log messages related to duration measurement #14358

Closed

DrahtBot mentioned this pull request Oct 9, 2018

Refactor: Start to separate wallet from node #14437

Merged

DrahtBot added the Needs rebase label Nov 9, 2018

Empact force-pushed the app-init-load-block-index branch from c665630 to 64f9c90 Compare November 10, 2018 19:16

DrahtBot removed the Needs rebase label Nov 10, 2018

Empact force-pushed the app-init-load-block-index branch from 64f9c90 to d1a0c39 Compare January 17, 2019 12:49

DrahtBot added the Needs rebase label Mar 7, 2019

Empact force-pushed the app-init-load-block-index branch from d1a0c39 to 44c7789 Compare March 9, 2019 15:29

DrahtBot removed the Needs rebase label Mar 9, 2019

Empact force-pushed the app-init-load-block-index branch 2 times, most recently from f37aeea to 6dd65f8 Compare March 9, 2019 18:29

DrahtBot added the Needs rebase label May 7, 2019

Extract AppInitLoadBlockIndex from AppInitMain

c95a277

AppInitMain goes from ~650 lines to ~500. This also replaces constructs like `while(false)` and using `break` vs `return` with more explicit operation.

Empact force-pushed the app-init-load-block-index branch from 6dd65f8 to c95a277 Compare May 17, 2019 19:06

DrahtBot removed the Needs rebase label May 17, 2019

jb55 reviewed May 18, 2019

View reviewed changes

DrahtBot added the Needs rebase label May 20, 2019

Empact closed this May 28, 2019

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Extract AppInitLoadBlockIndex from AppInitMain #13582

Extract AppInitLoadBlockIndex from AppInitMain #13582

Uh oh!

Conversation

Empact commented Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jul 2, 2018

Uh oh!

Empact commented Jul 2, 2018

Uh oh!

jimpo commented Jul 2, 2018

Uh oh!

Empact commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimpo commented Jul 2, 2018

Uh oh!

Empact commented Jul 5, 2018

Uh oh!

laanwj commented Jul 5, 2018

Uh oh!

Empact commented Jul 6, 2018

Uh oh!

DrahtBot commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Empact commented Jul 9, 2018

Uh oh!

promag commented Jul 9, 2018

Uh oh!

Empact commented Jul 9, 2018

Uh oh!

promag commented Jul 16, 2018

Uh oh!

Empact commented Nov 10, 2018

Uh oh!

Empact commented Nov 10, 2018

Uh oh!

Empact commented Jan 17, 2019

Uh oh!

Empact commented Mar 9, 2019

Uh oh!

Empact commented May 17, 2019

Uh oh!

jb55 May 18, 2019

Choose a reason for hiding this comment

Uh oh!

Empact May 28, 2019

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented May 20, 2019

Uh oh!

Empact commented May 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Empact commented Jul 1, 2018 •

edited

Loading

Empact commented Jul 2, 2018 •

edited

Loading

DrahtBot commented Jul 6, 2018 •

edited

Loading