Skip to content

Conversation

@laanwj
Copy link
Member

@laanwj laanwj commented Mar 25, 2016

Replace the bitcoin-cli -rpcwait after spawning bitcoind with our own loop that detects when bitcoind exits prematurely (before RPC is up).

This prevents a hang in such a case (see #7463).

@laanwj laanwj force-pushed the 2016_03_detect_startup_failure branch 2 times, most recently from f22f8ed to 8869c3b Compare March 25, 2016 13:31
@laanwj
Copy link
Member Author

laanwj commented Mar 25, 2016

Not ready to merge yet. I've tested this with a -salvagewallet loop:

Unexpected exception caught during testing: bitcoind exited with status 1 during initialization
  File "/data/src/bitcoin/qa/rpc-tests/test_framework/test_framework.py", line 135, in main
    self.run_test()
  File "./wallet.py", line 295, in run_test
    self.nodes = start_nodes(3, self.options.tmpdir, [[m]] * 3)
  File "/data/src/bitcoin/qa/rpc-tests/test_framework/util.py", line 283, in start_nodes
    return [ start_node(i, dirname, extra_args[i], rpchost, binary=binary[i]) for i in range(num_nodes) ]
  File "/data/src/bitcoin/qa/rpc-tests/test_framework/util.py", line 267, in start_node
    wait_for_bitcoind_start(bitcoind_processes[i], url, i)
  File "/data/src/bitcoin/qa/rpc-tests/test_framework/util.py", line 144, in wait_for_bitcoind_start
    raise Exception('bitcoind exited with status %i during initialization' % process.returncode)
Stopping nodes

Even though reporting is better, it still hangs after "Stopping nodes", going to investigate why.

@laanwj laanwj force-pushed the 2016_03_detect_startup_failure branch 2 times, most recently from d4066d4 to 9c28f02 Compare March 25, 2016 16:19
@laanwj
Copy link
Member Author

laanwj commented Mar 25, 2016

Should be fixed now. The problem was that start_nodes can fail with part of the nodes already started.
wait_bitcoinds will subsequently wait forever for them to exit (as it uses a global variable to communicate bitcoind_processes).
One-to-last commit makes sure that they're stopped properly in that case.

Replace the `bitcoin-cli -rpcwait` after spawning bitcoind
with our own loop that detects when bitcoind exits prematurely.

And if one node fails to start, stop the others.

This prevents a hang in such a case (see bitcoin#7463).
@laanwj laanwj force-pushed the 2016_03_detect_startup_failure branch from 9c28f02 to 018b60c Compare March 26, 2016 07:11
@laanwj
Copy link
Member Author

laanwj commented Mar 26, 2016

This is faster than the current -rpcwait construction, so #7745 triggers more, seems I need to solve that too here.
Thanks Marco Falke for fixing this already.

try:
for i in range(num_nodes):
rpcs.append(start_node(i, dirname, extra_args[i], rpchost, binary=binary[i]))
except: # If one node failed to start, stop the others
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@jonasschnelli
Copy link
Contributor

Good idea!
utACK 018b60c

@maflcko
Copy link
Member

maflcko commented Mar 29, 2016

utACK 018b60c

@laanwj laanwj merged commit 018b60c into bitcoin:master Mar 29, 2016
laanwj added a commit that referenced this pull request Mar 29, 2016
018b60c test_framework: detect failure of bitcoind startup (Wladimir J. van der Laan)
return datadir

def rpc_url(i, rpchost=None):
return "http://rt:rt@%s:%d" % (rpchost or '127.0.0.1', rpc_port(i))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: rpcbind_test.py no longer works after this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably re-add that test to one of the standard sequence, otherwise it will just bitrot

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jun 9, 2016
ftrader added a commit to ftrader-bitcoinunlimited/hardfork_prototype_1_mvf-bu that referenced this pull request Dec 9, 2016
This is a merge of bitcoin/bitcoin#7744 / MVF-Core PR#16.

Instead of freezing up qa tests, wallet.py will fail on some 32-bit platforms where wallet salvage operation sometimes fails due to unknown reasons.

See also: bitcoin/bitcoin#7463 about the -salvagewallet problem.

Core has decided to disable the salvage part of the test in bitcoin/bitcoin#8038 , we may implement a Skip() of this test on 32-bit or disable that test step in a similar way.
The wallet salvage problem is definitely unrelated to any fork programming.
zkbot added a commit to zcash/zcash that referenced this pull request Mar 24, 2020
Backport RPC test harness PRs

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#6548
- bitcoin/bitcoin#6804
  - Just the coverage backend, not the flag to enable it.
- bitcoin/bitcoin#7744
- bitcoin/bitcoin#9832
  - Excludes `wallet-hd.py` change (missing bitcoin/bitcoin#8309).

Part of #2074.
zkbot added a commit to zcash/zcash that referenced this pull request Apr 3, 2020
Backport RPC test harness PRs

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#6548
- bitcoin/bitcoin#6804
  - Just the coverage backend, not the flag to enable it for all RPC tests.
- bitcoin/bitcoin#7744
- bitcoin/bitcoin#9832
  - Excludes `wallet-hd.py` change (missing bitcoin/bitcoin#8309).

Part of #2074.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants