Import pubkeys when importing p2sh with importmulti #14019

achow101 · 2018-08-21T21:29:54Z

This PR has importmulti import public keys if specified instead of ignoring them when the thing being imported is p2sh.

instagibbs · 2018-08-22T13:45:20Z

src/wallet/rpcdump.cpp

+                // TODO Is this necessary?
+                CScript scriptRawPubKey = GetScriptForRawPubKey(pubKey);
+
+                if (::IsMine(*pwallet, scriptRawPubKey) == ISMINE_SPENDABLE) {


Curious why we aren't calling HaveKey here and in similar locations?

Dunno. I just copied this from below

instagibbs · 2018-08-22T13:48:39Z

src/wallet/rpcdump.cpp

+                    throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script");
+                }
+
+                pwallet->MarkDirty();


why the MarkDirty here? Needs a comment imo.

Copied from below. It seems that we always do MarkDirty before importing stuff. This pattern appears throughout the other import stuff.

If #14303 is correct, these can be removed. Please review.

instagibbs · 2018-08-22T13:48:55Z

src/wallet/rpcdump.cpp

+                    throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script");
+                }
+
+                pwallet->MarkDirty();


why the MarkDirty here? Needs a comment imo.

instagibbs · 2018-08-22T13:53:36Z

src/wallet/rpcdump.cpp

+                }
+
+                // TODO Is this necessary?
+                CScript scriptRawPubKey = GetScriptForRawPubKey(pubKey);


Seems kind of expected until we stop abusing keys sadly.

DrahtBot · 2018-08-23T04:40:50Z

Reviewers, this pull request conflicts with the following ones:

Add SegWit support to importmulti #14454 (Add SegWit support to importmulti by MeshCollider)
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.

instagibbs · 2018-08-25T14:08:19Z

utACK 54ab69ce78a88f24c96ebcab09c63da27db2ee9d

I think removing the TODO is fine since we're doing it for everything until a future wallet refactor.

luke-jr · 2018-08-29T17:46:52Z

Why? If you're importing p2sh, the wallet shouldn't recognise lone keys...

achow101 · 2018-08-29T20:29:02Z

@luke-jr This PR is largely to work in combination with #14021, but I decided it would be better to split it into it's own PR. The reason for doing this is so that a PSBT can be made which includes the pubkey and the key derivation information (master key fingerprint and keypath) for a P2SH script (e.g. a multisig redeemScript). Without this, the wallet has no way of knowing the key and its metadata in order to put that into a PSBT. This is particularly useful for hardware wallet support.

sipa · 2018-08-29T20:34:11Z

@luke-jr It's an unfortunate side effect of the current design that importing keys/script sometimes results in outputs involving those becoming treated as IsMine.

The goal here isn't changing IsMine, but making sure the information is available for signing.

This issue will be independently solved by moving to descriptor based wallets, where nothing except the scripts defined by the imported descriptors will be treated as IsMine.

instagibbs · 2018-09-14T19:56:30Z

tACK 7bcf2cb

In use on active wallets.

jb55

utACK 7bcf2cb

Only nit I have is: since keys are getting imported in a loop, any error thrown on an individual key might be hard to diagnose. Happy to defer this to a future PR though.

promag · 2018-09-23T22:03:47Z

test/functional/wallet_importmulti.py

+                "timestamp": "now",
+            }]
+        )
+        assert result[0]['success']


nit, assert_equal(result[0]['success'], True).

promag · 2018-09-23T22:07:55Z

src/wallet/rpcdump.cpp

@@ -917,6 +917,53 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
                pwallet->SetAddressBook(dest, label, "receive");
            }

+            // Import public keys
+            for (size_t i = 0; i < pubKeys.size(); ++i) {
+                const std::string& pubkey = pubKeys[i].get_str();


Use HexToPubKey:

CPubKey pubKey = HexToPubKey(pubKeys[i].get_str())

promag · 2018-09-23T22:10:26Z

src/wallet/rpcdump.cpp

@@ -917,6 +917,53 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
                pwallet->SetAddressBook(dest, label, "receive");
            }

