Address all `TODO` in v21 #289

jamillambert · 2025-07-10T15:46:05Z

Go through all the TODO in the v21 types table. Add all the missing RPCs including models when required and tests.

Add a new macro create_wallet_with_descriptors that creates a descriptor wallet for use with v21 and v22 where the default is a legacy wallet. This is required for the importdescriptors test.

Move the wallet module to a subdirectory since there are multiple new RPCs in v21.

tcharding · 2025-07-10T21:27:03Z

integration_test/tests/generating.rs

+        let json: GenerateBlock = node.client.generate_block(&mining_addr.to_string(), &transactions, false).expect("generateblock");
+        let model: Result<mtype::GenerateBlock, GenerateBlockError> = json.into_model();
+        model.unwrap();
+    }


nit: I rekon the feature guards could just guard the generate_block call. The other two lines are identical.

tcharding · 2025-07-10T21:30:49Z

types/src/v21/util.rs

+/// >
+/// > Arguments:
+/// > 1. index_name    (string, optional) Filter results for an index with a specific name.
+/// >
+/// > Result:
+/// > {                               (json object)
+/// >   "name" : {                    (json object) The name of the index
+/// >     "synced" : true|false,      (boolean) Whether the index is synced or not
+/// >     "best_block_height" : n     (numeric) The block height to which the index is synced
+/// >   },
+/// >   ...


Would be better without all this. In general I rekon we should only add the docs up to 'required' args. Recently you did a few PRs that included option args but I didn't comment because they were brief docs and also they added some useful context. That isn't the case here IMO.

I removed the optional args and result section from the docs of all the added RPCs in this PR.

tcharding · 2025-07-10T21:32:33Z

types/src/v21/util.rs

+/// > }
+#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct GetIndexInfo(pub std::collections::BTreeMap<String, GetIndexInfoName>);


nit: I'd prefer to import BTreeMap.

tcharding · 2025-07-10T21:33:23Z

types/src/v21/util.rs

+
+/// `name` field of `getindexinfo`.
+#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]


Cool, can you help me to remember we use this now in review because since I didn't code with this yet I'm likely to forget.

tcharding

Only got to reviewing half mate, I'll come back. Gotta do some kids stuff now.

Add the RPC `generateblock` type to v21 and add reexports. Add model, into function, client macro and test. Update types table up to v29, v23 and later don't have docs for `Generating` and therefore no table entry.

Add the RPC `getindexinfo` type to v21 and add reexports. Add client macro and test. Update types table up to v29.

In preparation for adding v21 wallet RPCs move the module into a subdirectory and split out the into module. Move only, no code changes.

Add a new create wallet macro for v21 onwards to client that creates a descriptor wallet for tests that require one.

Add the RPC `importdescriptors` type to v21 and add reexports. Add client macro and test. Update types table up to v29.

Add the RPC `send` type to v21 and add reexports. Add model, client macro and test. Update types table up to v29.

Add the RPC `psbtbumpfee` type to v21 and add reexports. Add model, client macro and test. Update types table up to v29.

Add the RPC `upgradewallet` type to v21 and add reexports. Add client macro and test. Update types table up to v29.

tcharding · 2025-07-11T23:50:58Z

integration_test/tests/util.rs

+    let node = Node::with_wallet(Wallet::Default, &[]);
+    let _: GetIndexInfo = node.client.get_index_info().expect("getindexinfo");


IIUC this is going to return an empty map so doesn't really test the JSON shape. We have this same problem in many of our tests because I naively wrote a ton of them without thinking about the RPC method too much. @GideonBature has done a much better job at making sure there is something actually returned.

I created #291

Test fixed in #293

tcharding · 2025-07-11T23:58:40Z

client/src/client_sync/v21/wallet.rs

@@ -9,6 +9,66 @@
 //!
 //! See or use the `define_jsonrpc_minreq_client!` macro to define a `Client`.

+/// Implements Bitcoin Core JSON-RPC API method `createwallet` with descriptors=true (descriptor wallet)


Suggested change

/// Implements Bitcoin Core JSON-RPC API method `createwallet` with descriptors=true (descriptor wallet)

/// Implements Bitcoin Core JSON-RPC API method `createwallet` with descriptors=true (descriptor wallet)

///

/// This is a helper function only needed in v21 and v22 because from v23 onwards this is not needed

/// because descriptors=true is the default.

Done in #293

NB. I didn't use your exact wording of "only needed in v21 and v22 because from v23 onwards this is not needed"

tcharding · 2025-07-12T00:01:22Z

client/src/client_sync/v21/wallet.rs

+            }
+        }
+    };
+}


I think this macro should be done like the v23 one i.e., include function create_wallet as well as the with desrciptor function. Then in mod.rs of v23 and v24 only call this macro not the v17 one and from v23 onwards only call the v23 one.

And I"d comment liberally about args to the RPC method changing and that we only include args we actually use in the integration tests - or some such thing.

I've merged and left this suggested fix up to be done as a follow up because this PR is massive.

Addedd #292

It might just be that I am tired, but I don't understand what you mean.

I added a commit to address this in the follow up PR #293

tcharding · 2025-07-12T00:10:23Z

integration_test/tests/wallet.rs

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


