Test walletdisplayaddress by Abeeujah · Pull Request #539 · rust-bitcoin/corepc

Abeeujah · 2026-04-02T19:08:38Z

signer__wallet_display_address tests walletdisplayaddress RPC call mocking an external signer.

The test mocks a Hardware Wallet (HWW) interaction by:

Creating a temporary shell script that acts as an external signer.
Handling the 'enumerate', 'getdescriptors', and 'displayaddress' commands expected by Bitcoin Core.
Spawning a node with the -signer argument pointing to this mock.
Creating a descriptor-based wallet with external_signer enabled.
Asserting that calling wallet_display_address returns the expected address from the mock signer.

satsfy

Nice test coverage, and I tested this locally on Linux (bitcoind v30.2.0) with: cargo test --manifest-path integration_test/Cargo.toml --features latest --test signer -- --nocapture

Concept ACK on c53f47d

satsfy · 2026-04-06T21:04:34Z


    assert_eq!(first_tx.fingerprint, "deadbeef");
 }
+
+#[test]
+#[cfg(unix)]
+#[cfg(not(feature = "v21_and_below"))]
+fn signer__wallet_display_address() {
+    use std::os::unix::fs::PermissionsExt;
+
+    let xpub = "tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B";
+
+    // Mock signer script that handles `enumerate`, `getdescriptors`, and
+    // `displayaddress` sub-commands — the three commands bitcoind invokes when
+    // creating an external-signer wallet and then displaying an address.
+    let script_path = integration_test::random_tmp_file();
+    let script_body = format!(
+        r#"#!/bin/sh
+CMD=""
+ACCOUNT=""
+DESC=""
+while [ $# -gt 0 ]; do
+    case "$1" in
+    --fingerprint) shift ;;
+    --chain)       shift ;;
+    --account)     ACCOUNT="$2'"; shift ;;
+    --desc)        DESC="$2"; shift ;;
+    enumerate|getdescriptors|displayaddress) CMD="$1" ;;
+    esac
+    shift
+done
+
+case "$CMD" in
+    enumerate)
+    echo '[{{"fingerprint":"00000001","type":"trezor","model":"trezor_t"}}]'
+    ;;
+    getdescriptors)
+    cat <<EOF
+{{"receive":["wpkh([00000001/84h/1h/${{ACCOUNT}}]{xpub}/0/*)#h62dxaej"],"internal":["wpkh([00000001/84h/1h/${{ACCOUNT}}]{xpub}/1/*)#xw0vmgf2"]}}
+EOF
+    ;;
+    displayaddress)
+    case "$DESC" in
+        "wpkh([00000001/84h/1h/0h/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#3te6hhy7"|"wpkh([00000001/84'/1'/0'/0/0]02c97dc3f4420402e01a113984311bf4a1b8de376cac0bdcfaf1b3ac81f13433c7)#0yneg42r")
+        echo '{{"address":"bcrt1qm90ugl4d48jv8n6e5t9ln6t9zlpm5th68x4f8g"}}'
+        ;;
+        *)
+        echo '{{"error":"Unexpected descriptor","desc":"'"$DESC"'"}}'
+        exit 1
+        ;;
+    esac
+    ;;
+    *)
+    echo '{{"error":"Unknown command"}}'
+    exit 1
+    ;;
+esac
+"#
+    );
+    std::fs::write(&script_path, script_body).expect("write signer script");
+    std::fs::set_permissions(&script_path, std::fs::Permissions::from_mode(0o755)).expect("chmod");
+
+    let signer_arg = format!("-signer={}", script_path.to_str().unwrap());
+    let node = Node::with_wallet(Wallet::None, &[&signer_arg]);
+
+    let _: node::serde_json::Value = node
+        .client
+        .call(
+            "createwallet",
+            &[
+                "hww".into(),                  // wallet_name
+                true.into(),                   // disable_private_keys
+                false.into(),                  // blank
+                "".into(),                     // passphrase
+                false.into(),                  // avoid_reuse
+                true.into(),                   // descriptors
+                node::serde_json::Value::Null, // load_on_startup
+                true.into(),                   // external_signer
+            ],
+        )
+        .expect("createwallet with external signer");
+
+    let address = "bcrt1qm90ugl4d48jv8n6e5t9ln6t9zlpm5th68x4f8g";
+    let json: WalletDisplayAddress =
+        node.client.wallet_display_address(address).expect("walletdisplayaddress");
+    assert_eq!(json.address, address);
+}


