From a2aafd254d07d97166ed4f3a549238612b316dd2 Mon Sep 17 00:00:00 2001 From: Adam Gibson Date: Thu, 1 Oct 2020 12:58:04 +0100 Subject: [PATCH 1/4] Fixes #673. Shutdown cleanly on failure to access blockheight 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 --- jmbase/jmbase/__init__.py | 1 + jmbase/jmbase/twisted_utils.py | 18 ++++++++++++++++++ jmclient/jmclient/blockchaininterface.py | 14 +++++++++++--- jmclient/jmclient/maker.py | 6 +++--- jmclient/jmclient/wallet_service.py | 18 +++++++++++++++--- scripts/joinmarket-qt.py | 5 ++--- 6 files changed, 50 insertions(+), 12 deletions(-) create mode 100644 jmbase/jmbase/twisted_utils.py diff --git a/jmbase/jmbase/__init__.py b/jmbase/jmbase/__init__.py index e49ea0a..e071f11 100644 --- a/jmbase/jmbase/__init__.py +++ b/jmbase/jmbase/__init__.py @@ -7,6 +7,7 @@ from .support import (get_log, chunks, debug_silence, jmprint, utxo_to_utxostr, EXIT_ARGERROR, EXIT_FAILURE, EXIT_SUCCESS, hexbin, dictchanger, listchanger, JM_WALLET_NAME_PREFIX, JM_APP_NAME) +from .twisted_utils import stop_reactor from .bytesprod import BytesProducer from .commands import * diff --git a/jmbase/jmbase/twisted_utils.py b/jmbase/jmbase/twisted_utils.py new file mode 100644 index 0000000..57a6584 --- /dev/null +++ b/jmbase/jmbase/twisted_utils.py @@ -0,0 +1,18 @@ + +from twisted.internet.error import ReactorNotRunning, AlreadyCancelled +from twisted.internet import reactor + +def stop_reactor(): + """ Both in startup and shutdown, + the value of the bool `reactor.running` + does not reliably tell us whether the + reactor is running (!). There are startup + and shutdown phases not reported externally + by IReactorCore. + Hence the Exception catch is needed here. + """ + try: + if reactor.running: + reactor.stop() + except ReactorNotRunning: + pass diff --git a/jmclient/jmclient/blockchaininterface.py b/jmclient/jmclient/blockchaininterface.py index 3060f2b..dcf5274 100644 --- a/jmclient/jmclient/blockchaininterface.py +++ b/jmclient/jmclient/blockchaininterface.py @@ -214,9 +214,11 @@ class BitcoinCoreInterface(BlockchainInterface): # BareException type). log.error("Failure of RPC connection to Bitcoin Core. " "Application cannot continue, shutting down.") - if reactor.running: - reactor.stop() + stop_reactor() return None + # note that JsonRpcError is not caught here; for some calls, we + # have specific behaviour requirements depending on these errors, + # so this is handled elsewhere in BitcoinCoreInterface. return res def is_address_labeled(self, utxo, walletname): @@ -430,7 +432,13 @@ class BitcoinCoreInterface(BlockchainInterface): return retval def get_current_block_height(self): - return self.rpc("getblockcount", []) + try: + res = self.rpc("getblockcount", []) + except JsonRpcError as e: + log.error("Getblockcount RPC failed with: %i, %s" % ( + e.code, e.message)) + res = None + return res def get_best_block_hash(self): return self.rpc('getbestblockhash', []) diff --git a/jmclient/jmclient/maker.py b/jmclient/jmclient/maker.py index 7d7677f..598eaa1 100644 --- a/jmclient/jmclient/maker.py +++ b/jmclient/jmclient/maker.py @@ -6,7 +6,7 @@ import sys import abc import jmbitcoin as btc -from jmbase import bintohex, hexbin, get_log, EXIT_SUCCESS, EXIT_FAILURE +from jmbase import bintohex, hexbin, get_log, EXIT_SUCCESS, EXIT_FAILURE, stop_reactor from jmclient.wallet import estimate_tx_fee, compute_tx_locktime from jmclient.wallet_service import WalletService from jmclient.configure import jm_single @@ -25,7 +25,7 @@ class Maker(object): self.nextoid = -1 self.offerlist = None self.sync_wait_loop = task.LoopingCall(self.try_to_create_my_orders) - self.sync_wait_loop.start(2.0) + self.sync_wait_loop.start(2.0, now=False) self.aborted = False def try_to_create_my_orders(self): @@ -41,7 +41,7 @@ class Maker(object): self.sync_wait_loop.stop() if not self.offerlist: jlog.info("Failed to create offers, giving up.") - sys.exit(EXIT_FAILURE) + stop_reactor() jlog.info('offerlist={}'.format(self.offerlist)) @hexbin diff --git a/jmclient/jmclient/wallet_service.py b/jmclient/jmclient/wallet_service.py index b76eaa8..66014ae 100644 --- a/jmclient/jmclient/wallet_service.py +++ b/jmclient/jmclient/wallet_service.py @@ -15,6 +15,7 @@ from jmclient.output import fmt_tx_data from jmclient.blockchaininterface import (INF_HEIGHT, BitcoinCoreInterface, BitcoinCoreNoHistoryInterface) from jmclient.wallet import FidelityBondMixin +from jmbase import stop_reactor from jmbase.support import jmprint, EXIT_SUCCESS, utxo_to_utxostr, hextobin @@ -56,8 +57,10 @@ class WalletService(Service): # a functioning blockchain interface, but # that bci is now failing when we are starting # the wallet service. - raise Exception("WalletService failed to start " - "due to inability to query block height.") + jlog.error("Failure of RPC connection to Bitcoin Core in " + "wallet service startup. Application cannot " + "continue, shutting down.") + stop_reactor() else: jlog.warning("No blockchain source available, " + "wallet tools will not show correct balances.") @@ -91,8 +94,13 @@ class WalletService(Service): """ def critical_error(): - jlog.error("Failure to get blockheight from Bitcoin Core.") + jlog.error("Critical error updating blockheight.") + # this cleanup (a) closes the wallet, removing the lock + # and (b) signals to clients that the service is no longer + # in a running state, both of which can be useful + # post reactor shutdown. self.stopService() + stop_reactor() return False if self.current_blockheight: @@ -707,6 +715,10 @@ class WalletService(Service): st = time.time() # block height needs to be real time for addition to our utxos: current_blockheight = self.bci.get_current_block_height() + if not current_blockheight: + # this failure will shut down the application elsewhere, here + # just give up: + return wallet_name = self.get_wallet_name() self.reset_utxos() diff --git a/scripts/joinmarket-qt.py b/scripts/joinmarket-qt.py index 589b151..edb6e69 100755 --- a/scripts/joinmarket-qt.py +++ b/scripts/joinmarket-qt.py @@ -63,7 +63,7 @@ donation_address_url = "https://bitcoinprivacy.me/joinmarket-donations" #Version of this Qt script specifically JM_GUI_VERSION = '16dev' -from jmbase import get_log +from jmbase import get_log, stop_reactor from jmbase.support import DUST_THRESHOLD, EXIT_FAILURE, utxo_to_utxostr,\ bintohex, hextobin, JM_CORE_VERSION from jmclient import load_program_config, get_network, update_persist_config,\ @@ -1489,8 +1489,7 @@ class JMMainWindow(QMainWindow): event.accept() if self.reactor.threadpool is not None: self.reactor.threadpool.stop() - if reactor.running: - self.reactor.stop() + stop_reactor() else: event.ignore() From 202f8ee04774804af05edda6e95a14b85d260cda Mon Sep 17 00:00:00 2001 From: Adam Gibson Date: Thu, 1 Oct 2020 16:07:40 +0100 Subject: [PATCH 2/4] Add clarifying comments for delayed order creation. 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. --- jmbase/jmbase/twisted_utils.py | 7 +++---- jmclient/jmclient/blockchaininterface.py | 2 +- jmclient/jmclient/maker.py | 3 +++ jmclient/test/test_coinjoin.py | 9 ++++++++- 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/jmbase/jmbase/twisted_utils.py b/jmbase/jmbase/twisted_utils.py index 57a6584..6d35029 100644 --- a/jmbase/jmbase/twisted_utils.py +++ b/jmbase/jmbase/twisted_utils.py @@ -3,13 +3,12 @@ from twisted.internet.error import ReactorNotRunning, AlreadyCancelled from twisted.internet import reactor def stop_reactor(): - """ Both in startup and shutdown, - the value of the bool `reactor.running` + """ The value of the bool `reactor.running` does not reliably tell us whether the reactor is running (!). There are startup and shutdown phases not reported externally - by IReactorCore. - Hence the Exception catch is needed here. + by IReactorCore. So we must catch Exceptions + raised by trying to stop the reactor. """ try: if reactor.running: diff --git a/jmclient/jmclient/blockchaininterface.py b/jmclient/jmclient/blockchaininterface.py index dcf5274..24b23e7 100644 --- a/jmclient/jmclient/blockchaininterface.py +++ b/jmclient/jmclient/blockchaininterface.py @@ -6,7 +6,7 @@ import time from decimal import Decimal import binascii from twisted.internet import reactor, task -from jmbase import bintohex, hextobin +from jmbase import bintohex, hextobin, stop_reactor import jmbitcoin as btc from jmclient.jsonrpc import JsonRpcConnectionError, JsonRpcError diff --git a/jmclient/jmclient/maker.py b/jmclient/jmclient/maker.py index 598eaa1..631a511 100644 --- a/jmclient/jmclient/maker.py +++ b/jmclient/jmclient/maker.py @@ -25,6 +25,9 @@ class Maker(object): self.nextoid = -1 self.offerlist = None self.sync_wait_loop = task.LoopingCall(self.try_to_create_my_orders) + # don't fire on the first tick since reactor is still starting up + # and may not shutdown appropriately if we immediately recognize + # not-enough-coins: self.sync_wait_loop.start(2.0, now=False) self.aborted = False diff --git a/jmclient/test/test_coinjoin.py b/jmclient/test/test_coinjoin.py index 26e8305..0c851b8 100644 --- a/jmclient/test/test_coinjoin.py +++ b/jmclient/test/test_coinjoin.py @@ -69,6 +69,11 @@ def create_taker(wallet, schedule, monkeypatch): monkeypatch.setattr(taker, 'auth_counterparty', lambda *args: True) return taker +def create_orders(makers): + # fire the order creation immediately (delayed 2s in prod, + # but this is too slow for test): + for maker in makers: + maker.try_to_create_my_orders() def init_coinjoin(taker, makers, orderbook, cj_amount): init_data = taker.initialize(orderbook) @@ -133,6 +138,7 @@ def test_simple_coinjoin(monkeypatch, tmpdir, setup_cj, wallet_cls): makers = [YieldGeneratorBasic( wallet_services[i], [0, 2000, 0, 'swabsoffer', 10**7]) for i in range(MAKER_NUM)] + create_orders(makers) orderbook = create_orderbook(makers) assert len(orderbook) == MAKER_NUM @@ -177,6 +183,7 @@ def test_coinjoin_mixdepth_wrap_taker(monkeypatch, tmpdir, setup_cj): makers = [YieldGeneratorBasic( wallet_services[i], [0, cj_fee, 0, 'swabsoffer', 10**7]) for i in range(MAKER_NUM)] + create_orders(makers) orderbook = create_orderbook(makers) assert len(orderbook) == MAKER_NUM @@ -232,7 +239,7 @@ def test_coinjoin_mixdepth_wrap_maker(monkeypatch, tmpdir, setup_cj): makers = [YieldGeneratorBasic( wallet_services[i], [0, cj_fee, 0, 'swabsoffer', 10**7]) for i in range(MAKER_NUM)] - + create_orders(makers) orderbook = create_orderbook(makers) assert len(orderbook) == MAKER_NUM From 5af2d49a8a4aa2e93605d97ed0826cf9385433ca Mon Sep 17 00:00:00 2001 From: Adam Gibson Date: Mon, 5 Oct 2020 13:16:49 +0200 Subject: [PATCH 3/4] handle Qt wallet load failure --- jmbase/jmbase/twisted_utils.py | 3 +-- jmclient/jmclient/wallet_service.py | 7 +++++++ scripts/joinmarket-qt.py | 9 +++++++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/jmbase/jmbase/twisted_utils.py b/jmbase/jmbase/twisted_utils.py index 6d35029..a5df2e2 100644 --- a/jmbase/jmbase/twisted_utils.py +++ b/jmbase/jmbase/twisted_utils.py @@ -11,7 +11,6 @@ def stop_reactor(): raised by trying to stop the reactor. """ try: - if reactor.running: - reactor.stop() + reactor.stop() except ReactorNotRunning: pass diff --git a/jmclient/jmclient/wallet_service.py b/jmclient/jmclient/wallet_service.py index 66014ae..98b2da8 100644 --- a/jmclient/jmclient/wallet_service.py +++ b/jmclient/jmclient/wallet_service.py @@ -47,9 +47,13 @@ class WalletService(Service): self.wallet = wallet self.synced = False + # used to flag RPC failure at construction of object: + self.rpc_error = False + # keep track of the quasi-real-time blockheight # (updated in main monitor loop) self.current_blockheight = None + if self.bci is not None: if not self.update_blockheight(): # this accounts for the unusual case @@ -60,6 +64,9 @@ class WalletService(Service): jlog.error("Failure of RPC connection to Bitcoin Core in " "wallet service startup. Application cannot " "continue, shutting down.") + self.rpc_error = ("Failure of RPC connection to Bitcoin " + "Core in wallet service startup.") + # no need to call stopService as it has not yet been started. stop_reactor() else: jlog.warning("No blockchain source available, " + diff --git a/scripts/joinmarket-qt.py b/scripts/joinmarket-qt.py index edb6e69..b7dee58 100755 --- a/scripts/joinmarket-qt.py +++ b/scripts/joinmarket-qt.py @@ -1844,6 +1844,10 @@ class JMMainWindow(QMainWindow): mbtype='warn', title="Error") return + if decrypted == "error": + # special case, not a failure to decrypt the file but + # a failure of wallet loading, give up: + self.close() else: if not testnet_seed: testnet_seed, ok = QInputDialog.getText(self, @@ -1887,6 +1891,11 @@ class JMMainWindow(QMainWindow): self.walletRefresh.stop() self.wallet_service = WalletService(wallet) + # in case an RPC error occurs in the constructor: + if self.wallet_service.rpc_error: + JMQtMessageBox(self,self.wallet_service.rpc_error, + mbtype='warn',title="Error") + return "error" if jm_single().bc_interface is None: self.centralWidget().widget(0).updateWalletInfo( From 5604857ec1e34698ac17aebe4f805d541c8bc1e1 Mon Sep 17 00:00:00 2001 From: Adam Gibson Date: Wed, 7 Oct 2020 16:05:37 +0200 Subject: [PATCH 4/4] quit scripts gracefully on walletservice rpc startup failure --- jmclient/jmclient/wallet_utils.py | 2 ++ scripts/add-utxo.py | 2 ++ scripts/sendpayment.py | 2 ++ scripts/tumbler.py | 2 ++ 4 files changed, 8 insertions(+) diff --git a/jmclient/jmclient/wallet_utils.py b/jmclient/jmclient/wallet_utils.py index 7ca8304..07d00c7 100644 --- a/jmclient/jmclient/wallet_utils.py +++ b/jmclient/jmclient/wallet_utils.py @@ -1432,6 +1432,8 @@ def wallet_tool_main(wallet_root_path): # this object is only to respect the layering, # the service will not be started since this is a synchronous script: wallet_service = WalletService(wallet) + if wallet_service.rpc_error: + sys.exit(EXIT_FAILURE) if method not in noscan_methods and jm_single().bc_interface is not None: # if nothing was configured, we override bitcoind's options so that diff --git a/scripts/add-utxo.py b/scripts/add-utxo.py index f39efc5..426f273 100755 --- a/scripts/add-utxo.py +++ b/scripts/add-utxo.py @@ -172,6 +172,8 @@ def main(): wallet_path = get_wallet_path(options.loadwallet) wallet = open_wallet(wallet_path, gap_limit=options.gaplimit) wallet_service = WalletService(wallet) + if wallet_service.rpc_error: + sys.exit(EXIT_FAILURE) while True: if wallet_service.sync_wallet(fast=not options.recoversync): break diff --git a/scripts/sendpayment.py b/scripts/sendpayment.py index 798f3db..8901ed3 100755 --- a/scripts/sendpayment.py +++ b/scripts/sendpayment.py @@ -170,6 +170,8 @@ def main(): wallet_password_stdin=options.wallet_password_stdin, gap_limit=options.gaplimit) wallet_service = WalletService(wallet) + if wallet_service.rpc_error: + sys.exit(EXIT_FAILURE) # in this script, we need the wallet synced before # logic processing for some paths, so do it now: while not wallet_service.synced: diff --git a/scripts/tumbler.py b/scripts/tumbler.py index 7f050e6..5a918bb 100755 --- a/scripts/tumbler.py +++ b/scripts/tumbler.py @@ -45,6 +45,8 @@ def main(): wallet_path = get_wallet_path(wallet_name, None) wallet = open_test_wallet_maybe(wallet_path, wallet_name, max_mix_depth, wallet_password_stdin=options_org.wallet_password_stdin) wallet_service = WalletService(wallet) + if wallet_service.rpc_error: + sys.exit(EXIT_FAILURE) # in this script, we need the wallet synced before # logic processing for some paths, so do it now: while not wallet_service.synced: