test: add wait_for_cookie_credentials() to framework for rpcwait tests #18691

This PR adds a wait_for_cookie_credentials() method to the test framework and calls it before the -rpcwait tests, to avoid an intermittent race condition on the CI run with Valgrind where the cookie file isn't written yet when the CLI call with -rpcwait arrives to get_auth_cookie().

To reproduce/test, build with

diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 60c4d06f12..3dd06c4758 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -291,6 +291,7 @@ static bool InitRPCAuthentication()
 bool StartHTTPRPC()
 {
     LogPrint(BCLog::RPC, "Starting HTTP RPC server\n");
+    UninterruptibleSleep(std::chrono::seconds{11});
     if (!InitRPCAuthentication())

then run the test normally and with valgrind

test/functional/interface_bitcoin_cli.py -l debug
valgrind test/functional/interface_bitcoin_cli.py -l debug

Thanks to Marco Falke for all the help. Closes #18684.

The error was " Called Process failed with 'error: Could not locate RPC credentials. No authentication cookie could be found, and RPC password is not set. See -rpcpassword and -stdinrpcpass. Configuration file: (/home/travis/build/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20200417_132645/interface_bitcoin_cli_109/node0/bitcoin.conf) "

Yes. I'm still looking at it, but since valgrind runs don't work out-of-repo, I wanted to see if the valgrind CI passes with this change while looking further.

get_auth_cookie is used by every single test. It reads the auth cookie from disk, though it fails when the file was not yet created.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #18594 (cli: display multiwallet balances in -getinfo by jonatack)
• #18453 (rpc, cli: add multiwallet balances rpc and use it in -getinfo by jonatack)

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.

Btw, this happens on cirrus reliably: https://cirrus-ci.com/task/6597545033990144?command=functional_test#L1053

Or, if you want to test locally:

diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 60c4d06f12..3dd06c4758 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -291,6 +291,7 @@ static bool InitRPCAuthentication()
 bool StartHTTPRPC()
 {
     LogPrint(BCLog::RPC, "Starting HTTP RPC server\n");
+    UninterruptibleSleep(std::chrono::seconds{11});
     if (!InitRPCAuthentication())
         return false;
 

Thanks @MarcoFalke for your suggestions! Being able to reproduce makes all the difference. I updated the PR title and description and pushed a fix that seems to work.

I see that the CI for the latest merge on master (bump wallet_import timeout) failed for this same reason: https://travis-ci.org/github/bitcoin/bitcoin/jobs/676639823

Simplified.

Edit: FWIW I considered adding the following but don't think it's necessary? IIUC wait_for_cookie_credentials shouldn't see any other type of ValueError, nor ECONNREFUSED.

+            except OSError as e:
+                if e.errno != errno.ECONNREFUSED:  # Port not yet open?
+                    raise  # Unknown OS error
-            except ValueError:
-                pass
+            except ValueError as e:  # Cookie file not found and no rpcuser or rpcassword; bitcoind still starting.
+                if "No RPC credentials" not in str(e):
+                    raise

ACK 83b037921fac34d4c85ce6a2e137407596fed91e

Happy to merge with or without feedback addressed. Just let me know what you prefer.

Took the suggestions.

Rebased and added a docstring to wait_for_cookie_credentials().