Follow up to v21 TODO #293

jamillambert · 2025-07-14T20:12:04Z

There were a couple of points in the review of #289 that needed to be addressed.

Improve the tests as suggested, and add a comment to the rustdocs.

Redefine the create_wallet client macro so that for v21 and v22 where the default is a legacy wallet there is a separate function to create a descriptor wallet. Similarly for v23 and above where the default is a descriptor wallet there is a separate function to create a legacy wallet.

Closes #292

tcharding · 2025-07-14T22:08:01Z

integration_test/tests/wallet.rs

    let request = ImportDescriptorsRequest {
        desc: descriptor,
-        timestamp: serde_json::Value::String("now".to_string()),
+        timestamp: serde_json::Value::Number(serde_json::Number::from(start_time)),
    };

    let _: ImportDescriptors = node.client.import_descriptors(&[request]).expect("importdescriptors");


We want to check that the list isn't empty, right? Otherwise all the good work above is not validated.

True. When I checked the list it turns out the RPC was returning an error message "Cannot import descriptor without private keys to a wallet with private keys enabled". So I have modified the test to create a descriptor with a private key.

tcharding · 2025-07-14T22:15:58Z

integration_test/tests/wallet.rs

@@ -314,17 +314,37 @@ fn wallet__import_address() {
 #[cfg(not(feature = "v20_and_below"))]
 fn wallet__import_descriptors() {
    use node::{serde_json, ImportDescriptorsRequest};
+    use std::time::{SystemTime, UNIX_EPOCH};

    let node = Node::with_wallet(Wallet::None, &[]);
    let wallet_name = "desc_wallet";
    node.client.create_wallet_with_descriptors(wallet_name).expect("create descriptor wallet");


Perhaps feature gate and then we only need to call the macro that defines this function in v21 and v22, making the default descriptor wallet thing explicit.

#[cfg(feature = "v22_and_below")] node.client.create_wallet_with_descriptors(wallet_name).expect("create descriptor wallet"); // v23 onwards uses descriptor wallets by default. #[cfg(not(feature = "v22_and_below"))] node.client.create_wallet(wallet_name).expect("create wallet");

jamillambert · 2025-07-15T15:43:31Z

I added a new commit that addresses #292, I put it before the rewrite of the test, since the test uses the function. This also made one commit redundant, which has been dropped.

tcharding · 2025-07-15T22:21:53Z

integration_test/tests/util.rs

+    let txindex_info = index_info.0.get("txindex").unwrap();
+    let _ = txindex_info.synced;  // The synced field is a bool, accessing it validates the JSON deserialization


It doesn't matter all that much but just for educational purposes this comment is incorrect. Its the unwrap that validates that we got a non-empty map. Which means that the let index_info: GetIndexInfo ... line that validates the deserialization. Accessing the field doesn't do anything.

I removed this line.

tcharding · 2025-07-15T22:23:01Z

client/src/client_sync/v23/wallet.rs

            /// Calls `createwallet` with `wallet` as the only argument.
+            /// In v23 and later this creates a descriptor wallet. Use `create_legacy_wallet` to create
+            /// a legacy wallet.


Suggested change

/// Calls `createwallet` with `wallet` as the only argument.

/// In v23 and later this creates a descriptor wallet. Use `create_legacy_wallet` to create

/// a legacy wallet.

/// Calls `createwallet` with `wallet` as the only argument.

///

/// In v23 and later this creates a descriptor wallet. Use `create_legacy_wallet` to create

/// a legacy wallet.

Bit nit-picky and I wouldn't bother rebasing just for this.

tcharding · 2025-07-15T22:23:23Z

client/src/client_sync/v21/wallet.rs

+            /// > 5. avoid_reuse             (boolean, optional, default=false) Keep track of coin reuse, and treat dirty and clean coins differently with privacy considerations in mind.
+            /// > 6. descriptors             (boolean, optional, default=true) Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation
+            /// > 7. load_on_startup         (boolean, optional) Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged.
+            pub fn create_descriptor_wallet(&self, wallet: &str) -> Result<CreateWallet> {


This is waaay better, nice one man.

tcharding · 2025-07-15T22:27:55Z

integration_test/tests/wallet.rs

+    let _txid = node.client.send_to_address(&address, amount).expect("sendtoaddress");
+
+    // 3. Get the descriptor from the private key.
+    let raw_descriptor = format!("wpkh({})", privkey.to_wif());


For my education, why does the descriptor use wpkh but the address above is a p2pkh address?

Because one bit was copied from another test 😊. I have changed it now so both are native SegWit.

tcharding · 2025-07-15T22:28:53Z

integration_test/tests/wallet.rs

    let request = ImportDescriptorsRequest {
        desc: descriptor,
-        timestamp: serde_json::Value::String("now".to_string()),
+        timestamp: serde_json::Value::Number(serde_json::Number::from(start_time)),


nit: This whole thing is pretty ugly, would be nicer if the ImportDescriptorRequest had a constructor that took start_time and did the serde wrapping. Totally not required, just an observation.

Created a constructor in a new commit, and also moved the imports to the top of the file.

tcharding · 2025-07-15T22:29:12Z

integration_test/tests/wallet.rs

+    let info = node.client.get_descriptor_info(&raw_descriptor).expect("get_descriptor_info");
+    let descriptor = format!("{}#{}", raw_descriptor, info.checksum);


This took me a bit to figure out. It kept complaining that that the descriptor had no private key when it looked like it should.

tcharding

I'm ok to merge this as is if you want or if you want. I'll give you a chance to polish it if you feel like it.

jamillambert · 2025-07-16T08:49:02Z

I made changes to address all of your comments.

jamillambert · 2025-07-22T09:16:57Z

Rebased to remove merge conflict.

tcharding

ACK a6ddd2e

tcharding · 2025-07-23T03:13:32Z

Needs rebase.

The current test passes if the returned map is empty. Update the test to access the values and ensure the deserialization worked.

Change the client macro for v21 to match the style of the v23 definition. Only use this for v21 and v22 where the default wallet is a legacy wallet. Change the test to use the new function. Add a comment to the v23 definition clarifying why the legacy wallet function is needed.

The current test of the RPC always returns true due to scanning for the timestamp `now`. Move imports to the top of the file and allow unused. Improve the test to be more thorough by creating a new address and then scanning for the descriptor at a later time.

jamillambert · 2025-07-23T09:31:24Z

Rebased. It was the adding full stops patch that caused it.

github-actions bot added C-client C-integration-test labels Jul 14, 2025

jamillambert mentioned this pull request Jul 14, 2025

Address all TODO in v21 #289

Merged

jamillambert force-pushed the 0714-v21fixes branch from f414272 to 6876edc Compare July 14, 2025 20:56

tcharding reviewed Jul 14, 2025

View reviewed changes

jamillambert force-pushed the 0714-v21fixes branch from 6876edc to 6e86402 Compare July 15, 2025 10:35

jamillambert force-pushed the 0714-v21fixes branch from 11a0201 to f0d18f9 Compare July 15, 2025 15:53

tcharding reviewed Jul 15, 2025

View reviewed changes

jamillambert force-pushed the 0714-v21fixes branch 2 times, most recently from 2635d52 to 7defbf1 Compare July 16, 2025 08:45

jamillambert force-pushed the 0714-v21fixes branch from 7defbf1 to 02216a7 Compare July 22, 2025 09:16

jamillambert force-pushed the 0714-v21fixes branch from 02216a7 to a6ddd2e Compare July 22, 2025 09:43

tcharding previously approved these changes Jul 23, 2025

View reviewed changes

jamillambert added 4 commits July 23, 2025 10:29

Improve getindexinfo test

62e01e5

The current test passes if the returned map is empty. Update the test to access the values and ensure the deserialization worked.

Add a constructor for ImportDescriptorsRequest

746c5dc

jamillambert dismissed tcharding’s stale review via e396ad0 July 23, 2025 09:30

jamillambert force-pushed the 0714-v21fixes branch from a6ddd2e to e396ad0 Compare July 23, 2025 09:30

		let txindex_info = index_info.0.get("txindex").unwrap();
		let _ = txindex_info.synced; // The synced field is a bool, accessing it validates the JSON deserialization

		let info = node.client.get_descriptor_info(&raw_descriptor).expect("get_descriptor_info");
		let descriptor = format!("{}#{}", raw_descriptor, info.checksum);

Follow up to v21 TODO #293

Are you sure you want to change the base?

Follow up to v21 TODO #293

Uh oh!

Conversation

jamillambert commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

jamillambert commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

jamillambert Jul 16, 2025 • 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!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

jamillambert commented Jul 16, 2025

Uh oh!

jamillambert commented Jul 22, 2025

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Jul 23, 2025

Uh oh!

jamillambert commented Jul 23, 2025

Uh oh!

Uh oh!

jamillambert commented Jul 14, 2025 •

edited

Loading

jamillambert commented Jul 15, 2025 •

edited

Loading

jamillambert Jul 16, 2025 •

edited

Loading