Merge #730: Get rid of remaining direct rpc() calls outside blockchaininterface.py

d5cf3878ae Get rid of remaining direct rpc() calls outside blockchaininterface.py (Kristaps Kaupe)

Pull request description:

  Follow-up to #501.

  Test suite passes and also did some manual testnet testing. Had error receiving payjoin from testnet.demo.btcpayserver.org, but that doesn't seem related. More testing is needed before merging.

Top commit has no ACKs.

Tree-SHA512: 036483b0f47349dc3bd6fd8bb5be153e338f7fe547c7e5194becd1ba5975fe1d9880f2d557519a7a9a708da263dd18e38cf4c28a03d5fd9348fef73032b1e6e9

jmclient/jmclient/blockchaininterface.py

@@ -1,5 +1,6 @@
 
 import abc
+import ast
 import random
 import sys
 import time
@@ -25,12 +26,11 @@ class BlockchainInterface(object):
     def __init__(self):
         pass
 
+    @abc.abstractmethod
     def is_address_imported(self, addr):
-        try:
-            return self.rpc('getaccount', [addr]) != ''
-        except JsonRpcError:
-            return len(self.rpc('getaddressinfo', [addr])['labels']) > 0
+        """checks that address is already imported"""
 
+    @abc.abstractmethod
     def is_address_labeled(self, utxo, walletname):
         """checks that UTXO belongs to the JM wallet"""
 
@@ -68,6 +68,8 @@ class BlockchainInterface(object):
         else:
             return False
 
+
+# ElectrumWalletInterface is currently broken
 class ElectrumWalletInterface(BlockchainInterface): #pragma: no cover
     """A pseudo-blockchain interface using the existing 
     Electrum server connection in an Electrum wallet.
@@ -159,17 +161,18 @@ class ElectrumWalletInterface(BlockchainInterface): #pragma: no cover
         log.info("Got fee: " + btc.fee_per_kb_to_str(fee_per_kb_sat))
         return fee_per_kb_sat
 
+
 class BitcoinCoreInterface(BlockchainInterface):
 
     def __init__(self, jsonRpc, network):
         super().__init__()
         self.jsonRpc = jsonRpc
-        blockchainInfo = self.rpc("getblockchaininfo", [])
+        blockchainInfo = self._rpc("getblockchaininfo", [])
         if not blockchainInfo:
             # see note in BitcoinCoreInterface.rpc - here
             # we have to create this object before reactor start,
             # so reactor is not stopped, so we override the 'swallowing'
-            # of the Exception that happened in self.rpc():
+            # of the Exception that happened in self._rpc():
             raise JsonRpcConnectionError("RPC connection to Bitcoin Core "
                                          "was not established successfully.")
         actualNet = blockchainInfo['chain']
@@ -180,21 +183,28 @@ class BitcoinCoreInterface(BlockchainInterface):
             #special case of regtest and testnet having the same addr format
             raise Exception('wrong network configured')
 
+    def is_address_imported(self, addr):
+        try:
+            return self._rpc('getaccount', [addr]) != ''
+        except JsonRpcError:
+            return len(self._rpc('getaddressinfo', [addr])['labels']) > 0
+
     def get_block(self, blockheight):
         """Returns full serialized block at a given height.
         """
-        block_hash = self.rpc('getblockhash', [blockheight])
-        block = self.rpc('getblock', [block_hash, False])
+        block_hash = self._rpc('getblockhash', [blockheight])
+        block = self._rpc('getblock', [block_hash, False])
         if not block:
             return False
         return block
 
-    def rpc(self, method, args):
+    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).
+        Should not be called directly from outside code.
         """
         if method not in ['importaddress', 'walletpassphrase', 'getaccount',
                           'gettransaction', 'getrawtransaction', 'gettxout',
@@ -243,7 +253,7 @@ class BitcoinCoreInterface(BlockchainInterface):
                 "watchonly": True
             })
 
-        result = self.rpc('importmulti', [requests, {"rescan": False}])
+        result = self._rpc('importmulti', [requests, {"rescan": False}])
 
         num_failed = 0
         for row in result:
@@ -266,11 +276,11 @@ class BitcoinCoreInterface(BlockchainInterface):
 
     def import_addresses_if_needed(self, addresses, wallet_name):
         try:
