qt: Do not block GUI thread in RPCConsole #17659

1. 
   hebasto commented at 8:42 pm on December 3, 2019: member

   On master (35eda631ed3bd23d4a41761a85a96f925d4a6337) the GUI thread is blocked with QThread::wait() during bitcoin-qt shutdown routine. This causes unresponsive GUI if some commands are passed to the RPC console (#13217) before shutdown initiating.

   This PR:

   • removes blocking call and uses additional signal-to-slot connections.
   • makes bitcoin-qt shutdown routine more streamlined: the only QApplication::exec() is used in bitcoin-qt. Therefore, the main event loop never interrupts until shutdown, unlike master and alternative #13674.

   Refs:

   • #10504
   • #17145

   This PR is an alternative to #13674.

   Fix #13217

2. fanquake added the label GUI on Dec 3, 2019
3. 
   ryanofsky commented at 9:49 pm on December 3, 2019: member

   Fix #13217; this is an alternative to #13674 Fix #17495

   As a side effect, this PR makes bitcoin-qt shutdown routine more streamlined.

   Refs:

   • #10504
   • #17145

   Could you update this with a description of what this PR does? Useful information to include:

   • details about how behavior changed, what behavior was before and after this change
   • a short description of the code changes, what was wrong with the old code, and how the commits relate to each other
   • how this compares to the alternative pr

   It’s great to link to other issues and pull requests in a PR description for extra details, but it shouldn’t be necessary to open multiple other issues just to have a basic understanding of what a PR does and what motivated it

4. 
   DrahtBot commented at 0:10 am on December 4, 2019: member

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

   Conflicts

   No conflicts as of last run.

5. 
   promag commented at 1:09 am on December 4, 2019: member

   Calling waitfornewblock in the GUI console returns immediately.
6. 
   hebasto commented at 3:51 am on December 4, 2019: member

   Calling waitfornewblock in the GUI console returns immediately.

   I have observed such behavior on regtest only once, and cannot reproduce anymore.

   Could you provide steps to reproduce an issue reliably?

7. 
   hebasto commented at 10:12 am on December 4, 2019: member

   @ryanofsky

   OP has been updated.

8. 
   promag commented at 2:12 pm on December 4, 2019: member

   @hebasto actually it’s unrelated to this change. Just noticed that waitfornewblock returns immediately in the GUI console because IsRPCRunning is false unless you start with -server.
9. 
   promag commented at 9:23 pm on December 15, 2019: member

   The folllowing still hangs:

   • bitcoin-qt -regtest -server
   • open gui console
   • run waitfornewblock
   • try to quit

   Are you able to reproduce?

10. 
    hebasto commented at 8:06 am on December 16, 2019: member

    @promag

    The folllowing still hangs:

    * `bitcoin-qt -regtest -server`
    
    * open gui console
    
    * run `waitfornewblock`
    
    * try to quit
    

    This requires a forced interruption of the RPC thread. It seems a bit out of the scope of this PR, as it solves the blocking of the GUI thread only.

    In your example bitcoin-qt literally “waits for new block”, which is unreachable in regtest after quit is initiated ;) But this PR works on mainnet and testnet: bitcoin-qt quits just after a new block arrives.

    Maybe, to mention in OP that #17495 is fixed, is wrong, no?

11. 
    promag commented at 1:01 am on February 3, 2020: member

    @hebasto maybe just mention it’s not a complete fix.
12. 
    hebasto commented at 6:49 pm on March 27, 2020: member

    @promag

    @hebasto maybe just mention it’s not a complete fix.

    The mention about the fix of #17495 has been removed from PR description.

    The actual fix of #17495 is #18452 ;)

13. DrahtBot added the label Needs rebase on May 8, 2020
14. hebasto force-pushed on May 9, 2020
15. 
    hebasto commented at 4:15 am on May 9, 2020: member

    Rebased 485e3dea08d291f1b32ba94e09ee793440f4daec -> e7ee445b9191d37505c8711115c5ef85c86ba31f (pr17659.01 -> pr17659.02) due to the conflict with #16224.
16. DrahtBot removed the label Needs rebase on May 9, 2020
17. 
    MarcoFalke commented at 11:40 am on May 23, 2020: member

    Nice. Concept ACK
18. DrahtBot added the label Needs rebase on May 29, 2020
19. 
    jonasschnelli commented at 8:15 am on May 29, 2020: contributor

    Concept ACK. Needs rebase.
20. hebasto force-pushed on May 29, 2020
21. 
    hebasto commented at 9:34 am on May 29, 2020: member

    Rebased e7ee445b9191d37505c8711115c5ef85c86ba31f -> 776d15763ea1551d7a5a86be29cfcd125d173d9d (pr17659.02 -> pr17659.03) due to the conflict with #16432.
22. DrahtBot removed the label Needs rebase on May 29, 2020
23. DrahtBot added the label Needs rebase on Aug 13, 2020
24. qt: Add BitcoinGUI::clearGUI() ef23d25d6d
25. qt: Use BitcoinGUI::clearGUI() e0b15a0816
26. qt: Make requestShutdown() a slot e2d0fc815f
27. qt: Factor out requestNodeShutdown() slot 9495d26b2e
28. qt: Do not block GUI thread in RPCConsole 47912c505e
29. hebasto force-pushed on Aug 13, 2020
30. 
    hebasto commented at 2:41 pm on August 13, 2020: member

    Rebased 776d15763ea1551d7a5a86be29cfcd125d173d9d -> 47912c505eebc79c849cf14fcf9d2654caa0945a (pr17659.03 -> pr17659.04) due to the conflict with #19011.
31. DrahtBot removed the label Needs rebase on Aug 13, 2020
32. 
    fanquake commented at 6:51 am on August 14, 2020: member

    Lets move this over to the GUI repo. I’ve also ported the original issue there now as well, given that the non-gui part has been fixed.
33. fanquake closed this on Aug 14, 2020

    ---

34. hebasto deleted the branch on Aug 14, 2020
35. 
    hebasto commented at 10:41 am on August 14, 2020: member

    Moved to https://github.com/bitcoin-core/gui/pull/59.
36. DrahtBot locked this on Feb 15, 2022

Contributors
hebasto ryanofsky DrahtBot promag MarcoFalke jonasschnelli fanquake

Labels
GUI