NIT: the multi-line inline script in Rust is awkward to read. Please move the mock signer to a fixture file (e.g. integration_test/tests/fixtures/mock_signer.sh), and load/copy it from the test. Then I’ll re-test and ACK.

I get your point, It's ~11 lines now, does just what it should do, testwalletdisplayaddress, it should be easier on the eyes now?

Thanks for addressing, seem fair.

satsfy

tACK d599574

Tested on Linux (bitcoind v30.2.0) with cargo test --manifest-path integration_test/Cargo.toml --features latest --test signer -- --nocapture

satsfy

I suppose you might also want to update the mod.rs documentation files where it says //! | walletdisplayaddress | version + model | UNTESTED like in types/src/v30/mod.rs, because now it is tested.

tcharding · 2026-04-07T00:34:36Z

+    let _: node::serde_json::Value = node
+        .client
+        .call(
+            "createwallet",
+            &[
+                "hww".into(),                  // wallet_name
+                true.into(),                   // disable_private_keys
+                false.into(),                  // blank
+                "".into(),                     // passphrase
+                false.into(),                  // avoid_reuse
+                true.into(),                   // descriptors
+                node::serde_json::Value::Null, // load_on_startup
+                true.into(),                   // external_signer
+            ],
+        )
+        .expect("createwallet with external signer");


I rekon it would be better if we created a function create_wallet_external_signer in a macro impl_client_v22__create_wallet like we do for create_descriptor_wallet.

tcharding · 2026-04-07T00:39:23Z

+    let json: WalletDisplayAddress =
+        node.client.wallet_display_address(address).expect("walletdisplayaddress");


Can you call into_model and explicitly return the error type as well please.

let json: LoadTxOutSet = node_b.client.load_tx_out_set(dump_path).expect("loadtxoutset should succeed"); let model: Result<mtype::LoadTxOutSet, LoadTxOutSetError> = json.into_model(); let model = model.unwrap();

jamillambert

The test looks good. Just a few minor comments.

jamillambert · 2026-04-07T15:14:27Z

 crate::impl_client_v17__add_multisig_address!();
 crate::impl_client_v17__backup_wallet!();
 crate::impl_client_v17__bump_fee!();
+crate::impl_client_v22__create_wallet!();


Nit. order by version. i.e. v21 before v22.

I appreciate the nit 👏

jamillambert · 2026-04-07T15:16:22Z

