Skip to content

util, rpc: Add parameter to deriveaddresses to display address information #25366

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
w0xlt wants to merge 2 commits into from
Closed

util, rpc: Add parameter to deriveaddresses to display address information #25366

w0xlt wants to merge 2 commits into from

Conversation

@w0xlt
Copy link
Contributor

@w0xlt w0xlt commented Jun 13, 2022 •
edited
Loading

This PR adds the details parameter to deriveaddresses RPC.
With this option enabled, this RPC will return the private and public key and the output script for each address, as seen below.

Motivation: I needed to retrieve the private key from an address generated by a descriptor wallet, but dumpprivkey is not available for new wallets.
This change allows extracting this type of information from an address generated by a descriptor.

If the descriptor contains xpub instead of xpriv, the private_key field will be omitted.
If the descriptor is non-ranged, the index field will be omitted.

This PR does not change the current behavior if the new parameter is not used.

Example:

$ bitcoin-cli -signet deriveaddresses "tr(tprv8ZgxMBicQKsPfMzHFsP1xhYbMGowTt7PPwkfsuBDhCN7rSnCTSConREDsnroDrtNtCCxzhGvhLYVCULPDtrzTV7w1wNWMPsBz2tbX3tKFRy/86'/1'/0'/0/*)#ndy4d8qe" "[0,2]" true
[
  {
    "index": 0,
    "address": "tb1p840d3777aztc9mpa4yndtk882ja8cpqstq96399gm5atk8ekt2ws3etmlg",
    "output_script": "51203d5ed8fbdee89782ec3da926d5d8e754ba7c0410580ba894a8dd3abb1f365a9d",
    "public_key": "02557828307dcf41e15753ec4c7e1f7af149dd85f8e2a10b81bc81174f9129997b",
    "private_key": "cP7pg6J5tFzAf1SiPVnJs12EPUgaiVh55uNbfaRgkjAr2cEqf1bK"
  },
  {
    "index": 1,
    "address": "tb1psjvm057x7v8rv45apk5jtplw5w49r9l4wn4q6mc3urdv57wg98zqnygxsq",
    "output_script": "51208499b7d3c6f30e36569d0da92587eea3aa5197f574ea0d6f11e0daca79c829c4",
    "public_key": "02c80f11f563cd9cbf5a26f335bb363fddb7155f97c1a3d26e0f2c8132df33b7d9",
    "private_key": "cPCwhcnJqeVEp2APohD11ZXqVhzEnvYmB37JHfif2sAJD5fZygiQ"
  },
  {
    "index": 2,
    "address": "tb1pggz3twdfe25cczrwc7mf3p3qyrp4m8f75t4ug8r0mfw9kaen3m0sfm3t3y",
    "output_script": "5120420515b9a9caa98c086ec7b698862020c35d9d3ea2ebc41c6fda5c5b77338edf",
    "public_key": "03f8441bf1a56ba725ab58084f449ec147c21359aa59441834df5f15097e846b1f",
    "private_key": "cRPpFpwTZ2P4Jz7Rna7A72caVhQEGyco1R8yHY1KV2s1KdrNEdkZ"
  }
]

@sipa
Copy link
Member

sipa commented Jun 14, 2022

That seems dangerous as descriptor wallets (by default) use unhardened derivation, so leaking one derived private key is sufficient for an attacker to compute every private key. The choice to only allow exporting the xprv was deliberate, as with that, it'd be more obvious to users that what was being revealed was their entire wallet.

Also: what does this do if multiple keys are involved?

@w0xlt
Copy link
Contributor Author

w0xlt commented Jun 14, 2022 •
edited
Loading

@sipa Yes, it's a good point. I initially considered that this might be as risky as running listdescriptors with the private parameter set to true.

I changed the code to support multiple keys (and added functional tests for this case).

Example:

