util, gui: Fix shutdown segfault and repair broken overview page staking status #1901
Conversation
GRC::Initialize must be called as part of StartNode(), which creates a thread group netThreads forked from a thread in the global threads group in init. The scraper components are run in the netThread groups as part of this, since they share data structures with scraper_net, and the collapse of the threads has to be coordinated.
The removal of mvTimers, Timer, and timerfire() broke the global status update for the overview page. This reimplements the periodic status update using a QTimer.
There was a problem hiding this comment.
I can't reproduce the segfault, but this looks fine. We will probably want to address the underlying problem that caused it soon since the thread model will change with a newer Bitcoin base. They share some data structures with the message processing thread, but the scraper housekeeping threads do not handle any net messaging functionality directly.
My intention was to create a small stepping stone. Gridcoin—and, quite frankly, Bitcoin as well—rely so heavily on assumptions about the compiler-generated code for the lifecycle of global state that it becomes very difficult to reason about inter-module dependencies, concurrency, and object lifetimes. The Fern release lets us breathe now, so we may want to take time to redesign the architecture properly and reorganize some of the spaghetti that relies on global variables to avoid issues like this.
| void BitcoinGUI::updateGlobalStatus() | ||
| { | ||
| // This is needed to prevent segfaulting due to early GUI initialization compared to core. | ||
| if (miner_first_pass_complete) | ||
| { | ||
| try | ||
| { | ||
| GetGlobalStatus(); | ||
| overviewPage->updateglobalstatus(); | ||
| setNumConnections(clientModel->getNumConnections()); | ||
| } | ||
| catch(std::runtime_error &e) | ||
| { | ||
| LogPrintf("GENERAL RUNTIME ERROR!"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I finally removed the ridiculous GlobalStatusStruct and related code for an upcoming PR. Should be fine to merge this in the meantime, though.
There was a problem hiding this comment.
Yep. This probably creates more work for you in the interim, but it was pretty simple to get it to work again.
|
I am still getting it every once and a while, although it is better. We definitely have a lot of work to do on the global state. Pretty fragile stuff, especially on init and shutdown... |
|
I am going to go ahead and merge, and then we can work on straightening global state and the thread pools in a future PR while we have breathing room. This should be similar in behavior to the Fern release code now, but large scale global init is very tricky as you know and quite a few things have moved around in the gridcoin refactoring. |
…cyrossignol) - refactor: port chainparams #1878 (@div72) - gui: Update default font to Inter-Regular and console font to Inconsolata (@opsinphark, @jamescowens) - gui: Add "review beacon verification" button to wizard summary page #1912 (@cyrossignol) - rpc, wallet: Implement liststakes #1909 (@jamescowens) - rpc: Add "getlaststake" RPC function #1913 (@cyrossignol) - gui: Install bold variant of Inter font #1914 (@cyrossignol) - refactor: Consolidate Gridcoin-specific code #1894 (@cyrossignol) - script: Setup improvements #1895 (@nathanielcwm) - gui: Diagnostics refresh #1899 (@jamescowens) - superblock: Optimize superblock size calculation #1906 (@cyrossignol) - gui: Adjust stylesheets and scale icons to improve HiDPI side toolbar display #1911 (@jamescowens) - doc: Tell user to disable win32 application support in WSL (for building) #1917 (@nathanielcwm) - rpc: Revise and expand help for beaconconvergence rpc call #1918 (@jamescowens) - scheduler: Increase default update check interval to 5 days #1920 (@cyrossignol) - gui: Prevent multiple dialogs from being open at the same time #1922 (@scribblemaniac) - refactor: Clean up remaining legacy timer code #1892 (@cyrossignol) - build: Add --without-brotli option to curl.mk #1902 (@G_UK) - test: Remove fs_tests... file after the fs test #1903 (@div72) - util, gui: Fix shutdown segfault and repair broken overview page staking status #1901 (@jamescowens) - scraper: Fix order of destruction for global scraper objects #1904 (@cyrossignol) - scraper: Fix global object destruction order for MacOS #1905 (@cyrossignol) - util: Decouple out-of-sync state from block acceptance #1921 (@cyrossignol)
This fixes a detail that I missed for gridcoin-community#1901. It ensures that snapshot accrual reloads stored accrual for CPIDs that never staked a block before when applying a snapshot from disk.
#1894 changed the thread group for the scraper and caused a segfault to occur on shutdown, probably due to the shared data structures across the scraper and scraper_net (which really lives in the net code). This restores the scraper to the net thread group.
This PR also repairs the broken overview page staking status, which was broken due to the removal of mvTimers and associated functions. The periodic update functionality is restored via a QTimer approach.