rpc: add address_type field in getaddressinfo

jonatack commented at 10:22 pm on August 27, 2024: member

While creating and verifying fresh addresses today, noticed it would be helpful to see the address type in RPC getaddressinfo.

Also extracted repeated help docs to a utility helper.

Will add a release note if there are concept acks.

DrahtBot commented at 10:22 pm on August 27, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	tdb3, 1440000bytes

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28710 (Remove the legacy wallet and BDB dependency by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label RPC/REST/ZMQ on Aug 27, 2024

jonatack commented at 10:49 pm on August 27, 2024: member

(Looking for Concept ACKs. Not sure this derives the output type correctly yet.)

tdb3 commented at 0:11 am on August 28, 2024: contributor

Concept ACK This seems like a useful addition. Some basic examples below (P2TR, P2WPKH, P2WSH, P2PKH, P2SH)

 0$ src/bitcoin-cli getaddressinfo bc1p8k4v4xuz55dv49svzjg43qjxq2whur7ync9tm0xgl5t4wjl9ca9snxgmlt
 1{
 2  "address": "bc1p8k4v4xuz55dv49svzjg43qjxq2whur7ync9tm0xgl5t4wjl9ca9snxgmlt",
 3  "address_type": "bech32m",
 4  "scriptPubKey": "51203daaca9b82a51aca960c1491588246029d7e0fc49e0abdbcc8fd17574be5c74b",
 5  "ismine": false,
 6  "solvable": false,
 7  "iswatchonly": false,
 8  "isscript": true,
 9  "iswitness": true,
10  "witness_version": 1,
11  "witness_program": "3daaca9b82a51aca960c1491588246029d7e0fc49e0abdbcc8fd17574be5c74b",
12  "ischange": false,
13  "labels": [
14  ]
15}
16$ src/bitcoin-cli getaddressinfo bc1q3w4hnne8j8u0mc2egwghc3rl6t2j7x9ql3r2ee
17{
18  "address": "bc1q3w4hnne8j8u0mc2egwghc3rl6t2j7x9ql3r2ee",
19  "address_type": "bech32",
20  "scriptPubKey": "00148bab79cf2791f8fde15943917c447fd2d52f18a0",
21  "ismine": false,
22  "solvable": false,
23  "iswatchonly": false,
24  "isscript": false,
25  "iswitness": true,
26  "witness_version": 0,
27  "witness_program": "8bab79cf2791f8fde15943917c447fd2d52f18a0",
28  "ischange": false,
29  "labels": [
30  ]
31}
32$ src/bitcoin-cli getaddressinfo bc1q29sg9klttna852haytm73cjre0mcr938cfnsvpzjt03crh8xpadq0x6rgs
33{
34  "address": "bc1q29sg9klttna852haytm73cjre0mcr938cfnsvpzjt03crh8xpadq0x6rgs",
35  "address_type": "bech32",
36  "scriptPubKey": "0020516082dbeb5cfa7a2afd22f7e8e243cbf7819627c2670604525be381dce60f5a",
37  "ismine": false,
38  "solvable": false,
39  "iswatchonly": false,
40  "isscript": true,
41  "iswitness": true,
42  "witness_version": 0,
43  "witness_program": "516082dbeb5cfa7a2afd22f7e8e243cbf7819627c2670604525be381dce60f5a",
44  "ischange": false,
45  "labels": [
46  ]
47}
48$ src/bitcoin-cli getaddressinfo 1JqDybm2nWTENrHvMyafbSXXtTk5Uv5QAn
49{
50  "address": "1JqDybm2nWTENrHvMyafbSXXtTk5Uv5QAn",
51  "address_type": "legacy",
52  "scriptPubKey": "76a914c398efa9c392ba6013c5e04ee729755ef7f58b3288ac",
53  "ismine": false,
54  "solvable": false,
55  "iswatchonly": false,
56  "isscript": false,
57  "iswitness": false,
58  "ischange": false,
59  "labels": [
60  ]
61}
62$ src/bitcoin-cli getaddressinfo 31iSdqvc6HmRb5LeRS1w5AipR16bgEnUx7
63{
64  "address": "31iSdqvc6HmRb5LeRS1w5AipR16bgEnUx7",
65  "address_type": "legacy",
66  "scriptPubKey": "a91400450b68b617ffda211fc12e1245e85a9c36b7bb87",
67  "ismine": false,
68  "solvable": false,
69  "iswatchonly": false,
70  "isscript": true,
71  "iswitness": false,
72  "ischange": false,
73  "labels": [
74  ]
75}

rpc, test: add "address_type" field in getaddressinfo 2ef697e5de

jonatack force-pushed on Aug 28, 2024

Add output types collection methods d52f8b52bd

init, rpc: use output types helpers for docs e579ad9a20

test: add coverage for expected output types in rpc help docs a283dbd9c4

in src/wallet/rpc/addresses.cpp:534 in 69af2aa15f outdated

530@@ -529,6 +531,7 @@ RPCHelpMan getaddressinfo()
531                     RPCResult::Type::OBJ, "", "",
532                     {
533                         {RPCResult::Type::STR, "address", "The bitcoin address validated."},
534+                        {RPCResult::Type::STR, "address_type", "The type of bitcoin address; will be one of " + Join(OutputTypes(/*omit=*/OutputType::P2SH_SEGWIT), ", ") + "."},

maflcko commented at 6:01 am on August 28, 2024:

Not sure about mapping the type to an enum that is only used to tell the wallet which descriptors to create. (It only appears as RPCArg, and I think using it in RPCResult is likely wrong). Also mapping p2sh-segwit to the “legacy” bin seems confusing and inconsistent with the input. So effectively this only differentiates between bech32m and bech32. This is fully redundant and less detailed than the already existing witness_version field.

jonatack commented at 7:41 am on August 28, 2024:

Yup still WIP. It would be nice to provide users with a one-to-one mapping between the input values, supplied to RPC getnewaddress address_type and the -addresstype and -changetype config options, and the same values returned on the other side. If it’s feasible, for as you rightly point out, it currently isn’t consistent. Pointers welcome.

maflcko commented at 7:51 am on August 28, 2024:

It would be nice to provide users with a one-to-one mapping between the input values

Again, I think those are different concepts. One is the wallet spkm-type of the built-in wallet, the other is the info returned on an address.

Possibly using decodescript::type could make sense here instead.

maflcko commented at 7:55 am on August 28, 2024:

If you want to return the descriptor type of the spkm, maybe it would be better to just return the full parent descriptor of the address instead? This should communicate the type, as well as other details that may be interesting. Though, I am not sure what your imagined use case is.

jonatack force-pushed on Aug 28, 2024

1440000bytes commented at 8:25 am on August 28, 2024: none

Concept ACK

Including p2sh-segwit in legacy doesn’t look correct.

0-$ bitcoin-cli -regtest getnewaddress "PR30727" "p2sh-segwit"
12MzNJX9GWfZAthZSBdKt6Ja4TF7vBQDi7Uq

 0$ bitcoin-cli -regtest getaddressinfo 2MzNJX9GWfZAthZSBdKt6Ja4TF7vBQDi7Uq
 1{
 2  "address": "2MzNJX9GWfZAthZSBdKt6Ja4TF7vBQDi7Uq",
 3- "address_type": "legacy",
 4  "scriptPubKey": "a9144e1f934cd4659c0f47ded79ac1be8fb65c978fe787",
 5  "ismine": true,
 6  "solvable": true,
 7  "desc": "sh(wpkh([e4e21d4d/49h/1h/0h/0/1]030a5b6cf916de338de34a2cab0724b2f7321ed08e84fe092c95e19a84ccac4748))#fkhy0ckw",
 8  "parent_desc": "sh(wpkh([e4e21d4d/49h/1h/0h]tpubDCjkBmMqh7D5kViEKCaQPi5UP2RyM8VPLGPZh9yjSJiXwCTX1FJbduXkFnQcs8wNFRSx9gLTYsyvjRux5VxK4oufLAPZREqP6MgUhxzuxCP/0/*))#cwslx3xg",
 9  "iswatchonly": false,
10  "isscript": true,
11  "iswitness": false,
12  "script": "witness_v0_keyhash",
13  "hex": "0014d796c2e4d80f1c0f30c48c661934b797559945c2",
14  "pubkey": "030a5b6cf916de338de34a2cab0724b2f7321ed08e84fe092c95e19a84ccac4748",
15  "embedded": {
16    "isscript": false,
17    "iswitness": true,
18    "witness_version": 0,
19    "witness_program": "d796c2e4d80f1c0f30c48c661934b797559945c2",
20    "pubkey": "030a5b6cf916de338de34a2cab0724b2f7321ed08e84fe092c95e19a84ccac4748",
21    "address": "bcrt1q67tv9excpuwq7vxy33npjd9hja2ej3wzhjnmjx",
22    "scriptPubKey": "0014d796c2e4d80f1c0f30c48c661934b797559945c2"
23  },
24  "ischange": false,
25  "timestamp": 1724825745,
26  "hdkeypath": "m/49h/1h/0h/0/1",
27  "hdseedid": "0000000000000000000000000000000000000000",
28  "hdmasterfingerprint": "e4e21d4d",
29  "labels": [
30    "PR30727"
31  ]
32}

DrahtBot added the label CI failed on Aug 28, 2024

DrahtBot commented at 9:10 am on August 28, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29350258282

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

maflcko commented at 12:27 pm on October 1, 2024: member

Are you still working on this?

jonatack commented at 8:42 pm on October 2, 2024: member

Yes, will update.

rpc: add address_type field in getaddressinfo #30727

Code Coverage

Reviews

Conflicts