Skip to content
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

wallet: listdescriptors uses normalized descriptor form #21277

Merged
merged 1 commit into from
Feb 26, 2021
Merged

wallet: listdescriptors uses normalized descriptor form #21277

merged 1 commit into from
Feb 26, 2021

Conversation

S3RK
Copy link
Contributor

@S3RK S3RK commented Feb 23, 2021 •
edited
Loading

Rationale: show importable descriptors with listdescriptors RPC

It uses #19136 to derive xpub at the last hardened step.

Before:

[
    {
      "desc": "wpkh(tpubD6NzVbkrYhZ4YUQRJL49TWw1VR5v3QKUNYaGGMUfJUm19x5ZqQ2hEiPiYbAQvD2nHoPGQGPg3snLPM8sjmYpvx7XQhkmyfk8xhsUwKbXzyh/84'/1'/0'/0/*)#p4cn3erf",
      "timestamp": 1613982591,
      "active": true,
      "internal": false,
      "range": [
        0,
        999
      ],
      "next": 0
    },
    ...
]

After:

[
  {
    "desc": "wpkh([d4ade89c/84'/1'/0']tpubDDUEYcVXy6Vh5meHvcXN3sAr4k3fWwLZGpAHbkAHL8EnkDxp4d99CjNhJHfM2fUJicANvAKnCZS6XaVAgwAeKYc1KesGCN5qbQ25qQHrRxM/0/*)#8wq8rcft",
    "timestamp": 1613982591,
    "active": true,
    "internal": false,
    "range": [
      0,
      999
    ],
    "next": 0
  },
  ...
]

@S3RK
Copy link
Contributor Author

S3RK commented Feb 23, 2021

cc @achow101 @laanwj

@laanwj
Copy link
Member

laanwj commented Feb 23, 2021

Concept ACK! Thank you.

@laanwj laanwj mentioned this pull request Feb 23, 2021
Closed
@bitcoin bitcoin deleted a comment from AKZ98 Feb 23, 2021
@luke-jr
Copy link
Member

luke-jr commented Feb 24, 2021

What does it currently return here? Before/after would be nice. Maybe it's ToString that needs to get revised?

Would be nice to avoid needing the wallet unlocked to simply listdescriptors...

@S3RK
Copy link
Contributor Author

S3RK commented Feb 24, 2021 •
edited
Loading

What does it currently return here? Before/after would be nice. Maybe it's ToString that needs to get revised?

Updated description to show before/after. ToString() is fine, it's just ToNormalizedString() is better in this case.

Would be nice to avoid needing the wallet unlocked to simply listdescriptors...

In general I agree, but to derive normalised descriptors for hardened derivations we need private keys.

UPD: Apparently, I didn't save the updated description at time of posting. Fixed now

@laanwj
Copy link
Member

laanwj commented Feb 25, 2021 •
edited
Loading

I'd agree that needing to have the wallet unlocked is a drawback.

But the thing is that the current output is not really useful for anything (I think). You could say it's not even a valid descriptor as it does a hardened dirivation from a xpub.

E.g. like

    "desc": "sh(wpkh(tpubD6NzVbkrYhZ4Yj6Abshu22tUGYKVK6xcazXuGmxRkLTqrMfpc6u7Bpr31wmqe8kJc7Y5ZCvKdvVRtsZhfB5hdLpUndFFc5mXat72euorqFE/49'/1'/0'/1/*))#g7vg94cx",

In general I agree, but to derive normalised descriptors for hardened derivations we need private keys.

Hmm. It might be an idea to store these at generation time.

@S3RK
Copy link
Contributor Author

S3RK commented Feb 25, 2021

Hmm. It might be an idea to store these at generation time.

It's totally doable, but doesn't it violate users security assumptions? If I have my master private key encrypted at rest I would be unpleasantly surprised to find child private keys in memory.

@laanwj
Copy link
Member

laanwj commented Feb 25, 2021 •
edited
Loading

If I have my master private key encrypted at rest I would be unpleasantly surprised to find child private keys in memory.

I mean the public key, not the private key. This is all about public derivation right? I'm certainly not suggesting private keys should be stored unencrypted.

@achow101
Copy link
Member

The xpub cache should actually allow us to generate the normalized form without needing to unlock the wallet. After all, that is how we are able to refresh the address pool when the wallet is locked. However modifying the normalization function to do that may be an invasive change. I can look into doing that. Regardless, doing that would not materially effect this PR and I think it can be done in a followup.

ACK a69c3b3

@laanwj laanwj merged commit 8d37841 into bitcoin:master Feb 26, 2021
@S3RK S3RK deleted the listdescriptors_normalized branch February 26, 2021 13:27
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 26, 2021
@laanwj
Merge bitcoin#21277: wallet: listdescriptors uses normalized descript…
e9cbf91 
…or form

a69c3b3 wallet: listdescriptors uses normalized descriptor form (Ivan Metlushko)

Pull request description:

  Rationale: show importable descriptors with `listdescriptors` RPC

  It uses bitcoin#19136 to derive xpub at the last hardened step.

  **Before**:
  ```
  [
      {
        "desc": "wpkh(tpubD6NzVbkrYhZ4YUQRJL49TWw1VR5v3QKUNYaGGMUfJUm19x5ZqQ2hEiPiYbAQvD2nHoPGQGPg3snLPM8sjmYpvx7XQhkmyfk8xhsUwKbXzyh/84'/1'/0'/0/*)#p4cn3erf",
        "timestamp": 1613982591,
        "active": true,
        "internal": false,
        "range": [
          0,
          999
        ],
        "next": 0
      },
      ...
  ]
  ```

  **After**:
  ```
  [
    {
      "desc": "wpkh([d4ade89c/84'/1'/0']tpubDDUEYcVXy6Vh5meHvcXN3sAr4k3fWwLZGpAHbkAHL8EnkDxp4d99CjNhJHfM2fUJicANvAKnCZS6XaVAgwAeKYc1KesGCN5qbQ25qQHrRxM/0/*)#8wq8rcft",
      "timestamp": 1613982591,
      "active": true,
      "internal": false,
      "range": [
        0,
        999
      ],
      "next": 0
    },
    ...
  ]
  ```

ACKs for top commit:
  achow101:
    ACK a69c3b3

Tree-SHA512: 4f92e726cb8245aa0b520729cfd272945f0c66830f0555544e0618883aca516318543fa6ab1862058c64b4e4ed54ad1da953e032f4846eef7454122031c1b005
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021
@S3RK @luke-jr
wallet: listdescriptors uses normalized descriptor form
affd76a 
Github-Pull: bitcoin#21277
Rebased-From: a69c3b3
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AKZ98 AKZ98 AKZ98 approved these changes

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@S3RK @laanwj @luke-jr @achow101 @AKZ98 @fanquake