wallet: listdescriptors uses normalized descriptor form #21277

1. 
   S3RK commented at 9:03 am on February 23, 2021: member

   Rationale: show importable descriptors with listdescriptors RPC

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

   Before:

    0[
    1    {
    2      "desc": "wpkh(tpubD6NzVbkrYhZ4YUQRJL49TWw1VR5v3QKUNYaGGMUfJUm19x5ZqQ2hEiPiYbAQvD2nHoPGQGPg3snLPM8sjmYpvx7XQhkmyfk8xhsUwKbXzyh/84'/1'/0'/0/*)#p4cn3erf",
    3      "timestamp": 1613982591,
    4      "active": true,
    5      "internal": false,
    6      "range": [
    7        0,
    8        999
    9      ],
   10      "next": 0
   11    },
   12    ...
   13]
   

   After:

    0[
    1  {
    2    "desc": "wpkh([d4ade89c/84'/1'/0']tpubDDUEYcVXy6Vh5meHvcXN3sAr4k3fWwLZGpAHbkAHL8EnkDxp4d99CjNhJHfM2fUJicANvAKnCZS6XaVAgwAeKYc1KesGCN5qbQ25qQHrRxM/0/*)#8wq8rcft",
    3    "timestamp": 1613982591,
    4    "active": true,
    5    "internal": false,
    6    "range": [
    7      0,
    8      999
    9    ],
   10    "next": 0
   11  },
   12  ...
   13]
   

2. wallet: listdescriptors uses normalized descriptor form a69c3b35f8
3. fanquake added the label Wallet on Feb 23, 2021
4. 
   S3RK commented at 9:05 am on February 23, 2021: member

   cc @achow101 @laanwj
5. 
   laanwj commented at 9:06 am on February 23, 2021: member

   Concept ACK! Thank you.
6. AKZ98 approved
7. fanquake deleted a comment on Feb 23, 2021
8. 
   luke-jr commented at 3:33 am on February 24, 2021: member

   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…

9. 
   S3RK commented at 7:03 am on February 24, 2021: member

   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

10. 
    laanwj commented at 8:30 am on February 25, 2021: member

    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

    0    "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.

11. 
    S3RK commented at 9:03 am on February 25, 2021: member

    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.

12. 
    laanwj commented at 4:08 pm on February 25, 2021: member

    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.

13. 
    achow101 commented at 5:58 pm on February 25, 2021: 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 a69c3b35f8974b378a87a3e42d331bd4147e07df

14. laanwj merged this on Feb 26, 2021
15. laanwj closed this on Feb 26, 2021

    ---

16. S3RK deleted the branch on Feb 26, 2021
17. sidhujag referenced this in commit e9cbf91787 on Feb 26, 2021
18. luke-jr referenced this in commit affd76a0ae on Jun 27, 2021
19. DrahtBot locked this on Aug 18, 2022

Contributors
S3RK laanwj AKZ98 luke-jr achow101

Labels
Wallet