+            // Import public keys
+            for (size_t i = 0; i < pubKeys.size(); ++i) {


nit, use range-based for-loop?

This does not work as UniValue does not have an end() function apparently.

promag · 2018-09-23T22:16:05Z

test/functional/wallet_importmulti.py

+        ms = self.nodes[1].createmultisig(2, [pub1, pub2])
+        result = self.nodes[0].importmulti(
+            [{
+                'scriptPubKey' : { 'address' : ms['address']},


nit, space before }?

we should style lint python

etscrivner · 2018-09-28T03:40:01Z

src/wallet/rpcdump.cpp

+                    throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey is not a valid public key");
+                }
+
+                CTxDestination pubkey_dest = pubKey.GetID();


nit: pubkey_dest => pubKeyDest?

No. The style guide says to use snake case for variable names.

Any reason why the other local variables don't follow this naming convention? (For example, pubKey, pubKeyScript, and scriptRawPubKey)

The style convention has changed over time. New code should follow the new convention. Changing all of the existing code to follow that would break a ton of PRs so we don't do that.

Gotcha. So the variables I named are new code AFAICT - though I may be missing something. They appear to be new variables defined as part of the work done for this PR.

etscrivner · 2018-09-28T04:17:47Z

tACK 6412265

instagibbs · 2018-10-09T04:19:31Z

tACK 6412265

meshcollider · 2018-10-10T13:25:19Z

this PR is replaced by #14454, please review that instead :)

fanquake · 2018-10-14T11:29:28Z

@achow101 Is #14454 a full replacement for this PR? If so, can you close this.

achow101 · 2018-10-15T21:06:23Z

Superseded by #14454. Also, from discussion on IRC just now, it seems that this feature is no longer necessary following #14424

@achow101

c11875c Add segwit address tests for importmulti (MeshCollider) 201451b Make getaddressinfo return solvability (MeshCollider) 1753d21 Add release notes for importmulti segwit change (MeshCollider) 353c064 Fix typo in test_framework/blocktools (MeshCollider) f6ed748 Add SegWit support to importmulti with some ProcessImport cleanup (MeshCollider) Pull request description: Add support for segwit to importmulti, supports P2WSH, P2WPKH, P2SH-P2WPKH, P2SH-P2WSH. Adds a new `witnessscript` parameter which must be used for the witness scripts in the relevant situations. Also includes some tests for the various import types. ~Also makes the change in #14019 redundant, but cherry-picks the test from that PR to test the behavior (@achow101).~ Fixes #12253, also addresses the second point in #12703, and fixes #14407 Tree-SHA512: 775a755c524d1c387a99acddd772f677d2073876b72403dcfb92c59f9b405ae13ceedcf4dbd2ee1d7a8db91c494f67ca137161032ee3a2071282eeb411be090a

fanquake added Wallet RPC/REST/ZMQ labels Aug 21, 2018

achow101 mentioned this pull request Aug 22, 2018

Import key origin data through descriptors in importmulti #14021

Merged

instagibbs reviewed Aug 22, 2018

View reviewed changes

achow101 mentioned this pull request Aug 26, 2018

Import watch only pubkeys to the keypool if private keys are disabled #14075

Merged

achow101 force-pushed the import-multi-pubkeys branch from 54ab69c to 7bcf2cb Compare August 26, 2018 20:06

achow101 mentioned this pull request Sep 4, 2018

Hardware wallet support #14145

Closed

jb55 approved these changes Sep 22, 2018

View reviewed changes

promag reviewed Sep 23, 2018

View reviewed changes

Import pubkeys when importing p2sh with importmulti

6412265

achow101 force-pushed the import-multi-pubkeys branch from 7bcf2cb to 6412265 Compare September 27, 2018 03:53

etscrivner reviewed Sep 28, 2018

View reviewed changes

gwillen mentioned this pull request Oct 7, 2018

SegWit support for importmulti #12253

Closed

meshcollider mentioned this pull request Oct 10, 2018

Add SegWit support to importmulti #14454

Merged

achow101 mentioned this pull request Oct 12, 2018

Add Coldcard bitcoin-dot-org/Bitcoin.org#2612

Closed

achow101 closed this Oct 15, 2018

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

Import pubkeys when importing p2sh with importmulti #14019

Import pubkeys when importing p2sh with importmulti #14019

Uh oh!

Conversation

achow101 commented Aug 21, 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!

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

Uh oh!

Uh oh!

instagibbs commented Aug 25, 2018

Uh oh!

luke-jr commented Aug 29, 2018

Uh oh!

achow101 commented Aug 29, 2018

Uh oh!

sipa commented Aug 29, 2018

Uh oh!

instagibbs commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jb55 left a comment

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!

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!

etscrivner Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

etscrivner commented Sep 28, 2018

Uh oh!

instagibbs commented Oct 9, 2018

Uh oh!

meshcollider commented Oct 10, 2018

Uh oh!

fanquake commented Oct 14, 2018

Uh oh!

achow101 commented Oct 15, 2018

Uh oh!

Uh oh!

DrahtBot commented Aug 23, 2018 •

edited

Loading

instagibbs commented Sep 14, 2018 •

edited

Loading

etscrivner Sep 28, 2018 •

edited

Loading