From 1f0915054c3e1e3fdc43f2aa90ca5ef5c0d1b0b5 Mon Sep 17 00:00:00 2001 From: relativisticelectron <60378539+relativisticelectron@users.noreply.github.com> Date: Mon, 19 Dec 2022 21:30:17 +0100 Subject: [PATCH] Bugfix: Addressinfo (#2001) --- .pre-commit-config.yaml | 2 +- .../specter/server_endpoints/controller.py | 17 ++-- .../server_endpoints/wallets/wallets_api.py | 91 ++++++++++++------- src/cryptoadvance/specter/static/helpers.js | 2 +- .../templates/includes/address-data.html | 2 +- .../templates/includes/recipient-box.html | 4 +- .../specter/templates/includes/tx-data.html | 26 +----- src/cryptoadvance/specter/wallet.py | 2 +- tests/fix_devices_and_wallets.py | 7 +- tests/test_ep_wallet_api.py | 76 ++++++++++++++++ 10 files changed, 159 insertions(+), 70 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index e4440645e..1654b33c2 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -3,4 +3,4 @@ repos: rev: 22.3.0 hooks: - id: black - language_version: python3.9 + language_version: python3.10 diff --git a/src/cryptoadvance/specter/server_endpoints/controller.py b/src/cryptoadvance/specter/server_endpoints/controller.py index 2e2d4fa2e..a1990860e 100644 --- a/src/cryptoadvance/specter/server_endpoints/controller.py +++ b/src/cryptoadvance/specter/server_endpoints/controller.py @@ -63,8 +63,7 @@ logger = logging.getLogger(__name__) @app.errorhandler(RpcError) def server_rpc_error(rpce): """Specific SpecterErrors get passed on to the User as flash""" - logger.error(request.headers["Accept"]) - if request.headers["Accept"] == "application/json": + if request.headers.get("Accept") == "application/json": return {"error": str(rpce)} if rpce.error_code == -18: # RPC_WALLET_NOT_FOUND @@ -85,7 +84,7 @@ def server_rpc_error(rpce): @app.errorhandler(SpecterError) def server_specter_error(se): """Specific SpecterErrors get passed on to the User as flash (or in error-field for json)""" - if request.headers["Accept"] == "application/json": + if request.headers.get("Accept") == "application/json": return {"error": str(se)} flash(str(se), "error") try: @@ -105,7 +104,7 @@ def server_notFound_error(e): # if rpc is not available error_msg = "Could not find Resource (404): %s" % request.url app.logger.error(error_msg) - if request.headers["Accept"] == "application/json": + if request.headers.get("Accept") == "application/json": return {"error": error_msg} return render_template("500.jinja", error=e), 404 @@ -117,7 +116,7 @@ def server_error(e): app.logger.error(error_msg) trace = traceback.format_exc() app.logger.error(trace) - if request.headers["Accept"] == "application/json": + if request.headers.get("Accept") == "application/json": return {"error": error_msg} return render_template("500.jinja", error=e, traceback=trace), 500 @@ -126,7 +125,7 @@ def server_error(e): def server_broken_core_connection(e): error_msg = "You got disconnected from your node (no RPC connection)" logger.exception(e) - if request.headers["Accept"] == "application/json": + if request.headers.get("Accept") == "application/json": return {"error": error_msg} flash(error_msg, "error") try: @@ -147,7 +146,7 @@ def server_error_timeout(e): # make sure specter knows that rpc is not there app.specter.check() app.logger.error("ExternalProcessTimeoutException: %s" % e) - if request.headers["Accept"] == "application/json": + if request.headers.get("Accept") == "application/json": return {"error": error_msg} flash(error_msg, "warn") return redirect( @@ -167,7 +166,7 @@ def server_error_csrf(e): app.logger.error("CSRF Exception: %s" % e) trace = traceback.format_exc() app.logger.error(trace) - if request.headers["Accept"] == "application/json": + if request.headers.get("Accept") == "application/json": return {"error": error_msg} flash(error_msg, "error") return redirect(request.url) @@ -180,7 +179,7 @@ def server_error_405(e): app.logger.error(f"{error_msg} : {e}") trace = traceback.format_exc() app.logger.error(trace) - if request.headers["Accept"] == "application/json": + if request.headers.get("Accept") == "application/json": return {"error": error_msg} flash(_("Session expired. Please refresh and try again."), "error") return redirect(request.url) diff --git a/src/cryptoadvance/specter/server_endpoints/wallets/wallets_api.py b/src/cryptoadvance/specter/server_endpoints/wallets/wallets_api.py index 0504a3ea3..7af2d65aa 100644 --- a/src/cryptoadvance/specter/server_endpoints/wallets/wallets_api.py +++ b/src/cryptoadvance/specter/server_endpoints/wallets/wallets_api.py @@ -325,7 +325,7 @@ def decoderawtx(wallet_alias): success=True, tx=tx, rawtx=rawtx, - walletName=wallet.name, + wallet_name=wallet.name, ) except Exception as e: app.logger.exception( @@ -512,38 +512,60 @@ def addresses_list(wallet_alias): def addressinfo(wallet_alias): wallet = app.specter.wallet_manager.get_by_alias(wallet_alias) address = request.form.get("address", "") - if address: - descriptor = wallet.get_descriptor( - address=address, keep_xpubs=False, to_string=True, with_checksum=True + if not address: + return jsonify(success=False) + + descriptor = wallet.get_descriptor( + address=address, keep_xpubs=False, to_string=True, with_checksum=True + ) + + xpubs_descriptor = wallet.get_descriptor( + address=address, keep_xpubs=True, to_string=True, with_checksum=True + ) + if (not descriptor) or (not xpubs_descriptor): + logger.debug( + f"No descriptor or xpubs_descriptor was found for address {address} in wallet {wallet.name}" ) - xpubs_descriptor = wallet.get_descriptor( - address=address, keep_xpubs=True, to_string=True, with_checksum=True - ) - # The last two regex groups are optional since Electrum's derivation path is shorter - derivation_path_pattern = ( - r"(\/[0-9h]+)(\/[0-9h]+)(\/[0-9h]+)(\/[0-9h]+)?(\/[0-9h]+)?" - ) - # Only "descriptor" gives full derivation path, looks usually like this: - # wpkh([8c24a510/84h/1h/0h/0/0]0331edcb16cfd ... e02552539d984)#35zjhlhm - match = re.search(derivation_path_pattern, descriptor) - if not match: - logger.debug( - f"Derivation path of this descriptor {descriptor} could not be parsed. Sth. wrong with the regex pattern which was {derivation_path_pattern}?" - ) + return jsonify(success=False) + + address_info = wallet.get_address_info(address=address) + + # The last two regex groups are optional since Electrum's derivation path is shorter + derivation_path_pattern = ( + r"(\/[0-9h]+)(\/[0-9h]+)(\/[0-9h]+)(\/[0-9h]+)?(\/[0-9h]+)?" + ) + # Only "descriptor" gives full derivation path, looks usually like this: + # wpkh([8c24a510/84h/1h/0h/0/0]0331edcb16cfd ... e02552539d984)#35zjhlhm + match = re.search(derivation_path_pattern, descriptor) + if match: logger.debug(f"This is the derivation path match: {match.group()}") - derivation_path = "m" + match.group() - address_info = wallet.get_address_info(address=address) + else: + logger.debug( + f"Derivation path of this descriptor {descriptor} could not be parsed. Sth. wrong with the regex pattern which was {derivation_path_pattern}?" + ) + # only partial information can be returned, because no match/derivation_path could be found return { - "success": True, + "success": False, "address": address, "descriptor": descriptor, "xpubs_descriptor": xpubs_descriptor, - "derivation_path": derivation_path, - "walletName": wallet.name, - "isMine": address_info and not address_info.is_external, + "wallet_name": wallet.name, + "is_mine": address_info and not address_info.is_external, **address_info, # address_info is an instance of Address(dict) } + derivation_path = "m" + match.group() + return { + "success": True, + "address": address, + "descriptor": descriptor, + "xpubs_descriptor": xpubs_descriptor, + "derivation_path": derivation_path, + "wallet_name": wallet.name, + "is_mine": address_info and not address_info.is_external, + **address_info, # address_info is an instance of Address(dict) + } + ################## Wallet CSV export data endpoints ####################### # Export wallet addresses list @@ -684,14 +706,21 @@ def is_address_mine(wallet_alias, address): wallet = app.specter.wallet_manager.get_by_alias(wallet_alias) # filter out invalid input - if (not address) or not isinstance(address, str): - return jsonify(False) + # and Segwit addresses are always between 14 and 74 characters long. + if (not address) or (not isinstance(address, str)) or len(address) < 14: + return jsonify( + { + "is_mine": False, + "wallet_name": wallet.name if wallet else None, + } + ) - # Segwit addresses are always between 14 and 74 characters long. - if len(address) < 14: - return jsonify(False) - - return jsonify(wallet.is_address_mine(address)) + return jsonify( + { + "is_mine": wallet.is_address_mine(address), + "wallet_name": wallet.name if wallet else None, + } + ) @wallets_endpoint_api.route("/wallet//send/estimatefee", methods=["POST"]) diff --git a/src/cryptoadvance/specter/static/helpers.js b/src/cryptoadvance/specter/static/helpers.js index 58046c310..6b69aa5ba 100644 --- a/src/cryptoadvance/specter/static/helpers.js +++ b/src/cryptoadvance/specter/static/helpers.js @@ -196,7 +196,7 @@ async function send_request(url, method_str, csrf_token, formData) { return {"error": `Error while calling ${url} with ${method_str} ${formData}` } } let jsonResponse = await response.json(); - console.log('The response from the fetch call:') + console.log(`${method_str} call response:`) console.log(jsonResponse) if (typeof(jsonResponse) === 'boolean') { return {} diff --git a/src/cryptoadvance/specter/templates/includes/address-data.html b/src/cryptoadvance/specter/templates/includes/address-data.html index 198b8843a..eac03434a 100644 --- a/src/cryptoadvance/specter/templates/includes/address-data.html +++ b/src/cryptoadvance/specter/templates/includes/address-data.html @@ -94,7 +94,7 @@ let derivation_path = jsonResponse.derivation_path; let addressIndex = jsonResponse.index; let isChange = jsonResponse.change; - let walletName = jsonResponse.walletName; + let walletName = jsonResponse.wallet_name; let address = jsonResponse.address; let walletLink = `{{ url_for('wallets_endpoint.wallet', wallet_alias='WALLET_ALIAS') }}`.replace("WALLET_ALIAS", this.wallet); let addressInfoHTML = ` diff --git a/src/cryptoadvance/specter/templates/includes/recipient-box.html b/src/cryptoadvance/specter/templates/includes/recipient-box.html index 513582130..7163a36a1 100644 --- a/src/cryptoadvance/specter/templates/includes/recipient-box.html +++ b/src/cryptoadvance/specter/templates/includes/recipient-box.html @@ -470,8 +470,8 @@ let url="{{ url_for('wallets_endpoint_api.is_address_mine', wallet_alias=wallet.alias, address='this_address') }}" url = url.replace('this_address', address) - let is_address_mine = send_request(url, 'GET', "{{ csrf_token() }}"); - return is_address_mine; + let response = await send_request(url, 'GET', "{{ csrf_token() }}"); + return response['is_mine']; } markRecipient(is_address_mine){ diff --git a/src/cryptoadvance/specter/templates/includes/tx-data.html b/src/cryptoadvance/specter/templates/includes/tx-data.html index 06da34a8b..d6f16a0b1 100644 --- a/src/cryptoadvance/specter/templates/includes/tx-data.html +++ b/src/cryptoadvance/specter/templates/includes/tx-data.html @@ -75,7 +75,7 @@ if (jsonResponse !== null && jsonResponse.success) { console.log(jsonResponse); let rawtx = jsonResponse.rawtx; - let walletName = jsonResponse.walletName; + let walletName = jsonResponse.wallet_name; let tx = jsonResponse.tx; let walletLink = `{{ url_for('wallets_endpoint.wallet', wallet_alias='WALLET_ALIAS') }}`.replace("WALLET_ALIAS", this.wallet); let rawtxHTML = ""; @@ -272,28 +272,12 @@ } async fetchAddressIsMine(address) { - let url = `{{ url_for('wallets_endpoint_api.addressinfo', wallet_alias='WALLET_ALIAS') }}`.replace("WALLET_ALIAS", this.wallet); - var formData = new FormData(); - formData.append('address', address) - formData.append('csrf_token', '{{ csrf_token() }}'); + let url = `{{ url_for('wallets_endpoint_api.is_address_mine', wallet_alias='WALLET_ALIAS', address='ADDRESS') }}`.replace("WALLET_ALIAS", this.wallet).replace("ADDRESS", address); try { - const response = await fetch( - url, - { - method: 'POST', - body: formData - } - ); - if(response.status != 200){ - showError(await response.text()); - return false; - } - const jsonResponse = await response.json(); - if (jsonResponse.success) { - return jsonResponse.isMine; - } + const response = await send_request(url, 'GET', '{{ csrf_token() }}'); + return response['is_mine']; } catch(e) { - showError(`{{ _("Failed to load address data...") }}`); + showError(`{{ _("Failed to check is_address_mine...") }}`); showError(e); } return false; diff --git a/src/cryptoadvance/specter/wallet.py b/src/cryptoadvance/specter/wallet.py index 208ed8d29..3e7037ffe 100644 --- a/src/cryptoadvance/specter/wallet.py +++ b/src/cryptoadvance/specter/wallet.py @@ -1366,7 +1366,7 @@ class Wallet: def is_address_mine(self, address): addrinfo = self.get_address_info(address) - return addrinfo and not addrinfo.is_external + return bool(addrinfo) and not addrinfo.is_external def get_electrum_file(self): """Exports the wallet data as Electrum JSON format""" diff --git a/tests/fix_devices_and_wallets.py b/tests/fix_devices_and_wallets.py index a15294af2..78ec2b1d9 100644 --- a/tests/fix_devices_and_wallets.py +++ b/tests/fix_devices_and_wallets.py @@ -166,9 +166,10 @@ def funded_ghost_machine_wallet( bitcoin_regtest: NodeController, unfunded_ghost_machine_wallet: Wallet ) -> Wallet: funded_ghost_machine_wallet = unfunded_ghost_machine_wallet - bitcoin_regtest.testcoin_faucet( - funded_ghost_machine_wallet.getnewaddress() - ) # default value are 20 BTC + if funded_ghost_machine_wallet.amount_total == 0: + bitcoin_regtest.testcoin_faucet( + funded_ghost_machine_wallet.getnewaddress() + ) # default value are 20 BTC return funded_ghost_machine_wallet diff --git a/tests/test_ep_wallet_api.py b/tests/test_ep_wallet_api.py index 04c3be387..204cba074 100644 --- a/tests/test_ep_wallet_api.py +++ b/tests/test_ep_wallet_api.py @@ -147,6 +147,82 @@ def test_txout_set_info(caplog, app, client): assert json.loads(res.data)["total_amount"] > 0 +@pytest.mark.slow +def test_addressinfo(caplog, client, funded_ghost_machine_wallet): + caplog.set_level(logging.DEBUG) + caplog.set_level(logging.DEBUG, logger="cryptoadvance.specter") + login(client, "secret") + + # get loaded wallet info + response = client.get("/wallets/wallets_loading/", follow_redirects=True) + loaded_wallet_name = json.loads(response.data)["loaded_wallets"][0] + receive_address = funded_ghost_machine_wallet.get_address(0, change=False) + url = f"/wallets/wallet/{loaded_wallet_name}/addressinfo/" + headers = { + "Content-Type": "application/x-www-form-urlencoded", + "ACCEPT": "application/json", + } + + # send post request + res = client.post( + url, data={"address": receive_address}, follow_redirects=True, headers=headers + ) + assert res.status == "200 OK" + assert ( + res.data.decode() + == '{"address":"bcrt1qvtdx75y4554ngrq6aff3xdqnvjhmct5wck95qs","change":false,"derivation_path":"m/84h/1h/0h/0/0","descriptor":"wpkh([8c24a510/84h/1h/0h/0/0]0331edcb16cfd0f8598052f7b287b07047a11c60967c1e7eb0257e02552539d984)#35zjhlhm","index":0,"is_mine":true,"label":null,"service_id":null,"success":true,"used":null,"wallet_name":"ghost_machine","xpubs_descriptor":"wpkh([8c24a510/84h/1h/0h]tpubDC4DsqH5rqHqipMNqUbDFtQT3AkKkUrvLsN6miySvortU3s1LGaNVAb7wX2No2VsuxQV82T8s3HJLv3kdx1CPjsJ3onC1Zo5mWCQzRVaWVX/0/0)#8f5u4zq2"}\n' + ) + + # send post request with bad address + invalid_address = "bcrt1qvtdx7" + res = client.post( + url, data={"address": invalid_address}, follow_redirects=True, headers=headers + ) + assert ( + res.data.decode() + == '{"error":"Request error for method getaddressinfo: Invalid address format"}\n' + ) + + # send post request with address, not belonging to wallet + # this recreates an edge case, see https://github.com/cryptoadvance/specter-desktop/issues/2000 + valid_address_not_beloging_to_wallet = ( + "bcrt1q895evdudfrmeut083vs85rc85g2wq6p6ql2hla" + ) + res = client.post( + url, + data={"address": valid_address_not_beloging_to_wallet}, + follow_redirects=True, + headers=headers, + ) + assert res.status == "200 OK" + assert res.data.decode() == '{"success":false}\n' + assert ( + caplog.text.count( + f"No descriptor or xpubs_descriptor was found for address {valid_address_not_beloging_to_wallet} in wallet ghost_machine" + ) + == 1 + ) + + # set erronious descriptor + with mock.patch.object( + funded_ghost_machine_wallet, + "get_descriptor", + return_value="this is not a descriptor", + create=True, + ) as m: + res = client.post( + url, + data={"address": receive_address}, + follow_redirects=True, + headers=headers, + ) + assert res.status == "200 OK" + assert ( + res.data.decode() + == '{"address":"bcrt1qvtdx75y4554ngrq6aff3xdqnvjhmct5wck95qs","change":false,"descriptor":"this is not a descriptor","index":0,"is_mine":true,"label":null,"service_id":null,"success":false,"used":null,"wallet_name":"ghost_machine","xpubs_descriptor":"this is not a descriptor"}\n' + ) + + # Ugly: Code duplication. Cannot import from other test_modules def login(client, password): """login helper-function"""