Based on #14454 #14565, last two commits only are for review.
Best reviewed with ?w=1
Allows a descriptor to be imported into the wallet using importmulti RPC. Start and end of range can be specified for ranged descriptors. The descriptor is implicitly converted to old structures on import.
Also adds a simple test of a P2SH-P2WPKH address being imported as a descriptor. More tests to come, as well as release notes.
1279+ // This is necessary to force the wallet to import the pubKey
1280+ CScript raw_pubkey_script = GetScriptForRawPubKey(x.second);
1281+ if (::IsMine(*pwallet, raw_pubkey_script) == ISMINE_SPENDABLE) {
1282+ throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this public key");
1283+ }
1284+ if (!pwallet->AddWatchOnly(raw_pubkey_script, timestamp)) {
For the same reason as pointed out in the other PR, you can’t do this; you’re going to mark payments to individual multisig pubkeys as incoming payments.
You’ll need a way to restrict this to P2PK, P2WPKH, and P2SH/P2WSH wrapped versions of those.
EDIT: I realize that when it’s about private keys, the same effect applies too, and there it can’t be avoided. Perhaps this stuff is just to scary, and we should wait until there’s a way to actually specify what to treat as ours explicitly…
Restricting it from multisig(to avoid being tricked as you mention) would make this even more confusing to a user.
Unfortunate.
1329+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Incompatibility found between watchonly and keys");
1330+ }
1331
1332- success = true;
1333- }
1334+ if (data.exists("scriptPubKey")) {
0if (data.exists("scriptPubKey") && data.exists("descriptor")) {
1 // throw error because these should be exclusive?
2}
and add test.
1237+ auto parsed_desc = Parse(descriptor, keys);
1238+ if (!parsed_desc) {
1239+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor is invalid");
1240+ }
1241+ if (!parsed_desc->IsRange() && data.exists("range")) {
1242+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Range should not be specified for an un-ranged descriptor");
1239+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor is invalid");
1240+ }
1241+ if (!parsed_desc->IsRange() && data.exists("range")) {
1242+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Range should not be specified for an un-ranged descriptor");
1243+ } else if (parsed_desc->IsRange() && !data.exists("range")) {
1244+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Descriptor is ranged, please specify the range");
1243+ } else if (parsed_desc->IsRange() && !data.exists("range")) {
1244+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Descriptor is ranged, please specify the range");
1245+ }
1246+ FlatSigningProvider out_keys;
1247+ std::vector<CScript> script_pub_keys;
1248+ for (int i = range_start; i <= range_end; i++) {
++i.
1284+ if (!pwallet->AddWatchOnly(raw_pubkey_script, timestamp)) {
1285+ throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
1286+ }
1287+ }
1288+ // Import private keys.
1289+ for (size_t i = 0; i < priv_keys.size(); i++) {
++i;
1334+ if (data.exists("scriptPubKey")) {
1335+ ProcessImportLegacy(pwallet, data, timestamp);
1336+ } else if (data.exists("descriptor")) {
1337+ ProcessImportDesc(pwallet, data, timestamp);
1338+ } else {
1339+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Either a descriptor or scriptPubKey must be provided.");
1332- success = true;
1333- }
1334+ if (data.exists("scriptPubKey")) {
1335+ ProcessImportLegacy(pwallet, data, timestamp);
1336+ } else if (data.exists("descriptor")) {
1337+ ProcessImportDesc(pwallet, data, timestamp);
ProcessImportDescriptor.
0@@ -0,0 +1,6 @@
1+Low-level RPC changes
2+----------------------
- :sweat_smile:
1224+ const UniValue& priv_keys = data.exists("keys") ? data["keys"].get_array() : UniValue();
1225+
1226+ const UniValue& range = data.exists("range") ? data["range"].get_array() : UniValue();
1227+ const int64_t range_start = range.size() > 0 ? range[0].get_int64() : 0;
1228+ const int64_t range_end = range.size() > 1 ? range[1].get_int64() : range_start;
1229+ if(range.size() > 2) {
if.
Rebased on #14565
Still planning on adding more tests + release notes, please don’t nitpick the lack of tests yet
Concept ACK, lightly tested 6894bf5 and it’s able to import a test watch-only wallet I used previously.
I didn’t use the watchonly while importing a descriptor with an xpub, should that be allowed? Or should watchonly argument not be allowed when using a descriptor?
Much smaller code change than I would have thought, nice!
It would be nice if range had a default of [0, DEFAULT_KEYPOOL_SIZE] or even using the -keypool argument.
Can you add a usage example with a descriptor?
When I cherry-pick #14477 on top of this and inspect a used address with getaddressinfo, the origin info in the descriptor seems both wrong and incomplete: "desc": "wpkh([224885f8]026...)", where 224885f8 is not the master fingerprint I used, and derivation info is missing. There might be some reusable stuff in #14021.
If I use watchonly: true, then getaddressinfo doesn’t show a descriptor and it says solvable: false, which seems wrong (the latter also happens without cherry-pick).
@Sjors thanks for the feedback :)
If I use watchonly: true, then getaddressinfo doesn’t show a descriptor and it says solvable: false, which seems wrong (the latter also happens without cherry-pick).
It should only watch for the scriptPubKey and import no other information, watch-only is a different requirement than being solvable without private keys.
the origin info in the descriptor seems both wrong and incomplete: “desc”: “wpkh([224885f8]026…)”, where 224885f8 is not the master fingerprint I used, and derivation info is missing.
You’re right, good point. I’ll take a look at andrews PR
I didn’t use the watchonly while importing a descriptor with an xpub, should that be allowed? Or should watchonly argument not be allowed when using a descriptor?
I think it should be allowed, because of the above reason. Using an xpub without watch only allows it to be solvable without being spendable
@MeshCollider @Sjors I was imagining that “watchonly” would be implicit when using descriptors (addr() and raw() would be watchonly; anything else would result in a solvable result).
The reason for “watchonly” is so that users need to be explicit about the fact they don’t want solvability (generally, you should always want solvability, but if you can’t, you can tell importmulti that it’s fine without).
@sipa I like your suggestion. In that case we should disallow usage of the watchonly param when combined with a descriptor.
Regarding getting origin information from the descriptors, @achow101 just rebased #14021 on top of this PR. It’s a non trivial change. Perhaps for this PR it’s best to not store origin information. Just make sure that if you do walletcreatefundedpsbt with bip32 flag set to true, then the result doesn’t crash decodepsbt.
addr(...). Mandating that removes ambiguity from how a descriptor ends up in a wallet.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
Can you rebase this on most recent #14565 ? It has a few logic fixes. discussing what I think are bugs
In addition, you can not ignore the watch_only field for descriptor import. watch_only needs to be set to true if the descriptor deals with public keys only: https://github.com/bitcoin/bitcoin/pull/14565/files#diff-522490d83dce5375d423b23886e4125eR1019 (this code is from the refreshed dependent PR)
By “haven’t tested” you mean “haven’t compiled” right? :-)
It looks like a tricky rebase. E.g. your code is referring to import_data.pubkeys and import_data.privkeys, the latter has been renamed to import_data.used_keys, but the former doesn’t exist anymore.
The ProcessImport function is now duplicated.
I’ve now finished rebasing, lightly tested, added some warnings, and added a couple more tests.
There is one thing that still needs addressing, which is that a descriptor which provides a private key should import that private key. Currently it only Expands() the descriptor and imports the public keys. The implementation of that is likely going to require some discussion so I will open a future PR for that, and leave it as a TODO for now.
1191+}
1192+
1193+static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
1194+{
1195+ try {
1196+ bool watch_only = data.exists("watchonly") ? data["watchonly"].get_bool() : false;
Concept ACK. I’ve had a quick skim of the code. A couple of high-level questions:
importdescriptor RPC method? There are enough modal arguments (eg redeemscript/witnessscript/pubkeys/watchonly(?) should only be used when importing addresses/scripts, range should only be used when importing descriptors) that it seems like this makes sense as a separate method.keys argument for importing private keys with a descriptor? Can’t you just include the private keys directly in the descriptor?
@jnewbery @Sjors as mentioned above
There is one thing that still needs addressing, which is that a descriptor which provides a private key should import that private key. Currently it only Expands() the descriptor and imports the public keys. The implementation of that is likely going to require some discussion so I will open a future PR for that, and leave it as a TODO for now.
So right now, the “keys” field is the only way to import a private key. That is because the descriptor code doesn’t currently support deriving specific private keys, Expand() only provides public keys and scripts. I have the code for importing private keys too ready, so let’s discuss that issue there when I open a PR. EDIT: #15024
1131- // TODO: add warnings if private keys don't correspond to watch-only
1132+ // Check if all the public keys have corresponding private keys in the import for spendability.
1133+ // This does not take into account threshold multisigs which could be spendable without all keys
1134+ bool spendable = std::all_of(pubkey_map.begin(), pubkey_map.end(), [&](const std::pair<CKeyID, CPubKey>& used_key){ return privkey_map.count(used_key.first) > 0; });
1135+ if (!watch_only && !spendable) {
1136+ warnings.push_back("Some private keys are missing, outputs will be considered watchonly. If this is intentional, specify the watchonly flag.");
re: #14491 (review)
s/specify the watchonly flag/specify the watchonly flag to suppress this warning/
Note: implementing this suggestion would make the warning text vary between ProcessImportLegacy and ProcessImportDescriptor.
1134+ bool spendable = std::all_of(pubkey_map.begin(), pubkey_map.end(), [&](const std::pair<CKeyID, CPubKey>& used_key){ return privkey_map.count(used_key.first) > 0; });
1135+ if (!watch_only && !spendable) {
1136+ warnings.push_back("Some private keys are missing, outputs will be considered watchonly. If this is intentional, specify the watchonly flag.");
1137+ }
1138+ if (watch_only && spendable) {
1139+ warnings.push_back("All private keys are provided, outputs will be considered spendable. If this is intentional, do not specify the watchonly flag.");
640+ self.log.info("Ranged descriptor import should fail without a specified range")
641+ self.test_importmulti({"desc": desc,
642+ "timestamp": "now"},
643+ False,
644+ error_code=-8,
645+ error_message='Descriptor is ranged, please specify the range')
end range suffices.
end counts as specifying the range, considering the helptext for the rpc
651+ "range": {"end": 1}},
652+ True,
653+ warnings=["Some private keys are missing, outputs will be considered watchonly. If this is intentional, specify the watchonly flag."])
654+ for address in addresses:
655+ self.test_address(address,
656+ solvable=True)
Parse to ParseDescriptor?
1116@@ -1108,14 +1117,25 @@ static UniValue ProcessImportDescriptor(ImportData& import_data, std::map<CKeyID
1117 }
1118 CPubKey pubkey = key.GetPubKey();
1119 CKeyID id = pubkey.GetID();
1120+
1121 // Check if this private key corresponds to a public key from the descriptor
1122 if (!pubkey_map.count(id)) {
1123- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Unused private key provided");
1124+ warnings.push_back("Ignoring irrelevant private key.");
In commit “[wallet] Allow descriptor imports with importmulti” (a0e341061463a25917d9faf4099219e71f982564)
It seems like all these warnings should be errors. I think the only reason for having warnings and supporting these nonsensical rpc calls was to provide backwards compatibility, which shouldn’t be a concern here.
re: #14491 (review)
These comments don’t appear to be resolved, but I think that’s ok.
For readability it might help if you split 9738518cf2e09cc77f252ccf2bc7ee2d938ed5c9 into:
ProcessImportLegacyI’d also like to see slightly more tests, e.g. one with a multisig descriptor.
@MeshCollider - I know you’re travelling so no rush on an answer to this, but do you have thoughts about my questions in IRC:
015:40 < jnewbery> meshcollider: what's the eventual plan for private keys in importmulti with descriptors? After [#15024](/bitcoin-bitcoin/15024/), there are two ways of importing the privkeys: either in the descriptor itself or with the keys argument. Would it be better to only allow importing privkeys from the descriptor itself?
115:42 < jnewbery> If you agree, then I think it makes sense to flip the ordering of the PRs (ie reduce 15024 to just commit https://github.com/bitcoin/bitcoin/pull/15024/commits/8a270b9582d82991766172430d508a1a7358e80d and merge that first, and then have 14491 following it)
you mean deprecate keys in importmulti or error if keys and desc are used?
We can’t deprecate keys in importmulti. That field is still required if importing a script not using a descriptor. My suggestion would be to error if desc and keys are provided in the same import object (in the same way that this PR errors if both desc and scriptPubKey are provided).
But perhaps I don’t have full context, and importing with a descriptor and private keys provided separately is important functionality. Looking forward to hearing meshcollider’s input.
1230-
1231- result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, "Missing required fields"));
1232- }
1233- if (warnings.size()) result.pushKV("warnings", warnings);
1234- return result;
1235+ if (warnings.size()) result.pushKV("warnings", warnings);
warnings and result handling here are an unintentional reversion of the changes from #14565: https://github.com/bitcoin/bitcoin/pull/14565/files#diff-522490d83dce5375d423b23886e4125eR1122.
0@@ -0,0 +1,4 @@
1+Descriptor import support
2+---------------------
3+
4+The `importmulti` RPC now supports importing of addresses from descriptors. A "desc" parameter can be provided instead of the "scriptPubKey" in a request, as well as an optional range for ranged descriptors to specify the start and end of the range to import.
I think there are still some residual issues from the rebase (see my comment on the warnings and result handling).
I agree with @Sjors comment here: #14491#pullrequestreview-189838451 that this would be much easier to review if you re-arranged the commits. The first commit is a combination of code-move (from ProcessImport() to ProcessImportLegacy()); refactor of ProcessImport(); and new functionality. If you split that into 3 or more commits, it’d be easier for reviewers. I’d also suggest squashing the following into earlier commits once that’s done:
Happy to help with some of that if you need a hand.
1162+
1163+ for (auto const& x : out_keys.scripts) {
1164+ import_data.import_scripts.emplace(x.second);
1165+ }
1166+
1167+ std::copy(out_keys.pubkeys.begin(), out_keys.pubkeys.end(), std::inserter(pubkey_map, pubkey_map.end()));
re: #14491 (review)
In commit “[wallet] Allow descriptor imports with importmulti” (a0e341061463a25917d9faf4099219e71f982564)
I don’t think this is correct, as it will cause import of all involved public keys as watch-only (even for example the public keys that are part of a multisig).
Is this issue still unresolved? In earlier discusion #14491 (review), it seemed to be. Either way it would be helpful to have a short comment here saying what effect adding these to pubkey_map has on solvability or wallet balances or whatever.
re: #14491 (review)
For future reference, this was fixed by removing out.pubkeys.emplace in #15263.
I think it would be helpful to have a comment here noting that this imports the public keys which occur inside P2PKH and P2WPKH descriptors, but not those inside multisig descriptors. It would also be great to include or link to sipa’s rationale for this in #14491 (comment)
I’ve had a go at rearranging the commits for this PR in a branch here: https://github.com/jnewbery/bitcoin/tree/pr14491.rearrange. It splits up the code move and new functionality, so with -w and --color-moved=zebra for the move commits, it should be a lot easier to review (review hints are in the commit log). I’ve also made a very minor change that undoes the reversion here: #14491 (review).
I think splitting up the commits in this way makes the PR much easier to review. @MeshCollider - feel free to grab that branch if you agree.
@jnewbery thanks, that is indeed way more readable. For those looking on Github, you can use ?w=1. So that just leaves my request for more tests.
I’m quite happy with how short the meat of this change in https://github.com/bitcoin/bitcoin/commit/e7c45d7eb425fbe8c7b9f92d21d10c99145d3771 is.
wallet_importmulti.py fails when rebased on master due to test refactor.
I’ve rebased my branch on master and fixed the test here: https://github.com/jnewbery/bitcoin/commit/087ba0734ee24438b2f495367c71fb257bb5b760
1290 }
1291
1292 // Check whether we have any work to do
1293- if (::IsMine(*pwallet, script) & ISMINE_SPENDABLE) {
1294- throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script");
1295+ for (const CScript& script : script_pub_keys) {
954@@ -942,9 +955,7 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
955 const std::string& witness_script_hex = data.exists("witnessscript") ? data["witnessscript"].get_str() : "";
956 const UniValue& pubKeys = data.exists("pubkeys") ? data["pubkeys"].get_array() : UniValue();
957 const UniValue& keys = data.exists("keys") ? data["keys"].get_array() : UniValue();
958- const bool internal = data.exists("internal") ? data["internal"].get_bool() : false;
959 const bool watchOnly = data.exists("watchonly") ? data["watchonly"].get_bool() : false;
960- const std::string& label = data.exists("label") ? data["label"].get_str() : "";
961
962 // Generate the script and destination for the scriptPubKey provided
963 CScript script;
In commit “[wallet] Refactor ProcessImport()” (34a7ce3d5cf3651e117502ab742087e966cd6ef2)
It’s confusing that these script and dest variables still declared here at a top level, when their actual usages after this error-checking block are now gone. Would suggest dropping script variable and calling script_pub_keys.emplace(GetScriptForDestination(...)) directly, and just declaring dest variables as needed where they are used. It’s not good to have these generically named variables with 150 line scopes that just get shadowed later and never used.
script is used below, in the RecurseImportData call. I have reduced the scope of dest though.
1077+ have_solving_data = parsed_desc->IsSolvable();
1078+ const bool watch_only = data.exists("watchonly") ? data["watchonly"].get_bool() : false;
1079+
1080+ int64_t range_start = 0, range_end = 0;
1081+ if (!parsed_desc->IsRange() && data.exists("range")) {
1082+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Range should not be specified for an un-ranged descriptor");
In commit “[wallet] Allow descriptor imports with importmulti” (a0e341061463a25917d9faf4099219e71f982564)
Could also trigger an error if a range is specified for a non-descriptor import.
1087+ const UniValue& range = data["range"];
1088+ range_start = range.exists("start") ? range["start"].get_int64() : 0;
1089+ if (!range.exists("end")) {
1090+ throw JSONRPCError(RPC_INVALID_PARAMETER, "End of range for descriptor must be specified");
1091+ }
1092+ range_end = range["end"].get_int64();
In commit “[wallet] Allow descriptor imports with importmulti” (a0e341061463a25917d9faf4099219e71f982564)
Could error check for 0 <= start <= end
1126+ privkey_map.emplace(id, key);
1127+ }
1128+ }
1129+
1130+ // Check if all the public keys have corresponding private keys in the import for spendability.
1131+ // This does not take into account threshold multisigs which could be spendable without all keys
In commit “[wallet] Allow descriptor imports with importmulti” (a0e341061463a25917d9faf4099219e71f982564)
// This does not take into account threshold multisigs which could be spendable without all keys
What does this mean? Is this something that needs to be documented for the user? Something that needs to be fixed later? Comment should be made more clear.
Reviewed 087ba0734ee24438b2f495367c71fb257bb5b760 but not acking, since I haven’t thought through the effects of the new ProcessImportDescriptor function yet, especially the AddWatchOnly pubkey stuff discussed in #14491 (review) and #14491 (review).
The bulk of changes in this PR look very good, though, and the review comments I left are all minor, so feel free to ignore them. @MeshCollider it would be helpful you could followup on the two AddWatchOnly threads #14491 (review) #14491 (review) to say what the current status is.
I think you’ve now changed it to never importing public keys.
If I import “pkh(036f1aef8329e88a8e7dca56a4e8f908697555478b26709f7513eba54db4acea21)”, and subsequently call getaddressinfo 1H2JaLY37d3PeqGgMV6yDStzHqenoRcCYF, it says “solvable: False”. All information for solvability is available, however.
The following tests reproduces what @sipa found:
0 # Test importing of a P2PKH address via descriptor
1 key = get_key(self.nodes[0])
2 self.log.info("Should import a p2pkh address from descriptor")
3 self.test_importmulti({"desc": "pkh(" + key.pubkey + ")",
4 "timestamp": "now",
5 "label": "Descriptor import test"},
6 True,
7 warnings=["Some private keys are missing, outputs will be considered watchonly. If this is intentional, specify the watchonly flag."])
8 test_address(self.nodes[1],
9 key.p2pkh_addr,
10 solvable=True,
11 ismine=False,
12 label="Descriptor import test")
More IRC discussion: http://www.erisian.com.au/bitcoin-core-dev/log-2019-01-24.html#l-742
Beyond the immediate bug that’s been found, I’m confused about the pubkey issues being discussed: #14491 (review), #14491 (review). Just at a really high level it’s not clear to me if there is:
@ryanofsky I suspect this is subjective, but this is my view:
I believe the above is all possible with the current IsMine logic, so I think it’s the most obvious solution. It requires only importing the public keys which occur inside P2PKH/P2WPKH, but not those inside multisig.
1178+ std::vector<CScript> scripts_temp;
1179+ parsed_desc->Expand(i, keys, scripts_temp, out_keys);
1180+ std::copy(scripts_temp.begin(), scripts_temp.end(), std::inserter(script_pub_keys, script_pub_keys.end()));
1181+ }
1182+
1183+ for (auto const& x : out_keys.scripts) {
const auto for consistency
1203
1204+ // Check if all the public keys have corresponding private keys in the import for spendability.
1205+ // This does not take into account threshold multisigs which could be spendable without all keys.
1206+ // Thus, threshold multisigs without all keys will be considered not spendable here, even if they are,
1207+ // perhaps triggering a false warning message. This is consistent with the current wallet IsMine check.
1208+ bool spendable = std::all_of(out_keys.pubkeys.begin(), out_keys.pubkeys.end(), [&](const std::pair<CKeyID, CPubKey>& used_key){ return privkey_map.count(used_key.first) > 0; });
I think this line deserves to be 3 lines
0bool spendable = std::all_of(out_keys.pubkeys.begin(), out_keys.pubkeys.end(), [&](const std::pair<CKeyID, CPubKey>& used_key) {
1 return privkey_map.count(used_key.first) > 0;
2});
I would even argue it could do with 4
0bool spendable = std::all_of(out_keys.pubkeys.begin(), out_keys.pubkeys.end(),
1 [&](const std::pair<CKeyID, CPubKey>& used_key) {
2 return privkey_map.count(used_key.first) > 0;
3 });
1220+{
1221+ UniValue warnings(UniValue::VARR);
1222+ UniValue result(UniValue::VOBJ);
1223+
1224+ try {
1225+ const bool internal = data.exists("internal") ? data["internal"].get_bool() : false;
Could just do
0= data.exists("internal") && data["internal"].get_bool()
1285+ std::map<CKeyID, CKey> privkey_map;
1286+ std::set<CScript> script_pub_keys;
1287+ bool have_solving_data;
1288+
1289+ if (data.exists("scriptPubKey") && data.exists("desc")) {
1290+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Both a descriptor and a scriptPubKey should not be provided.");
"Descriptors and scriptPubKeys cannot be used at the same time." maybe?
This commit is move-only and doesn't make any functional changes. It
simply moves code around within ProcessImport() in preparation for
refactors in the next commits.
This commit adds a ProcessImportLegacy() function which
currently does nothing. It also unindents a block of
code for a future move-only change.
Reviewer hint: review with -w to ignore whitespace changes.
This is almost entirely a move-only commit.
Reviewer hint: use --color-moved=zebra for review.
1143+ const UniValue& priv_keys = data.exists("keys") ? data["keys"].get_array() : UniValue();
1144+
1145+ FlatSigningProvider out_keys;
1146+
1147+ // Expand all descriptors to get public keys and scripts.
1148+ // TODO: get private keys from descriptors too
expandRange method to Descriptor instance
1177+
1178+ // Check if all the public keys have corresponding private keys in the import for spendability.
1179+ // This does not take into account threshold multisigs which could be spendable without all keys.
1180+ // Thus, threshold multisigs without all keys will be considered not spendable here, even if they are,
1181+ // perhaps triggering a false warning message. This is consistent with the current wallet IsMine check.
1182+ bool spendable = std::all_of(pubkey_map.begin(), pubkey_map.end(),
In commit “[wallet] Allow descriptor imports with importmulti” (9f48053d8f9a1feacc96d7e2a00c8a3a67576948)
It would be really nice to move this duplicated watchonly checking code out of ProcessLegacy and ProcessImportDescriptor either up into ProcessImport or down to into a common helper function. The watchonly checks are weird enough to deal with once, much less twice in two functions with slightly different variable names and comments.
989+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Range should not be specified for a non-descriptor import");
990+ }
991+
992 // Generate the script and destination for the scriptPubKey provided
993 CScript script;
994- CTxDestination dest;
In commit “[wallet] Allow descriptor imports with importmulti” (9f48053d8f9a1feacc96d7e2a00c8a3a67576948)
Note: Changes here more are directly related to previous commit “[wallet] Refactor ProcessImport()” (a1b25e12a5f57048a4639964d57c0b46eb84cd4e) than this one.