From 5ce4b55d53cb5f22ebed6a422a7c79f6326d5a03 Mon Sep 17 00:00:00 2001 From: Adam Gibson Date: Mon, 25 May 2020 12:16:08 +0100 Subject: [PATCH 1/3] Quit gracefully if Bitcoin RPC connection lost Fixes #442. First, the CONNREFUSED socket error is handled in jsonrpc. Second, we respond to this (but *not* to resets) with a reactor shutdown in BitcoinCoreInterface.rpc(). This also necessitates early-quitting in the calling function (WalletService.transaction_monitor) since the reactor stop will only stop future deferred calls, not the currently running one. The obvious sys.exit approach is only used in startup, because the reactor is not currently running at that point. Also minor change to DummyBlockchainInterface for test. --- jmclient/jmclient/blockchaininterface.py | 30 ++++++++++++++++++++-- jmclient/jmclient/jsonrpc.py | 11 ++++++-- jmclient/jmclient/wallet_service.py | 32 +++++++++++++++++++++--- jmclient/test/commontest.py | 5 ++++ 4 files changed, 71 insertions(+), 7 deletions(-) diff --git a/jmclient/jmclient/blockchaininterface.py b/jmclient/jmclient/blockchaininterface.py index 71e43ae..d9e272a 100644 --- a/jmclient/jmclient/blockchaininterface.py +++ b/jmclient/jmclient/blockchaininterface.py @@ -164,7 +164,12 @@ class BitcoinCoreInterface(BlockchainInterface): def __init__(self, jsonRpc, network): super(BitcoinCoreInterface, self).__init__() self.jsonRpc = jsonRpc - blockchainInfo = self.jsonRpc.call("getblockchaininfo", []) + blockchainInfo = self.rpc("getblockchaininfo", []) + if not blockchainInfo: + # see note in BitcoinCoreInterface.rpc - here + # we have to create this object before reactor start, + # so sys.exit *is* the right call: + sys.exit(EXIT_FAILURE) actualNet = blockchainInfo['chain'] netmap = {'main': 'mainnet', 'test': 'testnet', 'regtest': 'regtest'} @@ -183,12 +188,33 @@ class BitcoinCoreInterface(BlockchainInterface): return block def rpc(self, method, args): + """ Returns the result of an rpc call to the Bitcoin Core RPC API. + If the connection is permanently or unrecognizably broken, None + is returned *and the reactor is shutdown* (because we consider this + condition unsafe - TODO possibly create a "freeze" mode that could + restart when the connection is healed, but that is tricky). + """ if method not in ['importaddress', 'walletpassphrase', 'getaccount', 'gettransaction', 'getrawtransaction', 'gettxout', 'importmulti', 'listtransactions', 'getblockcount', 'scantxoutset']: log.debug('rpc: ' + method + " " + str(args)) - res = self.jsonRpc.call(method, args) + try: + res = self.jsonRpc.call(method, args) + except JsonRpcConnectionError as e: + # note that we only raise this in case the connection error is + # a refusal, or is unrecognized/unknown by our code. So this does + # NOT happen in a reset or broken pipe scenario. + # It is safest to simply shut down. + # Why not sys.exit? sys.exit calls do *not* work inside delayedCalls + # or deferreds in twisted, since a bare exception catch prevents + # an actual system exit (i.e. SystemExit is caught as a + # BareException type). + log.error("Failure of RPC connection to Bitcoin Core. " + "Application cannot continue, shutting down.") + if reactor.running: + reactor.stop() + return None return res def is_address_labeled(self, utxo, walletname): diff --git a/jmclient/jmclient/jsonrpc.py b/jmclient/jmclient/jsonrpc.py index ad8b1d2..436f1dc 100644 --- a/jmclient/jmclient/jsonrpc.py +++ b/jmclient/jmclient/jsonrpc.py @@ -110,15 +110,22 @@ class JsonRpc(object): self.conn.connect() continue elif e.errno == errno.EPIPE: - jlog.warn('Connection had broken pipe, attempting reconnect.') + jlog.warn('Connection had broken pipe, attempting ' + 'reconnect.') self.conn.close() self.conn.connect() continue elif e.errno == errno.EPROTOTYPE: - jlog.warn('Connection had protocol wrong type for socket error, attempting reconnect.') + jlog.warn('Connection had protocol wrong type for socket ' + 'error, attempting reconnect.') self.conn.close() self.conn.connect() continue + elif e.errno == errno.ECONNREFUSED: + # Will not reattempt in this case: + jlog.error("Connection refused.") + self.conn.close() + raise JsonRpcConnectionError("JSON-RPC connection refused.") else: jlog.error('Unhandled connection error ' + str(e)) raise e diff --git a/jmclient/jmclient/wallet_service.py b/jmclient/jmclient/wallet_service.py index c5d9206..ae19f9b 100644 --- a/jmclient/jmclient/wallet_service.py +++ b/jmclient/jmclient/wallet_service.py @@ -40,8 +40,10 @@ class WalletService(Service): # could be more flexible in future, and # the JM wallet object. self.bci = jm_single().bc_interface + # keep track of the quasi-real-time blockheight # (updated in main monitor loop) + self.current_blockheight = None if self.bci is not None: self.update_blockheight() else: @@ -73,14 +75,34 @@ class WalletService(Service): but will be called as part of main monitoring loop to ensure new transactions are added at the right height. + Any failure of the RPC call must result in this returning + False, otherwise return True (means self.current_blockheight + has been correctly updated). """ + if self.current_blockheight: + old_blockheight = self.current_blockheight + else: + old_blockheight = -1 try: self.current_blockheight = self.bci.get_current_block_height() - assert isinstance(self.current_blockheight, Integral) except Exception as e: + # This should never happen now, as we are catching every + # possible Exception in jsonrpc or bci.rpc: jlog.error("Failure to get blockheight from Bitcoin Core:") jlog.error(repr(e)) - return + if reactor.running: + reactor.stop() + return False + if not self.current_blockheight: + return False + + # We have received a new blockheight from Core, sanity check it: + assert isinstance(self.current_blockheight, Integral) + assert self.current_blockheight >= 0 + if self.current_blockheight < old_blockheight: + jlog.warn("Bitcoin Core is reporting a lower blockheight, " + "possibly a reorg.") + return True def startService(self): """ Encapsulates start up actions. @@ -240,9 +262,13 @@ class WalletService(Service): Service is constantly in near-realtime sync with the blockchain. """ - self.update_blockheight() + if not self.update_blockheight(): + return txlist = self.bci.list_transactions(100) + if not txlist: + return + new_txs = [] for x in txlist: # process either (a) a completely new tx or diff --git a/jmclient/test/commontest.py b/jmclient/test/commontest.py index 55395e0..01ce37a 100644 --- a/jmclient/test/commontest.py +++ b/jmclient/test/commontest.py @@ -29,6 +29,11 @@ class DummyBlockchainInterface(BlockchainInterface): def __init__(self): self.fake_query_results = None self.qusfail = False + self.cbh = 1 + + def get_current_block_height(self): + self.cbh += 1 + return self.cbh def rpc(self, a, b): return None From cc219ccba10ef493ef9a8a3e31ad676ec8f7fbef Mon Sep 17 00:00:00 2001 From: Adam Gibson Date: Thu, 11 Jun 2020 15:48:56 +0100 Subject: [PATCH 2/3] Handles RPC failure in Qt with message box and quit. --- jmclient/jmclient/wallet_service.py | 18 +++++++++++------- scripts/joinmarket-qt.py | 20 ++++++++++++++++++-- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/jmclient/jmclient/wallet_service.py b/jmclient/jmclient/wallet_service.py index ae19f9b..01b1e2c 100644 --- a/jmclient/jmclient/wallet_service.py +++ b/jmclient/jmclient/wallet_service.py @@ -79,6 +79,12 @@ class WalletService(Service): False, otherwise return True (means self.current_blockheight has been correctly updated). """ + + def critical_error(): + jlog.error("Failure to get blockheight from Bitcoin Core.") + self.stopService() + return False + if self.current_blockheight: old_blockheight = self.current_blockheight else: @@ -88,13 +94,9 @@ class WalletService(Service): except Exception as e: # This should never happen now, as we are catching every # possible Exception in jsonrpc or bci.rpc: - jlog.error("Failure to get blockheight from Bitcoin Core:") - jlog.error(repr(e)) - if reactor.running: - reactor.stop() - return False + return critical_error() if not self.current_blockheight: - return False + return critical_error() # We have received a new blockheight from Core, sanity check it: assert isinstance(self.current_blockheight, Integral) @@ -191,7 +193,9 @@ class WalletService(Service): """ if not syncresult: jlog.error("Failed to sync the bitcoin wallet. Shutting down.") - reactor.stop() + self.stopService() + if reactor.running: + reactor.stop() return jlog.info("Starting transaction monitor in walletservice") self.monitor_loop = task.LoopingCall( diff --git a/scripts/joinmarket-qt.py b/scripts/joinmarket-qt.py index 8513bd1..8cebd2d 100755 --- a/scripts/joinmarket-qt.py +++ b/scripts/joinmarket-qt.py @@ -1403,7 +1403,8 @@ class JMMainWindow(QMainWindow): event.accept() if self.reactor.threadpool is not None: self.reactor.threadpool.stop() - self.reactor.stop() + if reactor.running: + self.reactor.stop() else: event.ignore() @@ -1765,10 +1766,25 @@ class JMMainWindow(QMainWindow): t = self.centralWidget().widget(0) if not self.wallet_service: #failure to sync in constructor means object is not created newsyncmsg = "Unable to sync wallet - see error in console." + elif not self.wallet_service.isRunning(): + JMQtMessageBox(self, + "The Joinmarket wallet service has stopped; this is usually caused " + "by a Bitcoin Core RPC connection failure. Is your node running?", + mbtype='crit', + title="Error") + qApp.exit(EXIT_FAILURE) + return elif not self.wallet_service.synced: return else: - t.updateWalletInfo(get_wallet_printout(self.wallet_service)) + try: + t.updateWalletInfo(get_wallet_printout(self.wallet_service)) + except Exception: + # this is very likely to happen in case Core RPC connection goes + # down (but, order of events means it is not deterministic). + log.debug("Failed to get wallet information, is there a problem with " + "the blockchain interface?") + return newsyncmsg = "Wallet synced successfully." if newsyncmsg != self.syncmsg: self.syncmsg = newsyncmsg From 52b9ba857362ba2cbc9c83258e35d1be825c0184 Mon Sep 17 00:00:00 2001 From: Adam Gibson Date: Fri, 12 Jun 2020 20:20:26 +0100 Subject: [PATCH 3/3] Initialization of BCI raises Exception not sys.exit Reasoning for this change: to ensure that Qt will show a message and gracefully exit (or quit attempting to load a wallet) in all 3 cases: on startup it show an intelligible message if the RPC connection fails (as before this PR), if the RPC fails while no wallet is loaded and thus no wallet service is started, it should show an intelligible error message when you attempt to load a wallet and it fails, and finally it should show an intelligible error message before quitting, if the rpc connection fails during the period when the wallet is already loaded. By switching to an Exception instead of sys.exit, it does mean that starting a yieldgenerator shows a stack trace, but it also shows an intelligible error message (in red), and this is command line, so UI requirements are less strong. We preserve the "good" behaviour of no stack trace, but only error message, if the rpc connection is lost during running. --- jmclient/jmclient/blockchaininterface.py | 6 ++++-- jmclient/jmclient/wallet_service.py | 20 ++++++++++++++++---- scripts/joinmarket-qt.py | 1 - 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/jmclient/jmclient/blockchaininterface.py b/jmclient/jmclient/blockchaininterface.py index d9e272a..777530a 100644 --- a/jmclient/jmclient/blockchaininterface.py +++ b/jmclient/jmclient/blockchaininterface.py @@ -168,8 +168,10 @@ class BitcoinCoreInterface(BlockchainInterface): if not blockchainInfo: # see note in BitcoinCoreInterface.rpc - here # we have to create this object before reactor start, - # so sys.exit *is* the right call: - sys.exit(EXIT_FAILURE) + # so reactor is not stopped, so we override the 'swallowing' + # of the Exception that happened in self.rpc(): + raise JsonRpcConnectionError("RPC connection to Bitcoin Core " + "was not established successfully.") actualNet = blockchainInfo['chain'] netmap = {'main': 'mainnet', 'test': 'testnet', 'regtest': 'regtest'} diff --git a/jmclient/jmclient/wallet_service.py b/jmclient/jmclient/wallet_service.py index 01b1e2c..25118af 100644 --- a/jmclient/jmclient/wallet_service.py +++ b/jmclient/jmclient/wallet_service.py @@ -41,16 +41,27 @@ class WalletService(Service): # the JM wallet object. self.bci = jm_single().bc_interface + # main loop used to check for transactions, instantiated + # after wallet is synced: + self.monitor_loop = None + self.wallet = wallet + self.synced = False + # keep track of the quasi-real-time blockheight # (updated in main monitor loop) self.current_blockheight = None if self.bci is not None: - self.update_blockheight() + if not self.update_blockheight(): + # this accounts for the unusual case + # where the application started up with + # 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.") else: jlog.warning("No blockchain source available, " + "wallet tools will not show correct balances.") - self.wallet = wallet - self.synced = False # Dicts of registered callbacks, by type # and then by txinfo, for events @@ -119,7 +130,8 @@ class WalletService(Service): should *not* be restarted, instead a new WalletService instance should be created. """ - self.monitor_loop.stop() + if self.monitor_loop: + self.monitor_loop.stop() self.wallet.close() super(WalletService, self).stopService() diff --git a/scripts/joinmarket-qt.py b/scripts/joinmarket-qt.py index 8cebd2d..7e42e48 100755 --- a/scripts/joinmarket-qt.py +++ b/scripts/joinmarket-qt.py @@ -1753,7 +1753,6 @@ class JMMainWindow(QMainWindow): # add information callbacks: self.wallet_service.add_restart_callback(self.restartWithMsg) self.wallet_service.autofreeze_warning_cb = self.autofreeze_warning_cb - self.wallet_service.startService() self.syncmsg = "" self.walletRefresh = task.LoopingCall(self.updateWalletInfo)