init: Use InitError for all errors in bitcoind/qt #16366

MarcoFalke commented at 1:25 PM on July 10, 2019: member

Using the same InitError for startup error in the daemon and the gui makes it possible to run the tests with the gui again:

BITCOIND=bitcoin-qt ./test/functional/test_runner.py feature_includeconf feature_config_args

MarcoFalke force-pushed on Jul 10, 2019

DrahtBot added the label GUI on Jul 10, 2019

DrahtBot added the label Tests on Jul 10, 2019

DrahtBot commented at 3:59 PM on July 10, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16362 (gui: Bilingual translation by hebasto)
#16273 (refactor: Remove unused includes by practicalswift)
#16224 (gui: Bilingual GUI error messages by hebasto)
#15864 (Fix datadir handling by hebasto)
#14866 (Improve property evaluation way in bitcoin.conf by AkioNak)
#13389 (Utils and libraries: Fix #13371 - move umask operation earlier in AppInit() by n2yen)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

ryanofsky approved

ryanofsky commented at 4:06 PM on July 10, 2019: member

utACK fa6647ae68ca15d0618c5b7a81eff78b1d81d26e. I wonder if it'd be possible to run tests with BITCOIND=bitcoin-qt QT_QPA_PLATFORM=minimal on travis to catch more regressions.

hebasto commented at 4:49 PM on July 10, 2019: member

Concept ACK.

Usually, running self-compiled bitcoin-qt on Linux Mint 19.1 I get a message:

$ ./src/qt/bitcoin-qt
qt5ct: using qt5ct plugin

So local run tests on my environment returns errors:

$ BITCOIND=bitcoin-qt ./test/functional/test_runner.py feature_includeconf feature_config_args
Temporary test directory at /tmp/test_runner_₿_🏃_20190710_194358
Traceback (most recent call last):
  File "/home/hebasto/github/bitcoin/test/functional/create_cache.py", line 28, in <module>
    CreateCache().main()
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_framework.py", line 218, in main
    self.stop_nodes()
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_framework.py", line 397, in stop_nodes
    node.stop_node(wait=wait)
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_node.py", line 276, in stop_node
    raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
AssertionError: Unexpected stderr qt5ct: using qt5ct plugin != 
2019-07-10T16:43:58.707000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20190710_194358/cache
2019-07-10T16:43:59.616000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_framework.py", line 191, in main
    self.setup_chain()
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_framework.py", line 274, in setup_chain
    self._initialize_chain()
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_framework.py", line 514, in _initialize_chain
    self.stop_nodes()
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_framework.py", line 397, in stop_nodes
    node.stop_node(wait=wait)
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_node.py", line 276, in stop_node
    raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
AssertionError: Unexpected stderr qt5ct: using qt5ct plugin != 
2019-07-10T16:43:59.667000Z TestFramework (INFO): Stopping nodes
[node 0] Cleaning up leftover process
Traceback (most recent call last):
  File "./test/functional/test_runner.py", line 661, in <module>
    main()
  File "./test/functional/test_runner.py", line 324, in main
    runs_ci=args.ci,
  File "./test/functional/test_runner.py", line 356, in run_tests
    subprocess.check_output([sys.executable, tests_dir + 'create_cache.py'] + flags + ["--tmpdir=%s/cache" % tmpdir])
  File "/usr/lib/python3.6/subprocess.py", line 356, in check_output
    **kwargs).stdout
  File "/usr/lib/python3.6/subprocess.py", line 438, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['/usr/bin/python3', '/home/hebasto/github/bitcoin/test/functional/create_cache.py', '--cachedir=/home/hebasto/github/bitcoin/test/cache', '--configfile=/home/hebasto/github/bitcoin/test/functional/../config.ini', '--tmpdir=/tmp/test_runner_\u20bf_🏃_20190710_194358/cache']' returned non-zero exit status 1.

in src/bitcoind.cpp:147 in fa6647ae68 outdated

 147 |  
 148 |              // Daemonize
 149 |              if (daemon(1, 0)) { // don't chdir (1), do close FDs (0)
 150 | -                tfm::format(std::cerr, "Error: daemon() failed: %s\n", strerror(errno));
 151 | -                return false;
 152 | +                return InitError(strprintf("daemon() failed: %s\n", strerror(errno)));

hebasto commented at 4:53 PM on July 10, 2019:

                return InitError(strprintf("%s daemon failed: %s\n", PACKAGE_NAME, strerror(errno)));

MarcoFalke commented at 5:56 PM on July 10, 2019:

I think it really means that deamon() failed, so I'd prefer to keep the (,).

hebasto commented at 7:19 AM on July 11, 2019:

Agree.

in src/bitcoind.cpp:143 in fa6647ae68 outdated

 141 | @@ -146,19 +142,17 @@ static bool AppInit(int argc, char* argv[])
 142 |  #pragma GCC diagnostic push
 143 |  #pragma GCC diagnostic ignored "-Wdeprecated-declarations"
 144 |  #endif
 145 | -            tfm::format(std::cout, "Bitcoin server starting\n");
 146 | +            tfm::format(std::cout, PACKAGE_NAME "daemon starting\n");

laanwj commented at 3:23 PM on July 11, 2019:

I don't think 'daemon' is right here when it's not started in -daemon mode

MarcoFalke commented at 3:28 PM on July 11, 2019:

This print is guarded by a DAEMON compile time check and -daemon run time check, so it should be all right, no?

laanwj commented at 3:30 PM on July 11, 2019: member

InitError was supposed to only be called from the AppInit* functions, to make sure it isn't called before the appropriate listeners are installed. It seems I lost track of how the initialization process works though.

ryanofsky commented at 3:41 PM on July 11, 2019: member

to make sure it isn't called before the appropriate listeners are installed @laanwj what are "listeners" here? I don't understand what this could be referring to.

laanwj commented at 3:54 PM on July 11, 2019: member

@laanwj what are "listeners" here? I don't understand what this could be referring to.

uiInterface signal listeners etc

MarcoFalke commented at 4:06 PM on July 11, 2019: member

In bitcoind.cpp, I only modify the AppInit function, so the change is obviously correct there.

However, the gui uses a self-brewed AppInit*, I think. One of the first things in GuiMain is to // Subscribe to global signals from core. So it should be fine as well.

I'd presume it would be more difficult to rewrite the Gui init to call the same AppInit function as bitcoind does, so I'd rather not do that.

in src/qt/bitcoin.cpp:462 in fa6647ae68 outdated

 458 | @@ -459,8 +459,11 @@ int GuiMain(int argc, char* argv[])
 459 |      SetupUIArgs();
 460 |      std::string error;
 461 |      if (!node->parseParameters(argc, argv, error)) {
 462 | +        InitError(strprintf("Error parsing command line arguments: %s\n", error));
 463 | +        // Create a message box, because the gui has neither been created nor has subscribed to core signals
 464 |          QMessageBox::critical(nullptr, PACKAGE_NAME,
 465 | -            QObject::tr("Error parsing command line arguments: %1.").arg(QString::fromStdString(error)));
 466 | +            // message can not be translated because translations have not been initialized

ryanofsky commented at 6:46 PM on July 11, 2019:

Curious what QObject::tr was doing previously. Just passing the string untranslated?

MarcoFalke commented at 8:09 PM on July 11, 2019:

Yes, it would be picked up and sent to transifex for translation, but the method would only convert the std::string to a QString, I believe.

in src/qt/bitcoin.cpp:462 in fa6647ae68 outdated

 458 | @@ -459,8 +459,11 @@ int GuiMain(int argc, char* argv[])
 459 |      SetupUIArgs();
 460 |      std::string error;
 461 |      if (!node->parseParameters(argc, argv, error)) {
 462 | +        InitError(strprintf("Error parsing command line arguments: %s\n", error));

ryanofsky commented at 7:27 PM on July 11, 2019:

Just a suggestion, but it would make sense to change InitError() to node->initError() here and below, adding a new Node::initError() method like existing Chain::initError() and ChainImpl::initError() methods.

This would prevent error logs being dropped with #10102. Calling InitError in the bitcoin-gui process there can't have any effect because InitError uses global variables initialized in the bitcoin-node process. (Ideally the InitError function wouldn't even be accessible to gui code, but there is no enforcement yet for GUI code not accessing node globals currently, unlike the enforcement for wallet code not accessing node globals that was added in 78a2fb55c97fbc26f7b74c5b1fb999a2aff8ce88.)

Feel free to ignore this suggestion if it is confusing or too much work, since I can clean this up later.

MarcoFalke commented at 9:02 PM on July 11, 2019:

Excellent suggestion, but I am not sure how to implement this cleanly. We have the ./src/node and ./src/interface subdirectories, so it seems I'd have to split the global uiInterface+InitError out from ui_interface into ./src/node/ui and move the remainder of ui_interface into ./src/interface/ui?

I am happy to cherry-pick something if you code this up.

ryanofsky commented at 10:19 PM on July 11, 2019:

76b1c671f97a97e21326715c8ea21f5584ab5111 (https://github.com/ryanofsky/bitcoin/commits/pr/initerror) is the suggestion. I might not have described it clearly before.

MarcoFalke commented at 11:41 PM on July 11, 2019:

Thx. Cherry-picked into this branch.

in src/qt/bitcoin.cpp:460 in fa6647ae68 outdated

 458 | @@ -459,8 +459,11 @@ int GuiMain(int argc, char* argv[])
 459 |      SetupUIArgs();
 460 |      std::string error;
 461 |      if (!node->parseParameters(argc, argv, error)) {
 462 | +        InitError(strprintf("Error parsing command line arguments: %s\n", error));
 463 | +        // Create a message box, because the gui has neither been created nor has subscribed to core signals

ryanofsky commented at 7:42 PM on July 11, 2019:

It might be good to note in the comment on line 427 that the handleMessageBox call there is necessary for these InitError calls below to work:

https://github.com/bitcoin/bitcoin/blob/4fcccdac785e09ad5627b3bf4811dfba353693e8/src/qt/bitcoin.cpp#L427-L428

It'd also be possible to ensure handlers were properly registered inside InitError by adding an assert there:

assert(g_ui_signals.ThreadSafeMessageBox.num_slots() > 0);

https://github.com/bitcoin/bitcoin/blob/4fcccdac785e09ad5627b3bf4811dfba353693e8/src/ui_interface.cpp#L58-L62

MarcoFalke commented at 8:43 PM on July 11, 2019:

How would that add any value? Boost already has this assertion built in:

terminate called after throwing an instance of 'boost::wrapexcept<boost::signals2::no_slots_error>'
  what():  boost::signals2::no_slots_error

ryanofsky commented at 10:07 PM on July 11, 2019:

Oh, I didn't realize ThreadSafeMessageBox uses last_value<bool> so an exception will already be thrown. In this case the assert would be redundant.

https://www.boost.org/doc/libs/1_70_0/doc/html/boost/signals2/no_slots_error.html https://www.boost.org/doc/libs/1_70_0/doc/html/boost/signals2/last_value.html https://www.boost.org/doc/libs/1_70_0/doc/html/boost/signals2/last_valu_1_3_37_6_5_1_1_2.html

ryanofsky approved

ryanofsky commented at 7:59 PM on July 11, 2019: member

ACK fa6647ae68ca15d0618c5b7a81eff78b1d81d26e again. No changes since last review, I just dug into this a little more. I have some suggestions and questions below, but they are not too important, so feel free to ignore them.

init: Use InitError for all errors in bitcoind/qt

Also, remove unused <boost/thread.hpp> include (and others)

fad2502240

Call node->initError instead of InitError from GUI code

Avoids GUI code calling a node function, and having to live in the same process
as g_ui_signals and uiInterface global variables.

fa6f402bde

MarcoFalke force-pushed on Jul 11, 2019

hebasto commented at 12:10 PM on July 13, 2019: member

ACK fa6f402bde146f92ed131e0c9c8e15a55e723307

Tested on Linux Mint 19.1 with system Qt 5.9.5.

The "using qt5ct plugin" message that is printed to stderr by qt5ct can be suppressed by the following environment variable:

QT_LOGGING_RULES="qt5ct.debug=false"

Using this knowledge, feature_includeconf.py passed:

$ BITCOIND=bitcoin-qt QT_LOGGING_RULES="qt5ct.debug=false" ./test/functional/test_runner.py feature_includeconf
Temporary test directory at /tmp/test_runner_₿_🏃_20190713_150236
Remaining jobs: [feature_includeconf.py]
1/1 - feature_includeconf.py passed, Duration: 10 s

TEST                   | STATUS    | DURATION

feature_includeconf.py | ✓ Passed  | 10 s

ALL                    | ✓ Passed  | 10 s (accumulated) 
Runtime: 10 s

And feature_config_args.py passed too:

$ BITCOIND=bitcoin-qt QT_LOGGING_RULES="qt5ct.debug=false" ./test/functional/test_runner.py feature_config_args
Temporary test directory at /tmp/test_runner_₿_🏃_20190713_150609
Remaining jobs: [feature_config_args.py]
1/1 - feature_config_args.py passed, Duration: 17 s

TEST                   | STATUS    | DURATION

feature_config_args.py | ✓ Passed  | 17 s

ALL                    | ✓ Passed  | 17 s (accumulated) 
Runtime: 17 s

@MarcoFalke Could you add QT_LOGGING_RULES="qt5ct.debug=false" to the OP?

MarcoFalke requested review from laanwj on Jul 15, 2019

MarcoFalke commented at 1:24 PM on July 15, 2019: member

Could you add QT_LOGGING_RULES="qt5ct.debug=false" to the OP?

This is specific to your environment. If I add that, I would also have to add XDG_SESSION_TYPE=x11, because someone might be running on Wayland with Gnome and get the Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome.)

MarcoFalke added this to the milestone 0.19.0 on Jul 15, 2019

ryanofsky approved

ryanofsky commented at 5:18 PM on July 15, 2019: member

utACK fa6f402bde146f92ed131e0c9c8e15a55e723307. Only changes since last review are removing more includes and adding Node::initError method to avoid accessing node InitError function and global variables from GUI code.

MarcoFalke commented at 12:25 PM on July 22, 2019: member

This pull will be merged some time this week, unless there are objections.

MarcoFalke commented at 10:40 PM on July 23, 2019: member

Going to merge this, so that the tests pass with the gui

MarcoFalke merged this on Jul 23, 2019

MarcoFalke closed this on Jul 23, 2019

MarcoFalke referenced this in commit 67923d6b3c on Jul 23, 2019

MarcoFalke deleted the branch on Jul 23, 2019

deadalnix referenced this in commit c5a632bec6 on Apr 22, 2020

PastaPastaPasta referenced this in commit fe14f50beb on Oct 22, 2021

DrahtBot locked this on Dec 16, 2021