Wallet with sh(wpkh( descriptor can generate bech32 address it doesn’t know #21268

issue laanwj openend this issue on February 22, 2021
  1. laanwj commented at 2:59 pm on February 22, 2021: member

    A non-descriptor watch-only wallet with imported sh(wpkh( descriptor can generate Bech32 addresses that don’t match the descriptor.

    Steps to reproduce:

     0$ bitcoind -regtest -daemon -addresstype=bech32
     1$ bitcoin-cli -regtest createwallet test true
     2$ bitcoin-cli -regtest -rpcwallet=test importmulti \
     3    '[{"desc":"sh(wpkh(tpubDCJtdt5dgJpdhW4MtaVYDhG4T4tF6jcLR1PxL43q9pq1mxvXgMS9Mzw1HnXG15vxUGQJMMSqCQHMTy3F1eW5VkgVroWzchsPD5BUojrcWs8/0/*))#e8nc36sh",
     4     "timestamp":0, "range":1000, "watchonly":true, "keypool":true}]'
     5[
     6  {
     7    "success": true 
     8  }              
     9]
    10$ bitcoin-cli -regtest -rpcwallet="test" getnewaddress
    11bcrt1quecj0a95x3s570h0rs7l2rxgmcudhphzl0g2q4
    12$ bitcoin-cli -regtest -rpcwallet="test" getaddressinfo bcrt1quecj0a95x3s570h0rs7l2rxgmcudhphzl0g2q4
    13{
    14  "address": "bcrt1quecj0a95x3s570h0rs7l2rxgmcudhphzl0g2q4",
    15  "scriptPubKey": "0014e67127f4b434614f3eef1c3df50cc8de38db86e2",
    16  "ismine": false,
    17  "solvable": false,
    18  "iswatchonly": false,
    19  "isscript": false,
    20  "iswitness": true,
    21  "witness_version": 0,
    22  "witness_program": "e67127f4b434614f3eef1c3df50cc8de38db86e2",
    23  "ischange": false,
    24  "labels": [
    25    ""
    26  ]
    27}
    

    This is quite unexpected. It’s generating Bech32 address from a sh(wpkh( descriptor. And not only that, the wallet isn’t watching the generated address. So it can be quite a footgun, which could lead to (perceived) funds loss.

    For example with HWI, coming from Trezor software which defaults to p2sh-segwit, people will be importing, say, sh(wpkh(m/49'/1'/0'/0/*)). If they forget to set the address type to p2sh-segwit, they’ll generate addresses that seem to come out of the blue.

    Expected behavior: getnewaddress fails with no key available.

  2. laanwj added the label Bug on Feb 22, 2021
  3. laanwj added the label Wallet on Feb 22, 2021
  4. laanwj commented at 2:59 pm on February 22, 2021: member

    This issue was found by @Kvaciral while testing HWI. For completeness it happened for a change address generated by walletcreatefundedpsbt (so on the internal chain), but I think this reproduces the same issue.

    Edit: For descriptor wallets it’s handled correctly:

    0$ bitcoin-cli -regtest -named createwallet wallet_name=test3 disable_private_keys=true descriptors=true
    1$ bitcoin-cli -regtest -rpcwallet=test3 importdescriptors  '[{"desc":"sh(wpkh(tpubDCJtdt5dgJpdhW4MtaVYDhG4T4tF6jcLR1PxL43q9pq1mxvXgMS9Mzw1HnXG15vxUGQJMMSqCQHMTy3F1eW5VkgVroWzchsPD5BUojrcWs8/0/*))#e8nc36sh",
    2     "timestamp":0, "range":1000, "watchonly":true, "active":true}]'
    3$ bitcoin-cli -regtest -rpcwallet="test2" getnewaddress 
    4error code: -12
    5error message:
    6Error: No bech32 addresses available.
    7$ bitcoin-cli -regtest -rpcwallet="test3" getnewaddress '' p2sh-segwit
    82NA1PWXse3JjGGMcyjMETTCQnTpsLtQETQW
    
  5. laanwj added the label Descriptors on Feb 22, 2021
  6. achow101 commented at 5:33 pm on February 22, 2021: member

    Making getnewaddress fail on legacy wallets is a non-trivial task, and given that descriptor wallets is the “correct” way to be importing descriptor, I think it would be better to start moving towards descriptor wallets.


    I think this issue is a regression. I remember specifically making sure that an imported descriptor would end up with watching all of the address types for the pubkeys in the descriptor. I thought there was a test for that too. Does this behavior occur in previous versions too?

  7. maflcko renamed this:
    Wallet with `sh(wpkh(` descrptor can generate bech32 address it doesn't know
    Wallet with `sh(wpkh(` descriptor can generate bech32 address it doesn't know
    on Feb 22, 2021
  8. laanwj commented at 5:51 pm on February 22, 2021: member

    Making getnewaddress fail on legacy wallets is a non-trivial task, and given that descriptor wallets is the “correct” way to be importing descriptor, I think it would be better to start moving towards descriptor wallets.

    Agree. We might want to update the HWI with Bitcoin Core documentation to say so then.

    Does this behavior occur in previous versions too?

    I don’t know, we only tried with master.

  9. i5hi commented at 1:14 am on April 19, 2021: none

    This is kind of a related issue,

    We have the following multisig descriptor that is resulting in some weird behaviour on Bitcoin-Core.

    0deposit: wsh(multi(3,[ad812a29/84h/1h/0h]tpubDChVss7wefA63SZ7JsVdepCaoyo3qPojScTwPfpGZ6g9rVo3BRgTDabngF398egL578Ne99LyFkRE59BgSaQStK67PwtFRR47XXL1NhHEpu/0/*,[c2b507ce/84h/1h/0h]tpubDDY1HyiMDReMYtUrivMvZFNS2QLosJqL2p68vSXS1KaU3ASZSWDhDB1QGks6C9Gb3uuuhjQ6Pa2an32ak4soGBGpPzzu4jp6jfYzWVPPuUL/0/*,[bae7ef16/84h/1h/0h]tpubDCAgz23qpZQ7iNsU9tAnEkjihrPeNFLYvJ2CuzMSUdqiBKh3i9mEV6C1rJXBFdLE6YoaJoBkTJg2o8EzjLvwasNmDhTDVuqYFJo6piCPFZv/0/*))#kh6e0vw7
    1 
    2change: wsh(multi(3,[ad812a29/84h/1h/0h]tpubDChVss7wefA63SZ7JsVdepCaoyo3qPojScTwPfpGZ6g9rVo3BRgTDabngF398egL578Ne99LyFkRE59BgSaQStK67PwtFRR47XXL1NhHEpu/1/*,[c2b507ce/84h/1h/0h]tpubDDY1HyiMDReMYtUrivMvZFNS2QLosJqL2p68vSXS1KaU3ASZSWDhDB1QGks6C9Gb3uuuhjQ6Pa2an32ak4soGBGpPzzu4jp6jfYzWVPPuUL/1/*,[bae7ef16/84h/1h/0h]tpubDCAgz23qpZQ7iNsU9tAnEkjihrPeNFLYvJ2CuzMSUdqiBKh3i9mEV6C1rJXBFdLE6YoaJoBkTJg2o8EzjLvwasNmDhTDVuqYFJo6piCPFZv/1/*))#yzxdvr0a
    

    This descriptor imports successfully with importdescriptors and the following commands produce correct results:

    listtransactions - shows the correct history

    walletcreatefundedpsbt - creates a valid psbt that we were able to sign and broadcast

    HOWEVER,

    getnewaddress produces this error:

    0error code: -4
    1error message:
    2Error: This wallet has no available keys
    

    The same error occurs when we use importmulti when used with keypool=true

    bitcoind logs:

     02021-04-17T10:58:24Z [sushi] Wallet File Version = 10500
     12021-04-17T10:58:24Z [sushi] Keys: 0 plaintext, 0 encrypted, 0 w/ metadata, 0 total. Unknown wallet records: 0
     22021-04-17T10:58:24Z [sushi] Performing wallet upgrade to 169900
     32021-04-17T10:58:24Z [sushi] Wallet completed loading in              32ms
     42021-04-17T10:58:24Z [sushi] setKeyPool.size() = 0
     52021-04-17T10:58:24Z [sushi] mapWallet.size() = 0
     62021-04-17T10:58:24Z [sushi] m_address_book.size() = 0
     72021-04-17T10:58:25Z [sushi] RescanFromTime: Rescanning last 16 blocks
     82021-04-17T10:58:25Z [sushi] Rescan started from block 000000000000000d9f908b4e00f92a8a495ec21e2591443e6850513ca8e21071...
     92021-04-17T10:58:25Z [sushi] AddToWallet 846afdf0956e94f6831ab650e6703a5b268c1633d6961214c73a1948aaba16c0  new
    102021-04-17T10:58:25Z [sushi] Rescan completed in 
    11.....
    12.....
    132021-04-17T11:02:19Z [sushi] Already have script 532102ea3cf7d84387559e16532e1cae88e8d7f5b5857d58971c7443fcd5eecbbee4a42102675f0fcd50d99243ec8ffde921d1f0170e2089f8fd2c77b1e12d99372027d3e82102eaa9675a7b957ae3421bd9a902c4bbbcb16085a7c3a4b4c278d34b70e101141e53ae
    
  10. achow101 commented at 2:16 am on April 19, 2021: member
    @vmenond What is your full importdescriptors command? You need to have "active":true in it in order for the getnewaddress to use the imported descriptor.
  11. i5hi commented at 7:29 am on April 19, 2021: none
    Cheers @achow101 ! That did it.
  12. meshcollider commented at 1:55 am on September 29, 2021: contributor
    Related #19686
  13. achow101 commented at 10:31 pm on October 26, 2022: member
    Going to close this as I don’t think there is much interest in non-critical fixes in the legacy wallet.
  14. achow101 closed this on Oct 26, 2022

  15. bitcoin locked this on Oct 26, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me