Enable importprivkey, addmultisigaddress in descriptor wallets #30175

1. 
   sipa commented at 1:17 pm on May 25, 2024: member

   Please describe the feature you’d like to see added.

   Descriptor wallets currently do not support the “legacy” import commands importprivkey, importpubkey, importaddress, addmultisigaddress, importmulti, and importwallet. This was done because the semantics of these RPC cannot be replicated in descriptor wallets, as they just fundamentally work differently (being explicit about what to watch rather than “whatever matches the bag of keys and scripts we have”).

   I want to suggest the possibility to reconsider that. The exact semantics cannot be replicated, but ignoring that, for many of these RPCs, the almost certain intent of the RPCs can be replicated:

   • importprivkey and importpubkey can be mapped to a combo() descriptor
   • importaddress can be mapped to an addr() descriptor
   • addmultisigaddress can be mapped to a multi() descriptor, wrapped in sh(), sh(wsh()), or wsh() depending on the address type (already returned as the descriptor field in the RPC output).
   • importmulti and importwallet are more involved, but do contain enough information for mapping to descriptors.

   For the simpler ones out of these, it makes sense too. importprivkey is simpler to use than learning the descriptor syntax, and no less safe. A reason not to do this is if a user would reasonably expect different semantics, or if there are guides out there that use certain RPCs in a way that relies on the “bag of keys and scripts” behavior, but I doubt it. Another reason not to do this is lack of taproot support in these RPCs, though that could be addressed with warnings, or in some cases, extensions (e.g. nothing prevents us from adding a taproot variant to addmultisigaddress).

   Is your feature related to a problem, if so please describe it.

   I’ve seen several complaints about how hard importdescriptors is in use for importing a private key, or creating a multisig address.

   Describe the solution you’d like

   No response

   Describe any alternatives you’ve considered

   No response

   Please leave any additional context

   No response

2. sipa added the label Feature on May 25, 2024
3. 
   furszy commented at 2:58 pm on May 25, 2024: member

   I think importaddress and addmultisigaddress intents are clear enough and they map well to descriptors. Both specify the address type. Forcing users to learn how to craft a descriptor when they merely want to watch an address seems overwhelming to me. -> #27034 maps importaddress to addr() and raw() descriptors. And #28307 fixes and extends createmultisig/addmultisigaddress.

   Not sure about importprivkey as it would clash with the new void(KEY) descriptor (#29136). The intent there isn’t clear enough to me. Perhaps we could create a importkey or importwalletkey command where the user can input any type of key and specify its intent in one of the command arguments. Personally, I think the combo() descriptor should be more of a last-resort fallback that requires a warning, rather than the user’s first choice, as it does not support newer output types.

4. maflcko added the label RPC/REST/ZMQ on May 25, 2024
5. 
   achow101 commented at 1:26 am on June 5, 2024: member

   addmultisigaddress might be a little bit more complicated since users would expect it to behave the same way and be able to have the specific private keys from addresses. Given that we do not want to allow exposing child private keys, retaining that behavior would allow extracting specific child private keys in a rather roundabout way (addmultisigaddress which puts creates a descriptor with a child privkey, then listdescriptors true which outputs the descriptor with that privkey).

   Also both addmultisigaddress and createmultisig should allow xpubs and xprvs, currently only single keys are allowed.

   I’m not sure that allowing importmulti is that useful. All of the stuff that you can do with it can be done with importdescriptors, and extremely similarly as importdescriptors’ interface was based off of importmulti. In fact, if importing a descriptor with importmulti, the same argument can be used with importdescriptors. I suppose someone could be used to importmulti with importing all of the other stuff that it allows, but it’s quite a mess and doesn’t seem like it’s used that much? I have seem requests for importprivkey, importaddress, and importwallet for descriptor wallets, but never for importmulti for things that importdescriptors can’t do.

   Not sure about importprivkey as it would clash with the new void(KEY) descriptor (#29136).

   I think the distinction is clear enough given importprivkey’s historical behavior, and that void(KEY) and addhdkey would be new. Namely, the expectation is that importprivkey imports a private key which produces all of the scripts produced by combo(). void(KEY) and addhdkey explicitly are adding a key that can sign but cannot produce any scripts.

6. 
   sipa commented at 1:14 pm on June 12, 2024: member

   @achow101 Hmm, what would happen if addmultisigaddress for descriptor wallets just imports the computed descriptor as it does now (with RPC arguments that must be existing addresses or hex pubkeys)? Would it support (participating in) signing for the resulting multisig address (assuming signing for one of specified addresses/pubkeys was possible prior to the RPC call)? Would listdescriptors true reveal the corresponding private key?

   Agreed about importmulti. It’s nontrivial to map to descriptors, and probably does not have much demand.

7. 
   maflcko commented at 1:27 pm on June 12, 2024: member

   I think the issue remains that the rescan argument is boolean and optional in importaddress, which IIRC was one of the reasons to move toward importdescriptors. Yes, the importdescriptors interface is non-trivial, but I don’t see how the burden can be taken off the user to think about the rescan timestamp of the descriptor. It needs to be accurate, because if it is too late, funds/txs may be missed, if it is too early, hours/days may be spent on a rescan.

   If this is done, I’d say that importaddress should come with a breaking change for descriptor wallets to adjust the rescan argument to be identical in behavior to the one of importdescriptors.

8. bitcoin deleted a comment on Jun 12, 2024
9. bitcoin deleted a comment on Jun 12, 2024
10. 
    achow101 commented at 10:39 pm on July 9, 2024: member

    Hmm, what would happen if addmultisigaddress for descriptor wallets just imports the computed descriptor as it does now (with RPC arguments that must be existing addresses or hex pubkeys)? Would it support (participating in) signing for the resulting multisig address (assuming signing for one of specified addresses/pubkeys was possible prior to the RPC call)? Would listdescriptors true reveal the corresponding private key?

    That breaks current invariants in descriptor wallets that have private keys. We enforce that all descriptors being loaded in a wallet with private keys has at least one private key somewhere in that descriptor. So adding the multisig descriptor without any private keys would violate that.

Contributors
sipa furszy achow101 maflcko

Labels
Feature RPC/REST/ZMQ