./src/bitcoin-cli -signet deriveaddresses "wsh(multi(2,tprv8ZgxMBicQKsPePmENhT9N9yiSfTtDoC1f39P7nNmgEyCB6Nm4Qiv1muq4CykB9jtnQg2VitBrWh8PJU8LHzoGMHTrS2VKBSgAz7Ssjf9S3P/86'/1'/0'/0/*,tprv8ZgxMBicQKsPfMzHFsP1xhYbMGowTt7PPwkfsuBDhCN7rSnCTSConREDsnroDrtNtCCxzhGvhLYVCULPDtrzTV7w1wNWMPsBz2tbX3tKFRy/86'/1'/0'/0/*))#kjq88ke0" "0" true
[
  {
    "index": 0,
    "address": "tb1qupzqr3kdgn6zdjkemlfy5kytr42z96wsutjjylvj4qvwm2argvts62apa2",
    "output_script": "0020e04401c6cd44f426cad9dfd24a588b1d5422e9d0e2e5227d92a818edaba34317",
    "public_keys": [
      "02557828307dcf41e15753ec4c7e1f7af149dd85f8e2a10b81bc81174f9129997b",
      "03f1f8587257e595dd5a1699da22b749cb9bea5f5d0b9bccfb43f119092b1297de"
    ],
    "private_keys": [
      "cP7pg6J5tFzAf1SiPVnJs12EPUgaiVh55uNbfaRgkjAr2cEqf1bK",
      "cTDRhufTmDDw8hUGkpyY9bjX41Y4jiHYJVcZUpCeK3WJztm152hd"
    ]
  }
]

@achow101
Copy link
Member

achow101 commented Jun 14, 2022

I think the option for private keys should be separate from the option for other details. I am also hesitant about outputting private keys because of the unhardened derivation problem.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 15, 2022 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26039 (rpc: Run type check against RPCArgs (1/2) by MarcoFalke)
  • #24162 (rpc: add require_checksum flag to deriveaddresses by kallewoof)
  • #23549 (Add scanblocks RPC call (attempt 2) by jamesob)
  • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string 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.

This was referenced Jun 15, 2022
Closed
Merged
@w0xlt
Copy link
Contributor Author

w0xlt commented Jun 15, 2022

I removed the private keys from RPC result as it is raising concerns about unhardened derivation problem.
As mentioned before, my initial idea is to provide a way to get the private key (and also other information) about an address, as dumpprivkey is not available for descriptor wallets. Not sure if this PR is still useful without this field.

I think that separating private key option from the details can result in bad UX as the user can enter a xpub and set this option as true.

$ bitcoin-cli -signet deriveaddresses "wsh(multi(2,tprv8ZgxMBicQKsPePmENhT9N9yiSfTtDoC1f39P7nNmgEyCB6Nm4Qiv1muq4CykB9jtnQg2VitBrWh8PJU8LHzoGMHTrS2VKBSgAz7Ssjf9S3P/0/*,tpubDBYDcH8P2PedrEN3HxWYJJJMZEdgnrqMsjeKpPNzwe7jmGwk5M3HRdSf5vudAXwrJPfUsfvUPFooKWmz79Lh111U51RNotagXiGNeJe3i6t/1/*))#szn2yxkq" "0" true
[
  {
    "index": 0,
    "address": "tb1qqsat6c82fvdy73rfzye8f7nwxcz3xny7t56azl73g95mt3tmzvgs9a8vjs",
    "output_script": "0020043abd60ea4b1a4f4469113274fa6e3605134c9e5d35d17fd14169b5c57b1311",
    "public_keys": [
      "0281c133371848d11ac195189bdfd32c1bbf39ebc1ee1b627897afd2ff8770e77b",
      "03db7c6fb51e4386137e2a320c25a8681ebdc5130a902c7abb9fb94d7358f9ca70"
    ]
  }
]

@luke-jr
Copy link
Member

luke-jr commented Jun 18, 2022

(Arguably derivation means these "private keys" are really just a mid-state of the signature algorithm, and the entire wallet uses a single private key)

luke-jr
luke-jr reviewed Jun 18, 2022
View reviewed changes
src/rpc/output_script.cpp Outdated
Copy link
Member

@luke-jr luke-jr Jun 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Booleans should typically not be positional arguments, but instead members of an "options" Object.

Copy link
Contributor Author

@w0xlt w0xlt Jun 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 544c2cc. Thanks for review.

@w0xlt
Copy link
Contributor Author

w0xlt commented Jun 20, 2022

Force-pushed 544c2cc addressing @luke-jr's review.

@DrahtBot DrahtBot mentioned this pull request Jul 6, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request Sep 8, 2022
Merged
@w0xlt w0xlt mentioned this pull request Sep 13, 2022
Closed
@achow101
Copy link
Member

achow101 commented Oct 12, 2022

This PR does not seem to have conceptual support. Please leave a comment if you would like this to be reopened.

@achow101 achow101 closed this Oct 12, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@luke-jr luke-jr luke-jr left review comments

@Slavo77AssMiner Slavo77AssMiner Slavo77AssMiner approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@w0xlt @sipa @achow101 @DrahtBot @luke-jr @Slavo77AssMiner