This is an alternative to #14558 (it will warn when fields are being ignored). In addition:
Travis is sad about trailing whitespace:
0This diff appears to have added new lines with trailing whitespace.
1The following changes were suspected:
2diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
3@@ -915,0 +969,10 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
4+
5^---- failure generated from test/lint/lint-whitespace.sh
I’ve also discovered when writing this that importmulti does not actually require that watchonly is set when no solvability is desired. I’ve kept the existing behavior for now, as it seems pretty invasive to people who may be relying on that, though I’ve added a TODO.
EDIT: seems I misunderstand the original purpose; the “watchonly” is there to support importing something as watch-only without importing all the private keys (and is thus about spendability, not about solvability). That both matches the name better, and the existing (lack of) errors related to it. It also makes more sense, as there is no point in providing keys/script when no solvability is desired, while it makes perfect sense to include private keys when no spendability is desired.
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.
825+ P2SH,
826+ WITNESS_V0,
827+};
828+
829+static void ProcessSolvingImportStep(const CScript& script, ImportData& data, SolverContext ctx)
830+{
It’s not immediately obvious what this is doing, until you read txnouttype Solver documentation in standard.h.
Suggested comment:
0// Use Solver to obtain script type and parsed pubkeys or hashes:
827+};
828+
829+static void ProcessSolvingImportStep(const CScript& script, ImportData& data, SolverContext ctx)
830+{
831+ std::vector<std::vector<unsigned char>> solverdata;
832+ txnouttype typ = Solver(script, solverdata);
script_type would be clearer
919@@ -840,195 +920,144 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
920 if (!IsHex(output)) {
921 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid scriptPubKey");
922 }
923-
924 std::vector<unsigned char> vData(ParseHex(output));
925 script = CScript(vData.begin(), vData.end());
926 if (!ExtractDestination(script, dest) && !internal) {
ExtractDestination(s) and various parsing happening below was also a ProcessSolvingImportStep (SolverContext::TOP or a new context?), but I suppose the different types make that difficult?
I think that would be completely the wrong place; perhaps the control flow isn’t clear?
ProcessSolvingImportStep is a recursive function that matches up the parsed solvability information (only the keys, pubkeys, redeemscript, and witnessscript fields) with the provided address/script, recursing into subscripts as needed. Anything that isn’t specific to the type of script being imported doesn’t belong there. In particular, labels are unrelated to script types.
I’ve added some comments and renamed the function to RecurseImportData. Perhaps that is clearer?
ProcessImport is fairly large with lots of if branches and loops, so splitting it into multiple functions would help. The first function could be prepare_import_data, the second verify_and_import_data which calls RecurseImportData and then a third add_to_wallet. Not sure if they can be cleanly disentangled. Maybe some other time.
1040+ }
1041
1042- if (!pwallet->HaveCScript(witness_id) && !pwallet->AddCScript(witness_script)) {
1043- throw JSONRPCError(RPC_WALLET_ERROR, "Error adding p2wsh witnessScript to wallet");
1044- }
1045+ // TODO: it seems we're not actually requiring watchonly to be set for non-solvable inputs, fix this
That’s a great idea. I’ve implemented that and taken it a bit further. I think all new error conditions (that don’t signify seriously broken things like P2SH-inside-P2SH) are now reported as warnings instead of causing the RPC to fail.
Perhaps we want to introduce a new mode where all these warnings become errors, though -deprecatedrpc, but let’s discuss that in a separate RPC.
1092- if (std::find(destinations.begin(), destinations.end(), dest) == destinations.end()) {
1093+ // Verify and process input data
1094+ bool spendable = !watchOnly;
1095+ if (!watchOnly) {
1096+ ProcessSolvingImportStep(script, import_data, SolverContext::TOP);
1097+ if (import_data.redeemscript) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Redeemscript provided for a non-P2SH script");
watchOnly into ProcessSolvingImportStep and check there?
watchonly is true, there is nothing to do for ProcessSolvingImportStep, as there is no script to be analysed (see other comment).
Concept ACK. The new recursive ProcessSolvingImportStep and SolverContext are a lot more readable! I imagine that function lends itself to (future) unit tests better.
There’s still a bunch of processing and checking happening outside of ProcessSolvingImportStep though, which is less easy to follow, but at least that code is now shorter.
Do the existing tests cover the new sanity checks you added?
There’s still a bunch of processing and checking happening outside of ProcessSolvingImportStep though, which is less easy to follow, but at least that code is now shorter.
That’s intentional. It’s a recursive function to deal with analysing script specific solvability information. Anything that isn’t specific to the script being imported isn’t in there.
Do the existing tests cover the new sanity checks you added?
Not yet, will add those soon.
804@@ -805,6 +805,91 @@ UniValue dumpwallet(const JSONRPCRequest& request)
805 return reply;
806 }
807
808+struct ImportData
809+{
810+ // Input data
811+ std::unique_ptr<CScript> redeemscript; //! Provided redeemScript; will be mvoed to `import_scripts` if relevant.
1225+ pwallet->SetAddressBook(dest, label, "receive");
1226+ }
1227
1228 UniValue result = UniValue(UniValue::VOBJ);
1229 result.pushKV("success", UniValue(true));
1230+ if (warnings.size()) result.pushKV("warnings", warnings);
883+ data.require_keys.emplace(id);
884+ data.used_keys.emplace(id);
885+ data.import_scripts.emplace(script); // Special rule for IsMine: native P2WPKH requires the full script imported
886+ break;
887+ }
888+ default:
TX_NONSTANDARD, TX_NULL_DATA and TX_WITNESS_UNKNOWN explicitly to make it exhaustive? That will allow for -Wswitch-enum :-)
825+ WITNESS_V0, //! P2WSH witnessScript
826+};
827+
828+// Analyse the provided script, process the import data accordinging, and recurse into subscripts if necessary.
829+// Returns an error string, or the empty string for success.
830+static std::string RecurseImportData(const CScript& script, ImportData& data, ScriptContext ctx)
ctx constant
solverdata
script
813+ std::map<CKeyID, CKey> privkeys;
814+
815+ // Output data
816+ std::set<CScript> import_scripts;
817+ std::set<CKeyID> used_keys; // Import these private keys if available
818+ std::set<CKeyID> require_keys; // Fail if these public keys are not available
845+ data.used_keys.emplace(id);
846+ break;
847+ }
848+ case TX_SCRIPTHASH: {
849+ if (ctx == ScriptContext::P2SH) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Trying to nest P2SH inside another P2SH");
850+ if (ctx == ScriptContext::WITNESS_V0) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Trying to nest P2SH inside a P2WSH");
ctx == TOP
868+ uint256 fullid(solverdata[0]);
869+ CScriptID id;
870+ CRIPEMD160().Write(fullid.begin(), fullid.size()).Finalize(id.begin());
871+ auto subscript = std::move(data.witnessscript);
872+ if (!subscript) return "missing witnessscript";
873+ if (CScriptID(*subscript) != id) return "witnessScript does not match the scriptPubKey or redeemScript";
849+ if (ctx == ScriptContext::P2SH) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Trying to nest P2SH inside another P2SH");
850+ if (ctx == ScriptContext::WITNESS_V0) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Trying to nest P2SH inside a P2WSH");
851+ CScriptID id = CScriptID(uint160(solverdata[0]));
852+ auto subscript = std::move(data.redeemscript);
853+ if (!subscript) return "missing redeemscript";
854+ if (CScriptID(*subscript) != id) return "redeemScript does not match the scriptPubKey";
869+ CScriptID id;
870+ CRIPEMD160().Write(fullid.begin(), fullid.size()).Finalize(id.begin());
871+ auto subscript = std::move(data.witnessscript);
872+ if (!subscript) return "missing witnessscript";
873+ if (CScriptID(*subscript) != id) return "witnessScript does not match the scriptPubKey or redeemScript";
874+ data.import_scripts.emplace(script); // Special rule for IsMine: native P2WSH requires the full script imported
1105- throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
1106+ // Warn about inconsistencies between watchonly and provided keys/scripts
1107+ UniValue warnings(UniValue::VARR);
1108+ bool have_solving_data = import_data.redeemscript || import_data.witnessscript || import_data.pubkeys.size() || import_data.privkeys.size();
1109+ if (watchOnly && have_solving_data) {
1110+ warnings.push_back("redeemscript, witnessscript, keys, and pubkeys are ignored when watchonly is set.");
1093
1094- // Import the address
1095- if (::IsMine(*pwallet, scriptpubkey_script) == ISMINE_SPENDABLE) {
1096- throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script");
1097+ // Watchonly and private keys
1098+ if (watchOnly && import_data.privkeys.size()) {
1158- pwallet->MarkDirty();
1159-
1160- if (pwallet->HaveKey(vchAddress)) {
1161- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Already have this key");
1162+ if (!error.empty()) {
1163+ warnings.push_back("Ignoring information in keys, pubkeys, redeemscript, and witnessscript:: " + error);
s/::/:/
getaddressinfo["ischange"] tests yet, and would be a good time to add those.
utACK 33855fec46dbc3a7cad76875ca2a7660fcc19e92 aside from the ischange test, I can’t seem to convince myself we even honor the flag, even in master/0.17…
811+ std::unique_ptr<CScript> witnessscript; //! Provided witnessScript; will be moved to `import_scripts` if relevant.
812+ std::map<CKeyID, CPubKey> pubkeys;
813+ std::map<CKeyID, CKey> privkeys;
814+
815+ // Output data
816+ std::set<CScript> import_scripts;
std::set, could std::vector be used for these in order to preserve the order in which things will be imported? This is useful for #14075 where we want to have the order in which things are added to the wallet be the order that was specified in the import.
I think it’s always desired, particularly when someone imports private keys, as those would get added to the keypool in order of their occurrence in the wallet IIUC.
But if it’s an existing “feature” then of course it’s fine to change in it in the other PR.
@achow101 wrote in inline comment:
Instead of a
std::set, couldstd::vectorbe used for these in order to preserve the order in which things will be imported? This is useful for #14075 where we want to have the order in which things are added to the wallet be the order that was specified in the import.
It’s also better for recoverability if the wallet strives to use derivation paths in ascending order. Otherwise the user needs to remember what range they imported before.
It would be also keep us a bit closer to honouring a BIP44-style gap limit (if the user generates more than 20 addresses without receiving coins on any of them, we’d still break that gap, but at least that’s not the common case).
I realized I misread what “watchonly” was supposed to mean; it means it’s fine if the resulting address is not spendable (I was under the assumption it meant being fine if it’s not solvable).
I made some significant logic changes as a result to reflect the warning messages for that. All other comments are addressed, apart from the ordering of keys, and adding tests (which I will do soon).
1107- std::vector<CTxDestination> destinations = GetAllDestinationsForKey(pubkey);
1108- if (std::find(destinations.begin(), destinations.end(), dest) == destinations.end()) {
1109- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Key does not match address destination");
1110+ // Verify whether the watchonly option corresponds to the availability of private keys.
1111+ bool have_all_privkeys = true;
1112+ for (const auto& used_key : import_data.used_keys) {
std::any_of? :-)
1115+ break;
1116 }
1117+ }
1118+ if (!have_all_privkeys && !watchOnly) {
1119+ warnings.push_back("Assuming watchonly as not all private keys are provided.");
1120+ watchOnly = true;
1221+ CPubKey temp;
1222+ if (!pwallet->GetPubKey(id, temp) && !pwallet->AddWatchOnly(GetScriptForRawPubKey(pubkey), timestamp)) {
1223+ throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
1224+ }
1225+ }
1226+ if (!have_solving_data || !::IsMine(*pwallet, script)) { // Always call AddWatchOnly for watchOnly, so that watch timestamp gets updated
watchOnly here
false when the solvability data is bad).
1111
1112- // This is necessary to force the wallet to import the pubKey
1113- CScript scriptRawPubKey = GetScriptForRawPubKey(pubkey);
1114+ // Verify whether the watchonly option corresponds to the availability of private keys.
1115+ if (!watchOnly && std::any_of(import_data.used_keys.begin(), import_data.used_keys.end(), [&](const CKeyID& used_key){ return import_data.privkeys.count(used_key) == 0; })) {
1116+ warnings.push_back("Assuming watchonly as not all private keys are provided.");
re-utACK https://github.com/bitcoin/bitcoin/pull/14565/commits/7de84ca19a9719332879fd42569161b55e862391
Changes are some trivial variable renames/comment changes, replacement of the default case in the switch, changing some exceptions to warnings, and reworking the code to use !have_solvable_data instead of watchOnly during the import with a few related logic changes.
823+ TOP, //! Top-level scriptPubKey
824+ P2SH, //! P2SH redeemScript
825+ WITNESS_V0, //! P2WSH witnessScript
826+};
827+
828+// Analyse the provided script, process the import data accordinging, and recurse into subscripts if necessary.
scriptPubKey, determining which keys and which redeem scripts from the ImportData struct are needed to spend it, and mark them as used.”
870+ CScriptID id;
871+ CRIPEMD160().Write(fullid.begin(), fullid.size()).Finalize(id.begin());
872+ auto subscript = std::move(import_data.witnessscript);
873+ if (!subscript) return "missing witnessscript";
874+ if (CScriptID(*subscript) != id) return "witnessScript does not match the scriptPubKey or redeemScript";
875+ import_data.import_scripts.emplace(script); // Special rule for IsMine: native P2WSH requires the TOP script imported
script.cpp comment here, so future readers of this code understand why this is required?
852+ CScriptID id = CScriptID(uint160(solverdata[0]));
853+ auto subscript = std::move(import_data.redeemscript);
854+ if (!subscript) return "missing redeemscript";
855+ if (CScriptID(*subscript) != id) return "redeemScript does not match the scriptPubKey";
856+ import_data.import_scripts.emplace(*subscript);
857+ RecurseImportData(*subscript, import_data, ScriptContext::P2SH);
Doesn’t this need to return the value of RecurseImportData to avoid losing the error string?
Also, I think having a mix of breaks and returns here makes this function harder to follow. I think it’d be better if all the breaks were replaced by explicit returns, or if all returns were replaced with breaks (by assigning a std::string ret variable and returning it at the end).
803@@ -804,6 +804,95 @@ UniValue dumpwallet(const JSONRPCRequest& request)
804 return reply;
805 }
806
807+struct ImportData
808+{
809+ // Input data
810+ std::unique_ptr<CScript> redeemscript; //! Provided redeemScript; will be moved to `import_scripts` if relevant.
811+ std::unique_ptr<CScript> witnessscript; //! Provided witnessScript; will be moved to `import_scripts` if relevant.
812+ std::map<CKeyID, CPubKey> pubkeys;
pubkeys and privkeys struct members instead of freestanding variables in ProcessImport? It seems odd because the RecurseImportData function never touches them.
1209 pwallet->UpdateTimeFirstKey(timestamp);
1210 }
1211+ for (const auto& entry : import_data.pubkeys) {
1212+ const CPubKey& pubkey = entry.second;
1213+ const CKeyID& id = entry.first;
1214+ pwallet->mapKeyMetadata[id].nCreateTime = timestamp;
This seems like a change in behavior. It looks mapKeyMetadata was only set for previously for private keys, not public keys.
Can you drop this line or add a comment about what effect it has? It seems like whatever effect it has might be temporary because AddWatchOnly() won’t write this value to the database.
1227+ pwallet->SetAddressBook(dest, label, "receive");
1228+ }
1229
1230 UniValue result = UniValue(UniValue::VOBJ);
1231 result.pushKV("success", UniValue(true));
1232+ if (warnings.size()) result.pushKV("warnings", warnings);
result and warnings declarations above the try block, and moving result.pushKV("warnings") and return result below.
utACK 7de84ca19a9719332879fd42569161b55e862391. This is much better organized than the current code, though I agree with Sjors and think it would be nice to break up the process import function more: #14565 (review)
re: #14565 (comment):
I’ve also discovered when writing this that importmulti does not actually require that watchonly is set when no solvability is desired. I’ve kept the existing behavior for now, as it seems pretty invasive to people who may be relying on that, though I’ve added a TODO.
I don’t see the TODO. Would the TODO just be to turn the “Assuming watchonly as not all private keys are provided.” warning into an error?
re: #14565 (comment):
I realized I misread what “watchonly” was supposed to mean; it means it’s fine if the resulting address is not spendable (I was under the assumption it meant being fine if it’s not solvable).
Was having trouble understanding this. I think it is saying an earlier version of this PR allowed importing non-solvable scriptPubKeys as watch only, but now even watch-only scripts must be solvable. But is this actually a good requirement to have? Or is it just this way to avoid changing behavior?
re: #14565 (comment):
I’ve also discovered when writing this that importmulti does not actually require that watchonly is set when no solvability is desired. I’ve kept the existing behavior for now, as it seems pretty invasive to people who may be relying on that, though I’ve added a TODO. I don’t see the TODO. Would the TODO just be to turn the “Assuming watchonly as not all private keys are provided.” warning into an error?
It refers to an earlier misunderstanding I had, which I first tried to fix the even-more-wrong way. I’ve updated the comment now.
re: #14565 (comment):
I realized I misread what “watchonly” was supposed to mean; it means it’s fine if the resulting address is not spendable (I was under the assumption it meant being fine if it’s not solvable). Was having trouble understanding this. I think it is saying an earlier version of this PR allowed importing non-solvable scriptPubKeys as watch only, but now even watch-only scripts must be solvable. But is this actually a good requirement to have? Or is it just this way to avoid changing behavior?
Sorry for the confusion. Your comment made me read through history and make a few more changes. Here is my view:
Overall, as errors/warnings go, I think that the result of the above is that a number of previously permitted (but unnecessary) ways of calling this RPC now get detected. As it has a reasonable chance of breaking usage, I’m turning as many of those as possible into warnings into failures. All previously supported things should still work (but some more).
1143+ if (import_data.redeemscript) warnings.push_back("Ignoring redeemscript as this is not a P2SH script.");
1144+ if (import_data.witnessscript) warnings.push_back("Ignoring witnessscript as this is not a (P2SH-)P2WSH script.");
1145+ for (const auto& privkey : privkeys) {
1146+ if (import_data.used_keys.count(privkey.first) == 0) {
1147+ warnings.push_back("Ignoring irrelevant private key.");
1148+ break;
The warning says that the irrelevant private key will be ignored, but it looks like the private key will still be imported anyways.
Also, why exit this loop when an irrelevant private key is encountered?
1148+ break;
1149+ }
1150+ }
1151+ for (const auto& pubkey : pubkeys) {
1152+ if (import_data.require_keys.count(pubkey.first) == 0) {
1153+ warnings.push_back("Ignoring public key as it doesn't appear inside P2PKH or P2WPKH.");
971- // P2SH
972- if (!strRedeemScript.empty() && script.IsPayToScriptHash()) {
973- // Check the redeemScript is valid
974+ // Parse all arguments
975+ ImportData import_data;
976+ std::map<CKeyID, CPubKey> pubkeys;
pubkey_map to distinguish it from the pubKey variable. Or could rename the other variable.
privkey_map for consistency.
206@@ -207,25 +207,6 @@ def run_test (self):
207 assert_equal(result[0]['error']['code'], -4)
208 assert_equal(result[0]['error']['message'], 'The wallet already contains the private key for this address or script')
209
210- # Address + Private key + watchonly
211- self.log.info("Should not import an address with private key and with watchonly")
826+ std::unique_ptr<CScript> witnessscript; //! Provided witnessScript; will be moved to `import_scripts` if relevant.
827+
828+ // Output data
829+ std::set<CScript> import_scripts;
830+ std::set<CKeyID> used_keys; // Import these private keys if available
831+ std::set<CKeyID> require_keys; // These public keys are required for solvability
require_keys is a subset of used_keys. Could also choose a more normalized representation like std::map<CKeyID, bool /* required */> used_keys.
utACK 8a079e88f7a00cfbef0bb8b1d2016e5157343091, but I would make two suggestions:
Changes in this PR since my last review:
nCreateTime for watch only keys in the wrong map (mapKeyMetadata instead of m_script_metadata).ImportData struct pubkeys and privkeys members, replacing with local variables.RecurseImportData function comment and replacing breaks with returns.re: #14565 (comment)
I discovered that the intent was permitting non-spendability rather than non-solvability (which makes a lot more sense; it required fewer code and test changes, and importing something non-solvable isn’t prone to error, while something non-spendable is - you may randomly miss a private key)
I still don’t understand this. There are three cases, right?
It seems like all three cases are permitted in this PR, except there will be warnings if unused keys are provided, and errors if solving data is provided but some keys are missing. I guess I don’t know if this is the final state or you want to make more changes in the future.
I still don’t understand this. There are three cases, right?
- Solvable and spendable
- Solvable but not spendable
- Not solvable, not spendable
It seems like all three cases are permitted in this PR, except there will be warnings if unused keys are provided, and errors if solving data is provided but some keys are missing. I guess I don’t know if this is the final state or you want to make more changes in the future.
Right, that’s correct. The way to use them is:
The reason for the “watchonly” is distinguishing between solvable/spendable and solvable/unspendable: even if the non-spendable case you may want to import some of the related private keys. If you accidentally miss one of the keys however, the result will not be spendable. To make sure this is intentional, a warning will be given, which can be suppressed by saying “watchonly”.
If you accidentally miss one of the keys however, the result will not be spendable. To make sure this is intentional, a warning will be given, which can be suppressed by saying “watchonly”.
Now that’s a sentence worthy of going into the release notes (or help).
This PR absolutely needs more tests, but I won’t have time for that the next few days at least. If someone is interested in helping out, I think it would be useful to rewrite the existing tests to use the following pattern:K
importmulti query is constructedgetaddressinfo on the script/address itself (if it exists), but also calls it for all subscripts, P2SH/P2WSH wrapped versions, P2PK and P2PKH and P2WPKH versions of the involved keys, and tests that only the desired things are spendable/solvable/watched.1046- if (!allow_p2wpkh && dest.type() == typeid(WitnessV0KeyHash)) {
1047- throw JSONRPCError(RPC_INVALID_PARAMETER, "P2WPKH cannot be embedded in P2WSH");
1048+ for (size_t i = 0; i < pubKeys.size(); ++i) {
1049+ const auto& str = pubKeys[i].get_str();
1050+ if (!IsHex(str)) {
1051+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey must be a hex string");
Can you add the pubkey to the error message? (multiple pubkeys can be given in a request, and this error doesn’t indicate which one is invalid).
Same comment for other pubkey and privkey errors below.
1080- }
1081- pubkey = pubkey_temp;
1082+ CPubKey pubkey = key.GetPubKey();
1083+ CKeyID id = pubkey.GetID();
1084+ if (pubkeys.count(id)) {
1085+ warnings.push_back("Both public and private key provided.");
882+ CScriptID id;
883+ CRIPEMD160().Write(fullid.begin(), fullid.size()).Finalize(id.begin());
884+ auto subscript = std::move(import_data.witnessscript);
885+ if (!subscript) return "missing witnessscript";
886+ if (CScriptID(*subscript) != id) return "witnessScript does not match the scriptPubKey or redeemScript";
887+ import_data.import_scripts.emplace(script); // Special rule for IsMine: native P2WSH requires the TOP script imported
emplace will be called on script even if this is P2SH-P2WSH, but it’ll be a no-op since emplace(*subscript) was already called in the outer run through RecurseImportData(). I think it may be clearer to either make that explicit in this comment or only call this emplace if script_ctx == ScriptContext::TOP.
900+ case TX_WITNESS_UNKNOWN:
901+ return "unrecognized script";
902+ case TX_NULL_DATA:
903+ return "unspendable script";
904+ }
905+ return "";
1127+ }
1128
1129- pwallet->MarkDirty();
1130+ // Verify whether the watchonly option corresponds to the availability of private keys.
1131+ if (!watchOnly && std::any_of(import_data.used_keys.begin(), import_data.used_keys.end(), [&](const CKeyID& used_key){ return privkeys.count(used_key) == 0; })) {
1132+ warnings.push_back("Assuming watchonly as not all private keys are provided.");
I don’t understand why this is just a warning, rather than causing the import to fail. The client has asked to import a script/address as not watchonly but hasn’t provided the keys - shouldn’t we fail (and make the client import with watchonly set and no privkeys provided if they want to import as watchonly)?
EDIT: I see your earlier comment:
Perhaps we want to introduce a new mode where all these warnings become errors, though -deprecatedrpc, but let’s discuss that in a separate RPC.
My preference would be for all of these things to cause the import to fail by default, but we can save that for a future PR.
1216+ for (const auto& entry : privkeys) {
1217+ const CKey& key = entry.second;
1218+ CPubKey pubkey = key.GetPubKey();
1219+ const CKeyID& id = entry.first;
1220+ assert(key.VerifyPubKey(pubkey));
1221+ pwallet->mapKeyMetadata[id].nCreateTime = timestamp;
Is there a good reason that the key’s createtime metadata is updated before adding the key to the wallet?
It would be good to add a comment, but the map needs to be updated before calling AddKeyPubKey, because AddKeyPubKey uses it here: https://github.com/bitcoin/bitcoin/blob/86ff0413bb8f8173d3b3a1987875ff40b1094926/src/wallet/wallet.cpp#L291
A good cleanup would be to drop this line and instead pass the time as an argument to AddKeyPubKey, the same way it is passed as an argument to AddWatchOnly below.
Isn’t it safer to only update mapKeyMetadata[id].nCreateTime if timestamp is older than the current value? Otherwise what if:
"timestamp": "now"bitcoind -zapwallettxes and then rescan
In that case, would rescan pick it up anyway regardless of nCreateTime because it starts at genesis block?
Let’s leave this for later.
Agree. When this does get changed, we should also update CWallet::AddKeyPubKey() to also call UpdateTimeFirstKey() (in the same way that CWallet::AddWatchOnly() does).
1218+ CPubKey pubkey = key.GetPubKey();
1219+ const CKeyID& id = entry.first;
1220+ assert(key.VerifyPubKey(pubkey));
1221+ pwallet->mapKeyMetadata[id].nCreateTime = timestamp;
1222+ if (!pwallet->HaveKey(id) && !pwallet->AddKeyPubKey(key, pubkey)) {
1223 throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet");
I think the error messages in all these throws could be improved. Here, for example, the error text could be:
0throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Script import failed due to problem importing private key %s.", EncodeSecret(key));
As it is, it’s not clear that failing to import this key also causes the script to not be imported. Same comment for failures to import pubkey below.
It’s a bit of a shame that this operation isn’t atomic and failing to import one key or script will result in the prior keys being imported but the following keys not being imported. Might be worth adding a comment somewhere to highlight that?
1228+ for (const auto& entry : pubkeys) {
1229+ const CPubKey& pubkey = entry.second;
1230+ const CKeyID& id = entry.first;
1231+ CPubKey temp;
1232+ if (!pwallet->GetPubKey(id, temp) && !pwallet->AddWatchOnly(GetScriptForRawPubKey(pubkey), timestamp)) {
1233+ throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
1253- return result;
1254 } catch (...) {
1255- UniValue result = UniValue(UniValue::VOBJ);
1256 result.pushKV("success", UniValue(false));
1257+
1258 result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, "Missing required fields"));
This is a nice improvement. I’ve reviewed and left a bunch of nits, mostly around making error messages clearer.
I think this comment: #14565 (comment) should go in release notes or docs.
This PR absolutely needs more tests, but I won’t have time for that the next few days at least. If someone is interested in helping out, I think it would be useful to rewrite the existing tests to use the following pattern
I’ve started refactoring the wallet_importmulti.py into this style. It’s a WIP here: https://github.com/jnewbery/bitcoin/tree/importmulti_tests . I hope to continue working on it this week.
949 }
950 }
951
952- // Watchonly and private keys
953- if (watchOnly && keys.size()) {
954- throw JSONRPCError(RPC_INVALID_PARAMETER, "Watch-only addresses should not include private keys");
Yes, how else would you import a private key for something you don’t have all keys for? The old behavior was bogus.
I think I explained it elsewhere in this PR, but the intended behavior of watchonly is “import this, even though I don’t have all the relevant keys”. It wasn’t supposed to prevent importing any keys at all.
watchonly needs to be updated. It’s currently Stating whether matching outputs should be considered watched even when they're not spendable, only allowed if keys are empty.
re: #14565 (review)
I think the RPC help text for watchonly needs to be updated.
I suggested one possible update in #14565#pullrequestreview-179073149:
I think the documentation would be clearer if it just said that “watchonly” has no actual effect on the import, and only disables a warning if there are missing private keys.
818@@ -819,9 +819,97 @@ UniValue dumpwallet(const JSONRPCRequest& request)
819 return reply;
820 }
821
822+struct ImportData
823+{
824+ // Input data
825+ std::unique_ptr<CScript> redeemscript; //! Provided redeemScript; will be moved to `import_scripts` if relevant.
826+ std::unique_ptr<CScript> witnessscript; //! Provided witnessScript; will be moved to `import_scripts` if relevant.
//!<
http://www.doxygen.nl/manual/docblocks.html#memberdoc
I’ve started refactoring the wallet_importmulti.py into this style. It’s a WIP here: https://github.com/jnewbery/bitcoin/tree/importmulti_tests . I hope to continue working on it this week.
This work is (mostly) complete. Intermediate commits could probably do with some tidy-up, which I intend to do later today.
PR here: #14886
I’ve started writing tests for this PR: https://github.com/jnewbery/bitcoin/tree/pr14565.tests
It’s certainly not complete, but it makes a start by:
importmulti for the right warnings return value.getaddressinfo for the different address variants when privkeys are imported.To continue, we should:
I’m unlikely to be able to spend time on the tests for the next few days. Anyone is free to take the branch and continue building on it.
I’ve addressed many of the comments above, rebased on top of the new test changes from #14886, and made a few behavior changes as well:
876+ case TX_SCRIPTHASH: {
877+ if (script_ctx == ScriptContext::P2SH) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Trying to nest P2SH inside another P2SH");
878+ if (script_ctx == ScriptContext::WITNESS_V0) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Trying to nest P2SH inside a P2WSH");
879+ assert(script_ctx == ScriptContext::TOP);
880+ CScriptID id = CScriptID(uint160(solverdata[0]));
881+ auto subscript = std::move(import_data.redeemscript);
Suggested comment (above, and in TX_WITNESS_V0_SCRIPTHASH):
0// Remove redeemscript from import_data to check for superfluous script later.
868+ import_data.used_keys.emplace(pubkey.GetID(), false);
869+ return "";
870+ }
871+ case TX_PUBKEYHASH: {
872+ CKeyID id = CKeyID(uint160(solverdata[0]));
873+ import_data.used_keys[id] = true;
emplace() here like with TX_PUBKEY?
emplace() doesn’t overwrite the entry if it already exists (which could possible leave it with required=false).
1239+ const CKey& key = entry.second;
1240+ CPubKey pubkey = key.GetPubKey();
1241+ const CKeyID& id = entry.first;
1242+ assert(key.VerifyPubKey(pubkey));
1243+ pwallet->mapKeyMetadata[id].nCreateTime = timestamp;
1244+ if (!pwallet->HaveKey(id) && !pwallet->AddKeyPubKey(key, pubkey)) {
Suggested comment (above), because HaveKey is an ambiguous function name:
0// If the private key is not present in the wallet, insert it or add it to an existing public key.
utACK d6f2b34, but I suggest waiting for #14491 (descriptor support) rebase, as an extra check that we didn’t miss anything here.
deprecatedrpc thing to turn all warnings into errors
Or just add that as an option? It seems like a useful feature, especially since we can’t delete keys and don’t have a dry-run option. Different PR is fine.
0@@ -0,0 +1,5 @@
1+Low-level RPC changes
2+---------------------
3+
4+The `importmulti` RPC will now contain a new `warnings` field with strings
nit, could say:
854+ WITNESS_V0, //! P2WSH witnessScript
855+};
856+
857+// Analyse the provided scriptPubKey, determining which keys and which redeem scripts from the ImportData struct are needed to spend it, and mark them as used.
858+// Returns an error string, or the empty string for success.
859+static std::string RecurseImportData(const CScript& script, ImportData& import_data, const ScriptContext script_ctx)
IMO could do better than empty string as success — despite the fact that it works.
Alternatives:
std::pair<bool, std::string>bool and add argument std::string& error
PR description is misleading, especially:
It makes sure no changes to the wallet are made when an error in the input exists.
should state that it’s a per request check.
This introduces various changes to the importmulti logic:
* Instead of processing input and importing things at the same time, first
process all input data and verify it, so no changes are made in case of
an error.
* Verify that no superfluous information is provided (no keys or scripts
that don't contribute to solvability in particular).
* Add way more sanity checks, by means of descending into all involved
scripts.
Or just add that as an option? It seems like a useful feature, especially since we can’t delete keys and don’t have a dry-run option. Different PR is fine.
I’d really like to avoid that. There is no reason for that to be an option (which we may need to maintain forever), as the intended behavior is that these things are just errors, always. The only reason why they aren’t is because we don’t want to break existing software immediately.
1248-
1249 pwallet->UpdateTimeFirstKey(timestamp);
1250 }
1251+ for (const auto& entry : pubkey_map) {
1252+ const CPubKey& pubkey = entry.second;
1253+ const CKeyID& id = entry.first;
mapKeyMetadata here to include the timestamp?
re: #14565 (review)
Do we also need to update mapKeyMetadata here to include the timestamp?
I requested not doing this here: #14565 (review). It shouldn’t be necessary because timestamp is passed to AddWatchOnly below. In the future the mapKeyMetadata update above could also be removed, see #14565 (review).
124 """Run importmulti and assert success"""
125 result = self.nodes[1].importmulti([req])
126+ observed_warnings = []
127+ if 'warnings' in result[0]:
128+ observed_warnings = result[0]['warnings']
129+ assert_equal("\n".join(sorted(warnings)), "\n".join(sorted(observed_warnings)))
are the "\n".join adding anything here?
Presumably "\n".join(list1) == "\n".join(list2) ⇔ list1 == list2?
473 "timestamp": "now",
474 "keys": [wrong_privkey]},
475- False,
476- error_code=-5,
477- error_message='Key does not match address destination')
478+ True,
617+ True,
618+ warnings=["Some private keys are missing, outputs will be considered watchonly. If this is intentional, specify the watchonly flag."])
619 self.test_address(address,
620- solvable=True)
621+ solvable=True,
622+ ismine=False)
1069+ std::map<CKeyID, CKey> privkey_map;
1070+ for (size_t i = 0; i < keys.size(); ++i) {
1071+ const auto& str = keys[i].get_str();
1072+ CKey key = DecodeSecret(str);
1073+ if (!key.IsValid()) {
1074+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key encoding");
nit: should identify which private key was invalid in error message.
EDIT: same for all error messages below. importmulti can contain multiple scripts, addresses and keys. Any error messages should indicate which address/script/key was at fault.
Yes, very good point. I think Promag’s suggestions are good.
In any case, this doesn’t need to be included in this PR. A future PR to improve logging and test all failure modes would be nice.
123+ def test_importmulti(self, req, success, error_code=None, error_message=None, warnings=[]):
124 """Run importmulti and assert success"""
125 result = self.nodes[1].importmulti([req])
126+ observed_warnings = []
127+ if 'warnings' in result[0]:
128+ observed_warnings = result[0]['warnings']
118@@ -119,9 +119,13 @@ def get_multisig(self):
119 CScript([OP_HASH160, witness_script, OP_EQUAL]).hex(), # p2sh-p2wsh
120 script_to_p2sh_p2wsh(script_code)) # p2sh-p2wsh addr
121
122- def test_importmulti(self, req, success, error_code=None, error_message=None):
123+ def test_importmulti(self, req, success, error_code=None, error_message=None, warnings=[]):
I suggest using warnings=None (see example below) instead to clarify that warnings is not meant to be remembered across calls.
Background:
0>>> def test(i, i_arr=[]):
1... i_arr.append(i)
2... return i_arr
3...
4>>> test(1)
5[1]
6>>> test(2)
7[1, 2]
8>>> test(3)
9[1, 2, 3]
Suggested alternative:
0>>> def test(i, i_arr=None):
1... if i_arr is None:
2... i_arr=[]
3... i_arr.append(i)
4... return i_arr
5...
6>>> test(1)
7[1]
8>>> test(2)
9[2]
10>>> test(3)
11[3]
418+ solvable=True,
419+ timestamp=timestamp)
420
421 # Address + Public key + !Internal + Wrong pubkey
422- self.log.info("Should not import an address with a wrong public key")
423+ self.log.info("Should not import an address with the wrong public key as non-solvable")
1185
1186- // Import the address
1187- if (::IsMine(*pwallet, scriptpubkey_script) == ISMINE_SPENDABLE) {
1188+ // Check whether we have any work to do
1189+ if (::IsMine(*pwallet, script) & ISMINE_SPENDABLE) {
1190 throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script");
(unchanged by this PR)
This error message is slightly wrong, since ISMINE_SPENDABLE could mean that the wallet already contains private keys. I think it would be better to say that the address or script is already owned by the wallet.
1138+ // Check that all required keys for solvability are provided.
1139+ if (error.empty()) {
1140+ for (const auto& require_key : import_data.used_keys) {
1141+ if (!require_key.second) continue; // Not a required key
1142+ if (pubkey_map.count(require_key.first) == 0 && privkey_map.count(require_key.first) == 0) {
1143+ error = "some required keys are missing";
921+ default:
922+ return "unrecognized script";
923+ }
924+}
925
926 static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
This is a fairly involved function, so it’d be friendly to have a function-level comment:
A bunch of comments, mostly around error messages. Please treat these all as nits. They shouldn’t hold up merge and could be addressed in a follow-up PR.
This is certainly an improvement.
I’ve expanded the release notes here: https://github.com/jnewbery/bitcoin/commit/0348531309f64e669884f31b4de64efe0888bdd7. Feel free to squash into your commit if you think it’s an improvement.
utACK eacff95de4751b500f1cef623e4024918dcb05bb
used_keys representation, and renaming / moving some local variables.
utACK https://github.com/bitcoin/bitcoin/pull/14565/commits/eacff95de4751b500f1cef623e4024918dcb05bb
Agree with John, the final nits can be addressed in a followup