+        impl Client {
+            /// Creates a wallet with external_signer=true.
+            ///
+            /// > createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )


Suggested change

/// > createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )

/// > createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup external_signer )

jamillambert · 2026-04-07T15:27:26Z

+fn signer__wallet_display_address() {
+    use std::os::unix::fs::PermissionsExt;
+
+    let address = "bcrt1qm90ugl4d48jv8n6e5t9ln6t9zlpm5th68x4f8g";


Suggested change

let address = "bcrt1qm90ugl4d48jv8n6e5t9ln6t9zlpm5th68x4f8g";

// Mock address and xpub from Bitcoin Core's `wallet_signer.py`.

let address = "bcrt1qm90ugl4d48jv8n6e5t9ln6t9zlpm5th68x4f8g";

Or some other comment to where they came from.

jamillambert · 2026-04-07T15:31:11Z

+
+    let json: WalletDisplayAddress =
+        node.client.wallet_display_address(address).expect("walletdisplayaddress");
+    assert_eq!(json.address, address);


Suggested change

assert_eq!(json.address, address);

This is redundant, the modelled address is checked below.

true, has been dropped 👍

tcharding · 2026-04-08T05:21:28Z

 crate::impl_client_v21__create_wallet!();
+crate::impl_client_v22__create_wallet!();


This is surprising, I'd expect there to be just a single macro that implements createwallet.

In v21 createwallet was taking only 7 arguments, external_signer wasn't among them, until v22 where the arguments became 8 and external_signer was the addition; that's why I couldn't do the create_wallet_external_signer in impl_client_v21__create_wallet!

tcharding · 2026-04-08T05:36:52Z

hmm the create_wallet macros are a bit of a mess. We have create_wallet_descriptor and this introduces a bit more mess.

Perhaps we should have a patch at the front of this PR that removes the _descriptor macro and just implements impl_client_vXY__create_wallet for each version that modifies it and in each just duplicate all the code from the previous one?

@jamillambert do you have an opinion on this?

We could also just merge this as is and fix as a follow up.

tcharding

ACK 4c9c960

tcharding · 2026-04-08T05:38:35Z

~~Everything else looks good to me.~~ Oh woops, the test name is wrong, needs __modelled on it.

Implement `create_wallet_external_signer` for creating wallets that can interact with external signers e.g. hardware wallet. Add an integration test `signer__wallet_display_address` to verify the functionality of the `walletdisplayaddress` RPC call when using an external signer. The test mocks a Hardware Wallet (HWW) interaction by: 1. Creating a temporary shell script that acts as an external signer. 2. Handling the 'enumerate', 'getdescriptors', and 'displayaddress' commands expected by Bitcoin Core. 3. Spawning a node with the `-signer` argument pointing to this mock. 4. Creating a descriptor-based wallet with `external_signer` enabled. 5. Asserting that calling `wallet_display_address` returns the expected address from the mock signer. Drop all `UNTESTED` for `walletdisplayaddress` RPC

jamillambert

ACK df26039

jamillambert · 2026-04-08T14:17:31Z

hmm the create_wallet macros are a bit of a mess.

Yeah, it's the only macro that has multiple version definitions at the same time. I wasn't sure if duplicating all the code in the v21 macro in v22 and so on is better or not. What is done here is functionally fine and can be cleaned up in a follow up.

tcharding

ACK df26039

tcharding · 2026-04-09T00:04:16Z

can be cleaned up in a follow up.

Works for me.

Abeeujah requested a review from tcharding as a code owner April 2, 2026 19:08

github-actions Bot added the C-integration-test label Apr 2, 2026

satsfy suggested changes Apr 6, 2026

View reviewed changes

Abeeujah force-pushed the test-walletdisplayaddress branch from c53f47d to d599574 Compare April 6, 2026 22:03

Abeeujah requested a review from satsfy April 6, 2026 22:10

satsfy approved these changes Apr 6, 2026

View reviewed changes

satsfy reviewed Apr 7, 2026

View reviewed changes

tcharding reviewed Apr 7, 2026

View reviewed changes

Abeeujah force-pushed the test-walletdisplayaddress branch from d599574 to 1f2d935 Compare April 7, 2026 13:31

Abeeujah requested a review from jamillambert as a code owner April 7, 2026 13:31

github-actions Bot added C-client C-types labels Apr 7, 2026

Abeeujah force-pushed the test-walletdisplayaddress branch from 1f2d935 to b326c78 Compare April 7, 2026 14:18

Abeeujah requested a review from tcharding April 7, 2026 15:31

jamillambert reviewed Apr 7, 2026

View reviewed changes

Abeeujah force-pushed the test-walletdisplayaddress branch from b326c78 to 4c9c960 Compare April 7, 2026 15:39

tcharding reviewed Apr 8, 2026

View reviewed changes

tcharding previously approved these changes Apr 8, 2026

View reviewed changes

Abeeujah dismissed tcharding’s stale review via df26039 April 8, 2026 06:41

Abeeujah force-pushed the test-walletdisplayaddress branch from 4c9c960 to df26039 Compare April 8, 2026 06:41

jamillambert approved these changes Apr 8, 2026

View reviewed changes

tcharding approved these changes Apr 9, 2026

View reviewed changes

tcharding merged commit 207e17d into rust-bitcoin:master Apr 9, 2026
31 checks passed

		let json: WalletDisplayAddress =
		node.client.wallet_display_address(address).expect("walletdisplayaddress");

	/// > createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )
	/// > createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup external_signer )

	let address = "bcrt1qm90ugl4d48jv8n6e5t9ln6t9zlpm5th68x4f8g";
	// Mock address and xpub from Bitcoin Core's `wallet_signer.py`.
	let address = "bcrt1qm90ugl4d48jv8n6e5t9ln6t9zlpm5th68x4f8g";

		crate::impl_client_v21__create_wallet!();
		crate::impl_client_v22__create_wallet!();

Conversation

Abeeujah commented Apr 2, 2026

Uh oh!

satsfy left a comment

Choose a reason for hiding this comment

Uh oh!

satsfy Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satsfy left a comment

Choose a reason for hiding this comment

Uh oh!

satsfy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamillambert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Apr 8, 2026

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamillambert left a comment

Choose a reason for hiding this comment

Uh oh!

jamillambert commented Apr 8, 2026

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Apr 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

satsfy Apr 6, 2026 •

edited

Loading

satsfy left a comment •

edited

Loading

tcharding commented Apr 8, 2026 •

edited

Loading