Shutdown on blockcount RPC failure and other cases cleanly #693
Conversation
Prior to this commit, in case an RPC failure occurred when accesing the block height, the program would continue but the wallet would be in an un-writeable state (for command line programs, specifically yield generators; for Qt the shutdown would occur). This commit slightly cleans up the process of shutting down, ensuring that duplicate shutdown calls do not result in stack traces. It also ensures that also for command line programs, the application will immediately shutdown if the regular heartbeat call to query the block height fails, as this risks inconsistencies in the wallet (though the previous situation luckily did not result in this as the call to BaseWallet.close() resulted in the wallet being read only). A future PR should develop a more sophisticated approach to RPC call failures that may allow the program to wait. stopservice
|
As was discussed on #673 this is not a full solution, but just cleaning up what we already had (my above verbose commit comment explains this a bit more) and actually ensuring a shutdown for RPC failures which we cannot handle (please don't forget that basic broken-pipe or failures are actually dealt with, including 100 retries .. this must be some kind of exceptional situation). instead of previous bizarre "wallet is read-only". A full solution would be a much more extensive PR involving having clients keep track of the state of the walletservice, which is somehow signalling that it is in a "freeze" mode, waiting/hoping that the RPC comes back up or corrects itself (?) A detail that was missing in #673 was - what is the exact failure of RPC encountered? I'm wondering if it wasn't an actual error code being returned by the |
|
Tests pass, but there's now bad error handling when trying Before this PR: After: |
Also manually fire order creation in coinjoin tests. This clarification and test change is required due to the fact that LoopingCalls are designed to fire immediately by default, before the reactor is initialized (and therefore in a `running` state), making it not possible to shutdown the reactor as a result of events happening in that first call; so we delay the first call of the maker's orderbook populating code, so that if a no-coins error occurs, it will actually shut down the reactor and hence the whole yield generator program, as intended.
AdamISZ
force-pushed
the
shutdown-blockheight-failure
branch
from
e2e9990
to
202f8ee
Compare
|
Oh thanks for catching that, stupid bug. Fixed in 202f8ee (just imported |
|
Did another test where the old behaviour was better than the new one. It's 1) start Bitcoin Core, 2) start JoinMarketQt, 3) shutdown Bitcoin Core, 4) try opening wallet in JoinMarketQt. Before this PR: After, hangs up in this state: With this stack trace after you close Qt app: |
|
This is dumb, but I've just spent over an hour looking at this, and I can't even replicate your first result; the dialog box with "Walletservice failed to start ..." does not appear for me, the Exception is not caught. I believe I am doing the exact same thing, on master, i.e.: start Core, start joinmarketqt, stop core, load a wallet via load->wallet. for me it just outputs to terminal: ... and Qt does not close (i.e. closeEvent is not fired). Again, to emphasize, this was on master, i.e. i was trying to replicate the first of the two results you just showed. |
|
OK came back to it today, immediately obvious what the problem was: regtest code is in an |
|
As of 5af2d49 : So, the above issue raised by @kristapsk makes sense after more careful thought: the problem is that the failure is triggered in the constructor of the In this PR, I removed that general Exception in favour of a reactor shutdown, but this allowed the Qt code to continue with the remaining code in So here after this new commit, we instead signal, inside the Testing: I have tested: command line maker quits with proper error message if no coins, and also does so if the bitcoind shuts down during running (so a recheck of the above), check that the case illustrated by @kristapsk above now prints error dialog in GUI, then quits app, Checked also that in case there is no bitcoind error, the wallet load and startup proceed normally in Qt. |
|
Just noticed another not so nice error message in this PR vs master. To reproduce: 1) start Bitcoin Core (on testnet), 2) run wallet-tool.py wallet.jmdat, 3) wait for password prompt, 4) shutdown Bitcoin Core, 5) enter wallet password. Master: This PR: |
|
OK; so this is a pretty obscure case (one could argue the previous one was too); the error you're seeing there happens for the same reason as for the previous one, I can fix it up the same way (and I will) but it's worth noting this is really pretty minor and a very unlikely edge case - "minor" because basically the program crashes either way, it just crashes more messily now. |
|
Did some more testing and haven't found any new issues. |
|
Thanks for the thorough checks. |
See commit comments.
Testing: passes test suite (after second commit), and I get sensible error message shutdowns without stack traces (but still with usual verbose IRC messages), both on Qt (with dialog box) and on command line in cases: (a) RPC call for
getblockcountfails (I garbled the method name) and (b) RPC connection lost entirely (I shutdown the bitcoin daemon).