Logger and initialization refinement and overhaul #17
Conversation
dexX7
force-pushed
the
oc-0.10-extract-logger
branch
from
d25a48d to
fad528d
Compare
|
Reviewed & supported. OK to merge from my end :) |
Usage: - Any entity that likes to use the logger has to include `mastercore_log.h` - To add a new category, an entry in `mastercore_log.h` and `mastercore_log.cpp` should be created - To enable or disable logging of a specific category, `mastercore_log.cpp` is the place to go Notes: - It provides a central place for anything log related - With `_log.h` included, there is no need for `external ...` anymore - `mp_log()` and `mp_error()` were never used, so I don't see any need to carry them around for now - The categories might be enabled and disabled via RPC or config file at some point later
Instead of printing line numbers and file names, the status messages were updated. The dumping of all balances after the initial scan was removed completely for a few reasons: - It was only printed after a scan, but not on every start - The information may as well be outdated, if the scanning takes a long time (same applies to "Exodus balance", too..) - The information is not really relevant - The information is also available via other channels (e.g. via RPC "mscrpc", "getallbalances..MP") Updated output: ```bash $ ./src/qt/bitcoin-qt Initializing Omni Core v0.0.9.2-dev [regtest] Loading trades database: OK Loading send-to-owners database: OK Loading transactions database: OK Loading smart property database: OK Exodus balance: 37986.81207497 MSC Omni Core initialization completed Omni Core shutdown completed ``` ```bash $ ./src/qt/bitcoin-qt -startclean Initializing Omni Core v0.0.9.2-dev [regtest] Loading trades database: OK Loading send-to-owners database: OK Loading transactions database: OK Loading smart property database: OK Scanning for transactions in block 5 to block 749.. 1591 transactions processed, 432 meta transactions found Exodus balance: 37986.81207497 MSC Omni Core initialization completed Omni Core shutdown completed ```
No functional change (except `int -> unsigned int` as expected), and intended to make the changes of the following commits more visible.
In case the initial scan stopped early, it is reported. Further, prevent segfault, in case of some unexpected situation where `pblockindex == NULL`.
The initial transaction scanning can easily take several minutes, maybe longer, and the user should not have the impression the program froze.
Until now there was no way to stop the initial scan, but any shutdown request (intended or unintended) should be handled properly and shutdown the program within reasonable time, ideally without breaking anything.
`LogStatus()` is intended as superior replacement of `printf()`. It allows formatting, doesn't require `.c_str()`, and could further be configured at some point. For example, there could be a `-quiet` configuration option, to prevent any regular output to the console. In it's current form, it is affected by the configuration option `-logtimestamps`, to prepend timestamps to the console prints.
Ideally, `printf` is not used in the future, but completely substituted by `LogStatus()`, which allows further formatting, and doesn't require `.c_str()`.
The out of order block check is no longer done with Bitcoin Core 0.10, and the status reporting was a leftover.
`fDisableWallet` exists only, if not build with `--without-wallet`. In the case Omni Core is indeed build with `--witout-wallet`, then `fDisableWallet` wouldn't exist at all, and the check would be faulty.
This was a rather low-hanging fruit, it has no functional impact and does not affect pending PRs.
dexX7
force-pushed
the
oc-0.10-extract-logger
branch
from
fad528d to
93eae99
Compare
dexX7
changed the title
|
Alright. This pull request was updated significantly. Sorry @zathras-crypto, and please don't get scared by the wall of text, hehe.. the overall changes are actually rather small. I figured it would probably make sense to bundle several related changes, to reduce the overall time, because otherwise I'd probably would have pushed the logger PR, then the follow up, then another follow up, and another follow up. I suggest to step through commit to commit, and they were created to be easily reviewed one by one. Please let me know, if there is anything that should changed, removed, added, ... and I'm curious, which work flow you would prefer for the future: smaller chucks or bundled pull requests like this? Anyway, feedback is very welcomed.. :) |
|
@dexX7 there are some excellent suggestions in here - starting to review now. Quick nitpick - "LogStatus" instead of printf? It's just the name that seems off to me - to "log" something is to place an entry in a log; if we are not doing that (just printing to console does not log anything) is "log" the right term? |
|
Yeah, I have a mixed feeling about I'd be happy to replace this part, or the status messages, if you have any other idea. Maybe |
|
Perhaps I'm 'old skool' hehe but I've always referred to it as 'echoing to console' (too much time spent on batch files in the 90s maybe?!?!? haha).
2 commits reviewed - so far so good - sitting in the waiting room at the doc's - if it wasn't for a lappy and tethering I think daytime TV (one big infomercial) would drive me nuts!!! |
|
In this case I tend to use |
Agreed.
Reviewed, all fine with me mate - few minor niggles I've commented about but other than that OK with me. Great work mate, this is really good stuff :) |
|
Nice. So to summarize:
What do you think about the messages? Also a note about the logging altogther: we might adopt a similar approach like Bitcoin Core, where no categories are hardcoded, but provided as argument, for example: LogPrint("selectcoins", "total %s\n", FormatMoney(nBest));... instead of something like we use currently: if (selectcoins) { LogPrintf("total %s\n", FormatMoney(nBest)); }The categories are then activated via configuration or startup options: |
Yep :)
Messages are clear - all good :)
Yeah this is a nice model - improving logging had been on my todo for a while so I really appreciate you cleaning it up. My main objection to our current model is the requirement to recompile to enable debug flags (which means we can't easily ask integrators/users to enable them for support purposes). I'd been planning to go RPC, but it makes sense to adopt the Bitcoin Core methodology and use startup params so there is a standard logging methodology right across the software. |
The debug flags are no longer constant and could now be updated at runtime. :) One more point: the Exodus balance is printed at the end of the initialization, but the intend wasn't really clear to me, so I'm unsure how to handle it, or if there might be another way. I guess this is sort of a verification/consensus hint? If so, it might make sense to print to block height as well, to make it comparable. |
Indeed. But it's irrelevant information for most. I think perhaps a better way to go would be to have an EDIT: I guess |
It's reasonable, but what would you consider as informative, in this context? Printing all balances brings us back to the beginning, and there is actually not much I could think of, which I would consider as helpful. Some general numbers maybe, like the number of transactions, properties, etc. maybe, but all this can also be retrieved via explicit commands. |
|
It's interesting (for me at least) - I'm quite a visual person. The brain is essentially just one large pattern recognition engine and you'd be surprised at the things you spot simply because they look out of place compared to prior reference - even if you're not looking for them. I figure when you start the client as many times as we do (numerous times per day) dumping some info would give us an easy spot on some things without directly looking for them - eg if we echo Perhaps that's just my perspective on things though - perhaps a bit 'fringe' hehe - tl:dr; if we had a All just thinking out loud here mate :) |
|
Don't get me wrong, I like the idea, and a clean summary would be awesome. I have just some trouble pinning down specifics, at least when I think about it at the moment. It's easier for me to see something, and use that as basis, so to speak. :) But when I further think about it: I'd be interested in protocol usage information, e.g. the number (in total and "recently") of properties created, transactions or trades. Also important in my opinion would be to know, whether the client is fully synchronized and up to date, given that "initialized" doesn't equal "up to date", but only "started". |
|
Re my comments on splash above, I tested it - works well - this is a very nice change here mate thanks :) I just dropped the following into
|
|
Ha! Nicely done. I was actually thinking about the progress signal, but this seems to be easier than anticipated. I'll finish the branch quickly, and merge, to get a basis for the UI integration. :) Quick side note re: /**
* Translation function: Call Translate signal on UI interface, which returns a boost::optional result.
* If no translation slot is registered, nothing is returned, and simply return the input.
*/
inline std::string _(const char* psz)
{
boost::optional<std::string> rv = uiInterface.Translate(psz);
return rv ? (*rv) : psz;
}In this case there would be no sentence or word to translate, but a bunch of words with numbers, which are probably difficult to translate. :) |
The internally used functions were also renamed: - DebugLogPrint to LogFilePrint - StatusLogPrint to ConsolePrint The loggers are now available via: - file_log (unchanged) - PrintToConsole (as discussed)
- Increase time between progress reports from 15 s to 30 s - Stop early, if a block could not be retrieved from the disk - Extract progress reporting (to be ready to fire some signals or whatsoever)
|
Updated as discussed. Diff: 93eae99...dexX7:oc-0.10-extract-logger |
|
Did you want to throw a UI splash update in there too mate? |
|
@zathras-crypto: what are the up or downsides over I'll give it a try: as far as I can see: When using When using When using
When using When using When using
When mixing this with our
I'd like:
So far, I'd say combining I tested additionally firing |
- Prints to console - Writes to debug.log - Updates RPC status message - Updates splash screen label, when using Omni Core QT
I favour Logging these "still scanning..." lines seems excessive, but updating the RPC response during startup is very beneficial and it sounds like from your testing it's both or neither. |
Figured I'd just drop a note here - I'm still looking into it, but I've added your fix in But am still getting a "fatal internal error" when I call a shutdown during scan - output: I'm using As I say just FYI, I'll have a nose around & see what I can find :) EDIT: tail of debug.log here but not much help |
Given that this progress blocks the whole program, frequent updates aren't that bad, especially when almost every second something is written to the debug.log. FWIW: users scan a whole lot of transactions usually only once. I don't really like it either though, and I also don't like the infrequent UI updates, but I think it's a middleground, and the alternative, a smarter status reporter, with different time settings and more targeted reporting, seems like an overkill - at least at the very moment. This could be something for a follow up, and it sort of fits into the category of more targeted signaling in general.
Let me quickly build and test your branch. Can you describe how I can reproduce the issue? |
Agreed - it's a fantastic improvement over what we had previously (an apparent hung process for 60 mins) :)
If you just build and run Oh, and run with Thanks again bud |
|
Note, if you want to play around with it just introduce a math error somewhere (for example in STO calculation or something) that results in tokens disappearing/appearing unexpectedly to see it in action :) |
Haha oh well. This was unexpected.. :) Check this out: src/main.cpp#L2838 |
Oh! That wasn't there in 0.9 - hmm - that will affect shutdown's due to alerts also - nice spot :) |
|
Yeah. :) Is there anything you'd like to change in this PR before a merge? |
|
Nope, it all looks good mate
|
3f89cd0 Report initial scanning progress via multiple channels (dexX7) 355bf3d Update, refine and document msc_initial_scan (dexX7) fe25326 Document mastercore_init and mastercore_shutdown (dexX7) 4339037 Rename LogStatus to PrintToConsole (and related) (dexX7) 93eae99 Superficial cleanup of unused parts and comments (dexX7) f07a23d Guard against build without wallet during initialization (dexX7) 79749e9 Remove obsolete block order check status message (dexX7) 8fb73fc Replace most printf with LogStatus() (dexX7) 6a8a0cc Add LogStatus() to print directly to the console (dexX7) 947e52f Handle shutdown request during initial scan (dexX7) 3d4e079 Print initial scanning status every 15 seconds (dexX7) 0c80d4f Handle early or unfinished stop of initial transaction scan (dexX7) 8e1e138 Prepare and format msc_initial_scan() (dexX7) 6e9ae6e Prettify printing of initialization status messages (dexX7) 701ca0a Move debug logger into seperated file (dexX7)
|
Awesome. Here is a tip for your current branches (except the class C branch, which unfortunally requires a lot, but no different than the following, manual tweaking): # update the local copy of the main branch
git fetch upstream omnicore-0.0.10
git checkout omnicore-0.0.10
git pull upstream omnicore-0.0.10
# then checkout your feature branch
git checkout 0.0.10-Z-UI
# and apply all commits upon the new "base"
git rebase omnicore-0.0.10This should work without any issue for the UI branch, and I prefer this route over merging some main branch into a feature branch, every time there is an update, but maybe that's just personal taste. :) When doing this with the auditor branch, at some point git will complain due to conflicts in mastercore.cpp: Here, you could manually resolve the conflict by removing the red lines: @@ -1575,10 +1575,23 @@ static int msc_initial_scan(int nFirstBlock)
break;
}
-<<<<<<< HEAD
if (GetTime() >= nNow + 30) { // seconds
ReportScanProgress(nFirstBlock, nBlock, nLastBlock);
nNow = GetTime();
}
-=======
- CBlock block;
- for (int blockNum = nHeight;blockNum<=max_block;blockNum++)
- {
- if (ShutdownRequested()) {
- file_log("Shutdown requested, stop scan at block %d of %d\n", blockNum, max_block);
- break;
- }
-
- CBlockIndex* pblockindex = chainActive[blockNum];
- string strBlockHash = pblockindex->GetBlockHash().GetHex();
->>>>>>> Auditor : include DexX's fix for breaking out of scan (note still fatal error on shutdown)
CBlockIndex* pblockindex = chainActive[nBlock];
if (NULL == pblockindex) break;And after mastercore.cpp was saved, the file can be tagged as updated, and it can continue: Next, git will complain, because this specific commit is now empty, and this can be resolved by: ... and you're done! :) This is probably followed by overwriting the remote version of that branch: See here - it shows "commited by dexx", because I'm not you obviously.. :) |
|
Hmm, I actually just did this before seeing your comment - I just needed to pull from origin, resolve that merge conflict in |
Haha no.. rebasing is just an alternative to merging, but it's perfectly fine. :) But unfortunally I noticed one of the later commits of this PR actually did cause a conflict for the class C branch.. :/ (~two lines change though) |
Two lines ❗ No problems at all mate, it's to be expected when we're bouncing around various different things :) |
Maybe I'm overly concerned, when it comes to those things, but let's just say: I'm trying to avoid to interrupt the workflow of other pull requests, which have a higher priority in my opinion.. at least as best as I can. :) |
fac04cb refactor: Add lock annotations to Active* methods (MacroFake) fac15ff Fix logical race in rest_getutxos (MacroFake) fa97a52 Fix UB/data-race in RPCNotifyBlockChange (MacroFake) fa530bc Add ChainstateManager::GetMutex(), an alias for ::cs_main (MacroFake) Pull request description: This fixes two issues: * A data race in `ActiveChain`, which returns a reference to the chain (a `std::vector`), which is not thread safe. See also below traceback. * A corrupt rest response, which returns a blockheight and blockhash, which are unrelated to each other and to the result, as the chain might advance between each call without cs_main held. The issues are fixed by taking cs_main and holding it for the required time. ``` ================== WARNING: ThreadSanitizer: data race (pid=32335) Write of size 8 at 0x7b3c000008f0 by thread T22 (mutexes: write M131626, write M151, write M131553): #0 std::__1::enable_if<(is_move_constructible<CBlockIndex**>::value) && (is_move_assignable<CBlockIndex**>::value), void>::type std::__1::swap<CBlockIndex**>(CBlockIndex**&, CBlockIndex**&) /usr/lib/llvm-13/bin/../include/c++/v1/__utility/swap.h:39:7 (bitcoind+0x501239) OmniLayer#1 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::__swap_out_circular_buffer(std::__1::__split_buffer<CBlockIndex*, std::__1::allocator<CBlockIndex*>&>&) /usr/lib/llvm-13/bin/../include/c++/v1/vector:977:5 (bitcoind+0x501239) OmniLayer#2 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::__append(unsigned long) /usr/lib/llvm-13/bin/../include/c++/v1/vector:1117:9 (bitcoind+0x501239) OmniLayer#3 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::resize(unsigned long) /usr/lib/llvm-13/bin/../include/c++/v1/vector:2046:15 (bitcoind+0x4ffe29) OmniLayer#4 CChain::SetTip(CBlockIndex*) src/chain.cpp:19:12 (bitcoind+0x4ffe29) OmniLayer#5 CChainState::ConnectTip(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, ConnectTrace&, DisconnectedBlockTransactions&) src/validation.cpp:2748:13 (bitcoind+0x475d00) OmniLayer#6 CChainState::ActivateBestChainStep(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, bool&, ConnectTrace&) src/validation.cpp:2884:18 (bitcoind+0x47739e) OmniLayer#7 CChainState::ActivateBestChain(BlockValidationState&, std::__1::shared_ptr<CBlock const>) src/validation.cpp:3011:22 (bitcoind+0x477baf) OmniLayer#8 node::ThreadImport(ChainstateManager&, std::__1::vector<fs::path, std::__1::allocator<fs::path> >, ArgsManager const&) src/node/blockstorage.cpp:883:30 (bitcoind+0x23cd74) OmniLayer#9 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1657:9 (bitcoind+0x15863e) OmniLayer#10 decltype(static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(fp)()) std::__1::__invoke<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x15863e) OmniLayer#11 void std::__1::__invoke_void_return_wrapper<void, true>::__call<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/__functional/invoke.h:61:9 (bitcoind+0x15863e) OmniLayer#12 std::__1::__function::__alloc_func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:171:16 (bitcoind+0x15863e) OmniLayer#13 std::__1::__function::__func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:345:12 (bitcoind+0x15863e) OmniLayer#14 std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x88891f) OmniLayer#15 std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x88891f) OmniLayer#16 util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x88891f) OmniLayer#17 decltype(static_cast<void (*>(fp)(static_cast<char const*>(fp0), static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(fp0))) std::__1::__invoke<void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(void (*&&)(char const*, std::__1::function<void ()>), char const*&&, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x157e6a) OmniLayer#18 void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>&, std::__1::__tuple_indices<2ul, 3ul>) /usr/lib/llvm-13/bin/../include/c++/v1/thread:280:5 (bitcoind+0x157e6a) OmniLayer#19 void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7> >(void*) /usr/lib/llvm-13/bin/../include/c++/v1/thread:291:5 (bitcoind+0x157e6a) Previous read of size 8 at 0x7b3c000008f0 by main thread: #0 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::size() const /usr/lib/llvm-13/bin/../include/c++/v1/vector:680:61 (bitcoind+0x15179d) OmniLayer#1 CChain::Tip() const src/./chain.h:449:23 (bitcoind+0x15179d) OmniLayer#2 ChainstateManager::ActiveTip() const src/./validation.h:927:59 (bitcoind+0x15179d) OmniLayer#3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1841:35 (bitcoind+0x15179d) OmniLayer#4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) OmniLayer#5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) Location is heap block of size 232 at 0x7b3c00000870 allocated by main thread: #0 operator new(unsigned long) <null> (bitcoind+0x132668) OmniLayer#1 ChainstateManager::InitializeChainstate(CTxMemPool*, std::__1::optional<uint256> const&) src/validation.cpp:4851:21 (bitcoind+0x48e26b) OmniLayer#2 node::LoadChainstate(bool, ChainstateManager&, CTxMemPool*, bool, Consensus::Params const&, bool, long, long, long, bool, bool, std::__1::function<bool ()>, std::__1::function<void ()>) src/node/chainstate.cpp:31:14 (bitcoind+0x24de07) OmniLayer#3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1438:32 (bitcoind+0x14e994) OmniLayer#4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) OmniLayer#5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) Mutex M131626 (0x7b3c00000898) created at: #0 pthread_mutex_lock <null> (bitcoind+0xda898) OmniLayer#1 std::__1::mutex::lock() <null> (libc++.so.1+0x49f35) OmniLayer#2 node::ThreadImport(ChainstateManager&, std::__1::vector<fs::path, std::__1::allocator<fs::path> >, ArgsManager const&) src/node/blockstorage.cpp:883:30 (bitcoind+0x23cd74) OmniLayer#3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1657:9 (bitcoind+0x15863e) OmniLayer#4 decltype(static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(fp)()) std::__1::__invoke<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x15863e) OmniLayer#5 void std::__1::__invoke_void_return_wrapper<void, true>::__call<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/__functional/invoke.h:61:9 (bitcoind+0x15863e) OmniLayer#6 std::__1::__function::__alloc_func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:171:16 (bitcoind+0x15863e) OmniLayer#7 std::__1::__function::__func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:345:12 (bitcoind+0x15863e) OmniLayer#8 std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x88891f) OmniLayer#9 std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x88891f) OmniLayer#10 util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x88891f) OmniLayer#11 decltype(static_cast<void (*>(fp)(static_cast<char const*>(fp0), static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(fp0))) std::__1::__invoke<void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(void (*&&)(char const*, std::__1::function<void ()>), char const*&&, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x157e6a) OmniLayer#12 void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>&, std::__1::__tuple_indices<2ul, 3ul>) /usr/lib/llvm-13/bin/../include/c++/v1/thread:280:5 (bitcoind+0x157e6a) OmniLayer#13 void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7> >(void*) /usr/lib/llvm-13/bin/../include/c++/v1/thread:291:5 (bitcoind+0x157e6a) Mutex M151 (0x55aacb8ea030) created at: #0 pthread_mutex_init <null> (bitcoind+0xbed2f) OmniLayer#1 std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3) OmniLayer#2 __libc_start_main <null> (libc.so.6+0x29eba) Mutex M131553 (0x7b4c000042e0) created at: #0 pthread_mutex_init <null> (bitcoind+0xbed2f) OmniLayer#1 std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3) OmniLayer#2 std::__1::__unique_if<CTxMemPool>::__unique_single std::__1::make_unique<CTxMemPool, CBlockPolicyEstimator*, int const&>(CBlockPolicyEstimator*&&, int const&) /usr/lib/llvm-13/bin/../include/c++/v1/__memory/unique_ptr.h:728:32 (bitcoind+0x15c81d) OmniLayer#3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1426:24 (bitcoind+0x14e7b4) OmniLayer#4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) OmniLayer#5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) Thread T22 'b-loadblk' (tid=32370, running) created by main thread at: #0 pthread_create <null> (bitcoind+0xbd5bd) OmniLayer#1 std::__1::__libcpp_thread_create(unsigned long*, void* (*)(void*), void*) /usr/lib/llvm-13/bin/../include/c++/v1/__threading_support:443:10 (bitcoind+0x155e06) OmniLayer#2 std::__1::thread::thread<void (*)(char const*, std::__1::function<void ()>), char const (&) [8], AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, void>(void (*&&)(char const*, std::__1::function<void ()>), char const (&) [8], AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/thread:307:16 (bitcoind+0x155e06) OmniLayer#3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1656:29 (bitcoind+0x150164) OmniLayer#4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) OmniLayer#5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) SUMMARY: ThreadSanitizer: data race /usr/lib/llvm-13/bin/../include/c++/v1/__utility/swap.h:39:7 in std::__1::enable_if<(is_move_constructible<CBlockIndex**>::value) && (is_move_assignable<CBlockIndex**>::value), void>::type std::__1::swap<CBlockIndex**>(CBlockIndex**&, CBlockIndex**&) ================== ``` From https://cirrus-ci.com/task/5612886578954240?logs=ci#L4868 ACKs for top commit: achow101: re-ACK fac04cb theStack: Code-review ACK fac04cb Tree-SHA512: 9d619f99ff6373874c7ffe1db20674575605646b4b54b692fb54515a4a49f110a770026d7320ed6dfeaa7976be4cd89e93f821acdbf22c7662bd1c5be0cedcd2
1) File logger and the loggers in general:
Usage:
mastercore_log.hmastercore_log.handmastercore_log.cppshould be createdmastercore_log.cppis the place to goNotes:
_log.hincluded, there is no need forexternal ...anymoremp_log()andmp_error()were never used, so I don't see any need to carry them around for now2) Console or status logger:
As subsitute for
printf()another logger functionPrintToConsole()was added:PrintToConsole()has a similar behavior likefile_log(), but instead of printing tomastercore.log, the logger prints directly to the consolePrintToConsole()is superior toprintf(), as it comes with smarter tinyformat based formatting, and for example.c_str()can be omitted, when printing stringsprintf(), which were sort of annoying and difficult to tame.. ;)-logtimestampsto prepend the output with timestamps-quiet, to silence all regular console output3) More useful and readable initializtion messages:
Instead of printing line numbers and file names, the status messages were updated.
Further, the dumping of balances after an initial scan was removed completely for a few reasons:
mscrpc,getallbalances..MP4) Progress reporting during the intial transaction scanning:
Every 30 seconds the progress of the scanning is printed to the console, written to the debug log file, and the RPC status, as well as the splash screen progress label, are updated.
It was important for me to show that Omni Core hasn't stalled, or is broken and doesn't do anything during the scanning, but in fact has already processed n of m blocks.
5) Shutdown request handling during the initial scanning:
Until now there was no way to shutdown Omni Core during the initial scanning, whether the shutdown request was intentionally, or triggered by some other event, for example a restart of the OS. The only way was to forcefully kill the process, which is neither user friendly, nor does it properly close the databases, and in the worst case it could result in some corruption.
Closing the QT splash screen window, or killing the process, triggers a global (Bitcoin Core based) shutdown request, which is honored by
msc_initial_scan().6) Low-hanging fruits on the way:
After @zathras-crypto gave green light for the initial merge, I started to wonder how to address any potential merge conflicts, and I finally came to the conclusion that it's probably better to bundle somewhat related changes, to reduce the impact on other pull requests.
While I generally think changes should come in smaller packets, the gain seemed to outweight the cost in this case, and in particular I'm referring to the last three commits 79749e9, f07a23d and 93eae99, which are hopefully easy to review and basically without any real impact.
7) No merge conflicts:
I actually got away with creating none for the pending pull requests.
The UI PR #5 can be merged directly, and #13 can be merged without conflicts, after merging zathras-crypto#63.
8) Let's see some results of the updated initialization and status reports... :)