rpc: Require solvability in importmulti if importing more than the scriptPubKey#14558
rpc: Require solvability in importmulti if importing more than the scriptPubKey#14558achow101 wants to merge 14 commits intobitcoin:masterfrom
Conversation
Introduce two functions, ImportScriptsToKeystore and AddScriptsToWatchOnly. These functions move the actual script and public key importing to the end of ProcessImport.
AllUsedKeyStore is used to track whether all of the scripts and public keys in the keystore have been used in signing. To avoid issues with const-ness, the usage maps are in a separate struct which is pointed to by a pointer in AllUsedKeyStore. This allows GetPubKey and GetCScript to properly override the CBasicKeyStore functions.
Check that if more than just a scriptPubKey was imported, that there was enough data to be solvable. Check that if more than just a scriptPubKey was imported, that the minimal data needed for solvability was imported.
|
Concept ACK |
| return reply; | ||
| } | ||
|
|
||
| struct UsageMaps { |
There was a problem hiding this comment.
Any reason to not move these to AllUsedKeyStore?
There was a problem hiding this comment.
Yes. The functions GetKey, GetPubKey, and GetCScript are all const functions and need to be in order to override the CBasicKeyStore versions of them. However, those functions need to modify the usage maps. If they were in AllUsedKeyStore, those functions would not be const. By having them in a separate struct that has a pointer in AllUsedKeyStore, the functions can remain const but still allow us to update the usage info.
| class AllUsedKeyStore : public CBasicKeyStore | ||
| { | ||
| private: | ||
| struct UsageMaps* usage = new UsageMaps(); |
|
|
||
| // Process. // | ||
| CScript scriptpubkey_script = script; | ||
| CTxDestination scriptpubkey_dest = dest; |
There was a problem hiding this comment.
scriptpubkey_dest is unused?
| struct UsageMaps* usage = new UsageMaps(); | ||
| public: | ||
| bool AllUsed() const; | ||
| bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const override; |
There was a problem hiding this comment.
Make the parameters names match between declaration and definition (CPubKey& vchPubKeyOut vs CPubKey &pubkey_out) :-)
| bool AllUsed() const; | ||
| bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const override; | ||
| bool AddCScript(const CScript& redeemScript) override; | ||
| bool GetCScript(const CScriptID &hash, CScript& redeemScriptOut) const override; |
There was a problem hiding this comment.
Same here: CScript& redeemScriptOut vs CScript& script_out :-)
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. |
| { | ||
| LOCK(cs_KeyStore); | ||
| if (CBasicKeyStore::GetPubKey(address, pubkey_out)) { | ||
| usage->map_watch_key_use[address] = true; |
There was a problem hiding this comment.
You could invert the logic and keep track of the unused
- when adding a key, add to the unused set
- when reading a key, remove from the unused set
Then AllUsed could look like (s/usage/unused):
return
unused->watch_keys.empty() &&
unused->scripts.empty() &&
unused->keys.empty();I could be missing some detail.
|
Closing in favor of #14565 |
eacff95 Add release notes (Pieter Wuille) bdacbda Overhaul importmulti logic (Pieter Wuille) Pull request description: This is an alternative to #14558 (it will warn when fields are being ignored). In addition: * It makes sure no changes to the wallet are made when an error in the input exists. * It validates all arguments, and will fail if anything fails to parse. * Adds a whole bunch of sanity checks Tree-SHA512: fdee0b6aca8c643663f0bc295a7c1d69c1960951493b06abf32c58977f3e565f75918dbd0402dde36e508dc746c9310a968a0ebbacccc385a57ac2a68b49c1d0
8 participants
Currently, in
importmulti, it is possible to import not enough information to make a scriptPubKey solvable. It is also possible to import more information than necessary to make a scriptPubKey solvable. This PR changes this to require that an import must be either just the scriptPubKey or include everything necessary to make it solvable without any extra data.This is built on top of #14454.