-            imported_addresses = set(self.rpc('getaddressesbyaccount',
+            imported_addresses = set(self._rpc('getaddressesbyaccount',
                                                   [wallet_name]))
         except JsonRpcError:
-            if wallet_name in self.rpc('listlabels', []):
-                imported_addresses = set(self.rpc('getaddressesbylabel',
+            if wallet_name in self._rpc('listlabels', []):
+                imported_addresses = set(self._rpc('getaddressesbylabel',
                                                       [wallet_name]).keys())
             else:
                 imported_addresses = set()
@@ -283,7 +293,7 @@ class BitcoinCoreInterface(BlockchainInterface):
         batch_size = 1000
         iteration = 0
         while True:
-            new = self.rpc(
+            new = self._rpc(
                 'listtransactions',
                 ["*", batch_size, iteration * batch_size, True])
             for tx in new:
@@ -307,7 +317,7 @@ class BitcoinCoreInterface(BlockchainInterface):
         in the wallet (under any label/account), optionally
         skipping some.
         """
-        return self.rpc("listtransactions", ["*", num, skip, True])
+        return self._rpc("listtransactions", ["*", num, skip, True])
 
     def get_transaction(self, txid):
         """ Argument txid is passed in binary.
@@ -319,10 +329,10 @@ class BitcoinCoreInterface(BlockchainInterface):
         htxid = bintohex(txid)
         #changed syntax in 0.14.0; allow both syntaxes
         try:
-            res = self.rpc("gettransaction", [htxid, True])
+            res = self._rpc("gettransaction", [htxid, True])
         except Exception as e:
             try:
-                res = self.rpc("gettransaction", [htxid, 1])
+                res = self._rpc("gettransaction", [htxid, 1])
             except JsonRpcError as e:
                 #This should never happen (gettransaction is a wallet rpc).
                 log.warn("Failed gettransaction call; JsonRpcError: " + repr(e))
@@ -342,7 +352,7 @@ class BitcoinCoreInterface(BlockchainInterface):
         """
         txhex = bintohex(txbin)
         try:
-            txid = self.rpc('sendrawtransaction', [txhex])
+            txid = self._rpc('sendrawtransaction', [txhex])
         except JsonRpcConnectionError as e:
             log.warning('error pushing = ' + repr(e))
             return False
@@ -378,7 +388,7 @@ class BitcoinCoreInterface(BlockchainInterface):
                 log.warn("Invalid utxo format, ignoring: {}".format(txo))
                 result.append(None)
                 continue
-            ret = self.rpc('gettxout', [txo_hex, txo_idx, includeunconf])
+            ret = self._rpc('gettxout', [txo_hex, txo_idx, includeunconf])
             if ret is None:
                 result.append(None)
             else:
@@ -399,7 +409,7 @@ class BitcoinCoreInterface(BlockchainInterface):
         if super().fee_per_kb_has_been_manually_set(N):
             # use the local bitcoin core relay fee as floor to avoid relay problems
             btc_relayfee = -1
-            rpc_result = self.rpc('getnetworkinfo', None)
+            rpc_result = self._rpc('getnetworkinfo', None)
             btc_relayfee = rpc_result.get('relayfee', btc_relayfee)
             if btc_relayfee > 0:
                 relayfee_in_sat = int(Decimal(1e8) * Decimal(btc_relayfee))
@@ -419,7 +429,7 @@ class BitcoinCoreInterface(BlockchainInterface):
         estimate = -1
         retval = -1
         for i in range(tries):
-            rpc_result = self.rpc('estimatesmartfee', [N + i])
+            rpc_result = self._rpc('estimatesmartfee', [N + i])
             estimate = rpc_result.get('feerate', estimate)
             if estimate > 0:
                 break
@@ -433,7 +443,7 @@ class BitcoinCoreInterface(BlockchainInterface):
 
     def get_current_block_height(self):
         try:
-            res = self.rpc("getblockcount", [])
+            res = self._rpc("getblockcount", [])
         except JsonRpcError as e:
             log.error("Getblockcount RPC failed with: %i, %s" % (
                 e.code, e.message))
@@ -441,23 +451,32 @@ class BitcoinCoreInterface(BlockchainInterface):
         return res
 
     def get_best_block_hash(self):
-        return self.rpc('getbestblockhash', [])
+        return self._rpc('getbestblockhash', [])
 
-    def get_block_time(self, blockhash):
+    def get_best_block_median_time(self):
+        return self._rpc('getblockchaininfo', [])['mediantime']
+
+    def _get_block_header_data(self, blockhash, key):
         try:
             # works with pruning enabled, but only after v0.12
-            return self.rpc('getblockheader', [blockhash])['time']
+            return self._rpc('getblockheader', [blockhash])[key]
         except JsonRpcError:
-            return self.rpc('getblock', [blockhash])['time']
+            return self._rpc('getblock', [blockhash])[key]
+
+    def get_block_height(self, blockhash):
+        return self._get_block_header_data(blockhash, 'height')
+
+    def get_block_time(self, blockhash):
+        return self._get_block_header_data(blockhash, 'time')
 
     def get_tx_merkle_branch(self, txid, blockhash=None):
         if not blockhash:
-            tx = self.rpc("gettransaction", [txid])
+            tx = self._rpc("gettransaction", [txid])
             if tx["confirmations"] < 1:
                 raise ValueError("Transaction not in block")
             blockhash = tx["blockhash"]
         try:
-            core_proof = self.rpc("gettxoutproof", [[txid], blockhash])
+            core_proof = self._rpc("gettxoutproof", [[txid], blockhash])
         except JsonRpcError:
             raise ValueError("Block containing transaction is pruned")
         return self.core_proof_to_merkle_branch(core_proof)
@@ -469,12 +488,28 @@ class BitcoinCoreInterface(BlockchainInterface):
         return core_proof[80:]
 
     def verify_tx_merkle_branch(self, txid, block_height, merkle_branch):
-        block_hash = self.rpc("getblockhash", [block_height])
-        core_proof = self.rpc("getblockheader", [block_hash, False]) + \
+        block_hash = self._rpc("getblockhash", [block_height])
+        core_proof = self._rpc("getblockheader", [block_hash, False]) + \
             binascii.hexlify(merkle_branch).decode()
-        ret = self.rpc("verifytxoutproof", [core_proof])
+        ret = self._rpc("verifytxoutproof", [core_proof])
         return len(ret) == 1 and ret[0] == txid
 
+    def listaddressgroupings(self):
+        return self._rpc('listaddressgroupings', [])
+
+    def listunspent(self, minconf=None):
+        listunspent_args = []
+        if 'listunspent_args' in jm_single().config.options('POLICY'):
+            listunspent_args = ast.literal_eval(jm_single().config.get(
+                'POLICY', 'listunspent_args'))
+        if minconf is not None:
+            listunspent_args[0] = minconf
+        return self._rpc('listunspent', listunspent_args)
+
+    def testmempoolaccept(self, rawtx):
+        return self._rpc('testmempoolaccept', [[rawtx]])
+
+
 class RegtestBitcoinCoreMixin():
     """
     This Mixin provides helper functions that are used in Interface classes
@@ -486,7 +521,7 @@ class RegtestBitcoinCoreMixin():
         instruct to mine n blocks.
         """
         try:
-            self.rpc('generatetoaddress', [n, self.destn_addr])
+            self._rpc('generatetoaddress', [n, self.destn_addr])
         except JsonRpcConnectionError:
             #can happen if the blockchain is shut down
             #automatically at the end of tests; this shouldn't
@@ -510,17 +545,18 @@ class RegtestBitcoinCoreMixin():
         #mine enough to get to the reqd amt
         reqd = int(amt - self.current_balance)
         reqd_blocks = int(reqd/50) +1
-        if self.rpc('setgenerate', [True, reqd_blocks]):
+        if self._rpc('setgenerate', [True, reqd_blocks]):
         raise Exception("Something went wrong")
         """
         # now we do a custom create transaction and push to the receiver
-        txid = self.rpc('sendtoaddress', [receiving_addr, amt])
+        txid = self._rpc('sendtoaddress', [receiving_addr, amt])
         if not txid:
             raise Exception("Failed to broadcast transaction")
         # confirm
         self.tick_forward_chain(1)
         return txid
 
+
 class BitcoinCoreNoHistoryInterface(BitcoinCoreInterface, RegtestBitcoinCoreMixin):
 
     def __init__(self, jsonRpc, network):
@@ -537,8 +573,8 @@ class BitcoinCoreNoHistoryInterface(BitcoinCoreInterface, RegtestBitcoinCoreMixi
             log.debug("Starting scan of UTXO set")
             st = time.time()
             try:
-                self.rpc("scantxoutset", ["abort", []])
-                self.scan_result = self.rpc("scantxoutset", ["start",
+                self._rpc("scantxoutset", ["abort", []])
+                self.scan_result = self._rpc("scantxoutset", ["start",
                     addr_list])
             except JsonRpcError as e:
                 raise RuntimeError("Bitcoin Core 0.17.0 or higher required "
@@ -567,23 +603,19 @@ class BitcoinCoreNoHistoryInterface(BitcoinCoreInterface, RegtestBitcoinCoreMixi
     def list_transactions(self, num):
         return []
 
-    def rpc(self, method, args):
-        if method == "listaddressgroupings":
-            raise RuntimeError("default sync not supported by bitcoin-rpc-nohistory, use --recoversync")
-        elif method == "listunspent":
-            minconf = 0 if len(args) < 1 else args[0]
-            maxconf = 9999999 if len(args) < 2 else args[1]
-            return [{
-                "address": self._get_addr_from_desc(u["desc"]),
-                "label": self.wallet_name,
-                "height": u["height"],
-                "txid": u["txid"],
-                "vout": u["vout"],
-                "scriptPubKey": u["scriptPubKey"],
-                "amount": u["amount"]
-                } for u in self.scan_result["unspents"]]
-        else:
-            return super().rpc(method, args)
+    def listaddressgroupings(self):
+        raise RuntimeError("default sync not supported by bitcoin-rpc-nohistory, use --recoversync")
+
+    def listunspent(self):
+        return [{
+            "address": self._get_addr_from_desc(u["desc"]),
+            "label": self.wallet_name,
+            "height": u["height"],
+            "txid": u["txid"],
+            "vout": u["vout"],
+            "scriptPubKey": u["scriptPubKey"],
+            "amount": u["amount"]
+        } for u in self.scan_result["unspents"]]
 
     def set_wallet_no_history(self, wallet):
         #make wallet-tool not display any new addresses
@@ -595,9 +627,10 @@ class BitcoinCoreNoHistoryInterface(BitcoinCoreInterface, RegtestBitcoinCoreMixi
         wallet.disable_new_scripts = True
 
     def tick_forward_chain(self, n):
-        self.destn_addr = self.rpc("getnewaddress", [])
+        self.destn_addr = self._rpc("getnewaddress", [])
         super().tick_forward_chain(n)
 
+
 # class for regtest chain access
 # running on local daemon. Only
 # to be instantiated after network is up
@@ -611,7 +644,7 @@ class RegtestBitcoinCoreInterface(BitcoinCoreInterface, RegtestBitcoinCoreMixin)
         self.absurd_fees = False
         self.simulating = False
         self.shutdown_signal = False
-        self.destn_addr = self.rpc("getnewaddress", [])
+        self.destn_addr = self._rpc("getnewaddress", [])
 
     def estimate_fee_per_kb(self, N):
         if not self.absurd_fees:
@@ -655,8 +688,8 @@ class RegtestBitcoinCoreInterface(BitcoinCoreInterface, RegtestBitcoinCoreMixin)
         # in the wallet
         res = []
         for address in addresses:
-            #self.rpc('importaddress', [address, 'watchonly'])
+            #self._rpc('importaddress', [address, 'watchonly'])
             res.append({'address': address,
-                        'balance': int(Decimal(1e8) * self.rpc(
+                        'balance': int(Decimal(1e8) * self._rpc(
                             'getreceivedbyaddress', [address, 0]))})
         return {'data': res}

jmclient/jmclient/payjoin.py

@@ -872,8 +872,8 @@ class PayjoinServer(Resource):
         # it is still safer to at least verify the validity of the signatures
         # at this stage, to ensure no misbehaviour with using inputs
         # that are not signed correctly:
-        res = jm_single().bc_interface.rpc('testmempoolaccept', [[bintohex(
-            self.manager.payment_tx.serialize())]])
+        res = jm_single().bc_interface.testmempoolaccept(bintohex(
+            self.manager.payment_tx.serialize()))
         if not res[0]["allowed"]:
             return self.bip78_error(request, "Proposed transaction was "
                                     "rejected from mempool.",

jmclient/jmclient/wallet_service.py

@@ -484,7 +484,7 @@ class WalletService(Service):
         have been used in our Core wallet with the specific label
         for our JM wallet. This operation is generally immediate.
         """
-        agd = self.bci.rpc('listaddressgroupings', [])
+        agd = self.bci.listaddressgroupings()
         # flatten all groups into a single list; then, remove duplicates
         fagd = (tuple(item) for sublist in agd for item in sublist)
         # "deduplicated flattened address grouping data" = dfagd
@@ -647,7 +647,7 @@ class WalletService(Service):
                     jlog.warning("Merkle branch likely not available, use "
                         + "wallet-tool `addtxoutproof`")
                     merkle_branch = None
-                block_height = self.bci.rpc("getblockheader", [gettx["blockhash"]])["height"]
+                block_height = self.bci.get_block_height(gettx["blockhash"])
                 if merkle_branch:
                     assert self.bci.verify_tx_merkle_branch(txid, block_height, merkle_branch)
                 self.wallet.add_burner_output(path_repr, gettx["hex"], block_height,
@@ -729,12 +729,7 @@ class WalletService(Service):
         wallet_name = self.get_wallet_name()
         self.reset_utxos()
 
-        listunspent_args = []
-        if 'listunspent_args' in jm_single().config.options('POLICY'):
-            listunspent_args = ast.literal_eval(jm_single().config.get(
-                'POLICY', 'listunspent_args'))
-
-        unspent_list = self.bci.rpc('listunspent', listunspent_args)
+        unspent_list = self.bci.listunspent()
         # filter on label, but note (a) in certain circumstances (in-
         # wallet transfer) it is possible for the utxo to be labeled
         # with the external label, and (b) the wallet will know if it

jmclient/test/commontest.py

@@ -229,7 +229,7 @@ def ensure_bip65_activated():
     #on regtest bip65 activates on height 1351
     #https://github.com/bitcoin/bitcoin/blob/1d1f8bbf57118e01904448108a104e20f50d2544/src/chainparams.cpp#L262
     BIP65Height = 1351
-    current_height = jm_single().bc_interface.rpc("getblockchaininfo", [])["blocks"]
+    current_height = jm_single().bc_interface.get_current_block_height()
     until_bip65_activation = BIP65Height - current_height + 1
     if until_bip65_activation > 0:
         jm_single().bc_interface.tick_forward_chain(until_bip65_activation)

jmclient/test/test_psbt_wallet.py

@@ -231,8 +231,7 @@ def test_payjoin_workflow(setup_psbt_wallet, payment_amt, wallet_cls_sender,
     # don't want to push the tx right now, because of test structure
     # (in production code this isn't really needed, we will not
     # produce invalid payment transactions).
-    res = jm_single().bc_interface.rpc('testmempoolaccept',
-                                       [[bintohex(extracted_tx)]])
+    res = jm_single().bc_interface.testmempoolaccept(bintohex(extracted_tx))
     assert res[0]["allowed"], "Payment transaction was rejected from mempool."
 
     # *** STEP 2 ***

jmclient/test/test_tx_creation.py

@@ -143,7 +143,7 @@ def test_spend_freeze_script(setup_tx_creation):
     wallet_service = make_wallets(1, [[3, 0, 0, 0, 0]], 3)[0]['wallet']
     wallet_service.sync_wallet(fast=True)
 
-    mediantime = jm_single().bc_interface.rpc("getblockchaininfo", [])["mediantime"]
+    mediantime = jm_single().bc_interface.get_best_block_median_time()
 
     timeoffset_success_tests = [(2, False), (-60*60*24*30, True), (60*60*24*30, False)]
 

jmclient/test/test_wallet.py

@@ -755,7 +755,7 @@ def test_wallet_mixdepth_decrease(setup_wallet):
     assert max_mixdepth >= 1, "bad default value for mixdepth for this test"
     utxo = fund_wallet_addr(wallet, wallet.get_internal_addr(max_mixdepth), 1)
     bci = jm_single().bc_interface
-    unspent_list = bci.rpc('listunspent', [0])
+    unspent_list = bci.listunspent(0)
     # filter on label, but note (a) in certain circumstances (in-
     # wallet transfer) it is possible for the utxo to be labeled
     # with the external label, and (b) the wallet will know if it