rpc: Require solvability in importmulti if importing more than the scriptPubKey #14558

achow101 · 2018-10-24T04:45:23Z

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.

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.

sipa · 2018-10-24T07:11:26Z

Concept ACK

promag · 2018-10-24T09:22:07Z

src/wallet/rpcdump.cpp

    return reply;
 }

+struct UsageMaps {


Any reason to not move these to AllUsedKeyStore?

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.

Got it, thanks.

promag · 2018-10-24T13:33:59Z

src/wallet/rpcdump.cpp

+class AllUsedKeyStore : public CBasicKeyStore
+{
+private:
+    struct UsageMaps* usage = new UsageMaps();


This is leaking.

Nit, could remove struct?

practicalswift · 2018-10-24T14:54:03Z

src/wallet/rpcdump.cpp

-
-        // Process. //
+        CScript scriptpubkey_script = script;
+        CTxDestination scriptpubkey_dest = dest;


scriptpubkey_dest is unused?

practicalswift · 2018-10-24T14:58:22Z

src/wallet/rpcdump.cpp

+    struct UsageMaps* usage = new UsageMaps();
+public:
+    bool AllUsed() const;
+    bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const override;


Make the parameters names match between declaration and definition (CPubKey& vchPubKeyOut vs CPubKey &pubkey_out) :-)

practicalswift · 2018-10-24T15:00:55Z

src/wallet/rpcdump.cpp

+    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;


Same here: CScript& redeemScriptOut vs CScript& script_out :-)

DrahtBot · 2018-10-24T16:54:04Z

Reviewers, this pull request conflicts with the following ones:

Overhaul importmulti logic #14565 (Overhaul importmulti logic by sipa)
Add SegWit support to importmulti #14454 (Add SegWit support to importmulti by MeshCollider)
rpc: Early call once CWallet::MarkDirty in import calls #14303 (rpc: Early call once CWallet::MarkDirty in import calls by promag)
Import watch only pubkeys to the keypool if private keys are disabled #14075 (Import watch only pubkeys to the keypool if private keys are disabled by achow101)
Import key origin data through descriptors in importmulti #14021 (Import key origin data through importmulti by achow101)

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.

promag · 2018-10-24T21:57:29Z

src/wallet/rpcdump.cpp

+{
+    LOCK(cs_KeyStore);
+    if (CBasicKeyStore::GetPubKey(address, pubkey_out)) {
+        usage->map_watch_key_use[address] = true;


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.

achow101 · 2018-10-30T04:24:15Z

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

meshcollider and others added 14 commits October 20, 2018 23:45

Add SegWit support to importmulti with some ProcessImport cleanup

226671a

Add segwit address tests for importmulti

36719c3

Fix typo in test_framework/blocktools

f2b101a

Add release notes for importmulti segwit change

d73f658

Refactor ProcessImport to allow P2SH-P2WPKH again

cebab44

Make getaddressinfo return solvability

f2a4e1e

Improve segwit importmulti tests

31d8b4b

fixups for importmulti segwit support PR

417a64e

Rename issolvable to solvable

7c0268a

Do not add inner witness script as watch only

e4a66bb

refactor: introduce script importing helper functions in ProcessImport

6e71d4f

Introduce two functions, ImportScriptsToKeystore and AddScriptsToWatchOnly. These functions move the actual script and public key importing to the end of ProcessImport.

rpc: importmulti: Check for solvability and minimal import

9879272

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.

Test solvability requirement

7b93302

fanquake added the RPC/REST/ZMQ label Oct 24, 2018

promag reviewed Oct 24, 2018

View reviewed changes

practicalswift reviewed Oct 24, 2018

View reviewed changes

promag reviewed Oct 24, 2018

View reviewed changes

sipa mentioned this pull request Oct 25, 2018

Overhaul importmulti logic #14565

Merged

achow101 closed this Oct 30, 2018

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

rpc: Require solvability in importmulti if importing more than the scriptPubKey #14558

rpc: Require solvability in importmulti if importing more than the scriptPubKey #14558

Uh oh!

Conversation

achow101 commented Oct 24, 2018

Uh oh!

sipa commented Oct 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Oct 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Oct 24, 2018 •

edited

Loading