From what I just read and assuming I understand the RPC method, this is going to just return 'success=true' because of use of "now". If we are going to do such a basic test (and that is perfectly ok) we don't need to have the timestamp arg at all - just insert it in the client method that does the RPC call.

Another, more thorough test idea (although I'm not 100% its necessary) would be to

get the current time

get an address and send some coins to it

get the descriptor

mine 100 blocks

scan for that descriptor using the time from (1)

Personally I'd just remove the "now" and comment that this just a basic test and likely good enough for now. There are other tests we can write that add more value and are better uses of your time.

The takeaway here is we (all of us) should try to understand exactly what we are testing and test as simply as possible to get that coverage.

I went with the more thorough option of changing the test in #293

tcharding · 2025-07-12T00:27:00Z

integration_test/tests/wallet.rs

+
+    let json: Send = node.client.send(&outputs).expect("send");
+    let model: Result<mtype::Send, SendError> = json.into_model();
+    model.unwrap();


Cool. This works (I debug printed the result of the last unwrap) but I'm baffled as to why it works. Can you explain if you know otherwise I don't suppose it matters? The docs say 'data' is required but I don't understand why it works without it and what 'data' even is?

data is not required, even though it doesn't say so in the docs. From what I read "It is for arbitrary hex-encoded data in a transaction, typically as an [OP_RETURN] output e.g. { "data": "deadbeef" }".

ah, thanks.

tcharding

ACK a150076

tcharding · 2025-07-12T00:32:43Z

A lot of work here man, well done.

e396ad0 Improve import_descriptors test (Jamil Lambert, PhD) 746c5dc Add a constructor for ImportDescriptorsRequest (Jamil Lambert, PhD) 4f534f3 Rewrite create descriptor wallet client macro (Jamil Lambert, PhD) 62e01e5 Improve getindexinfo test (Jamil Lambert, PhD) Pull request description: 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 ACKs for top commit: tcharding: ACK e396ad0 Tree-SHA512: e035315a926e9a789068a72b0e21f838f30d5c7feb2eba7245dbeea73f500b0c433a11b24620e5141550199a3173cabf3bb478b174d34194806f89b020b8eb05

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

jamillambert force-pushed the 0704-v21-TODOs branch from 7704d52 to 86882e3 Compare July 10, 2025 15:53

tcharding reviewed Jul 10, 2025

View reviewed changes

jamillambert force-pushed the 0704-v21-TODOs branch from 86882e3 to 4909897 Compare July 11, 2025 07:42

jamillambert added 8 commits July 11, 2025 08:48

Add generateblock method, model and test

73368d4

Add the RPC `generateblock` type to v21 and add reexports. Add model, into function, client macro and test. Update types table up to v29, v23 and later don't have docs for `Generating` and therefore no table entry.

Add getindexinfo method, and test

ec52fad

Add the RPC `getindexinfo` type to v21 and add reexports. Add client macro and test. Update types table up to v29.

Move v21 wallet module into subdirectory

8e80267

In preparation for adding v21 wallet RPCs move the module into a subdirectory and split out the into module. Move only, no code changes.

Add create descriptor wallet fn to client

5ee20ed

Add a new create wallet macro for v21 onwards to client that creates a descriptor wallet for tests that require one.

Add importdescriptors method, and test

0ed10d4

Add the RPC `importdescriptors` type to v21 and add reexports. Add client macro and test. Update types table up to v29.

Add send method, model and test

2eecd0f

Add the RPC `send` type to v21 and add reexports. Add model, client macro and test. Update types table up to v29.

Add psbtbumpfee method, model and test

f855ead

Add the RPC `psbtbumpfee` type to v21 and add reexports. Add model, client macro and test. Update types table up to v29.

Add upgradewallet method and test

a150076

Add the RPC `upgradewallet` type to v21 and add reexports. Add client macro and test. Update types table up to v29.

jamillambert force-pushed the 0704-v21-TODOs branch from 4909897 to a150076 Compare July 11, 2025 07:48

tcharding reviewed Jul 11, 2025

View reviewed changes

tcharding reviewed Jul 12, 2025

View reviewed changes

tcharding approved these changes Jul 12, 2025

View reviewed changes

tcharding merged commit 88c69b3 into rust-bitcoin:master Jul 12, 2025
30 checks passed

tcharding mentioned this pull request Jul 12, 2025

Clean up create wallet stuff #292

Closed

jamillambert mentioned this pull request Jul 14, 2025

Follow up to v21 TODO #293

Merged

		let node = Node::with_wallet(Wallet::Default, &[]);
		let _: GetIndexInfo = node.client.get_index_info().expect("getindexinfo");

Address all TODO in v21 #289

Address all TODO in v21 #289

Uh oh!

Conversation

jamillambert commented Jul 10, 2025

Uh oh!

tcharding Jul 10, 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!

tcharding Jul 10, 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!

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!

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 14, 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!

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 Jul 12, 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!

jamillambert Jul 14, 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!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tcharding commented Jul 12, 2025

Uh oh!

Uh oh!

Address all `TODO` in v21 #289

Address all `TODO` in v21 #289

tcharding Jul 10, 2025 •

edited

Loading

tcharding Jul 10, 2025 •

edited

Loading

jamillambert Jul 14, 2025 •

edited

Loading

tcharding Jul 12, 2025 •

edited

Loading

jamillambert Jul 14, 2025 •

edited

Loading