Add SegWit support to importmulti

meshcollider commented at 1:58 am on October 10, 2018: contributor

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

meshcollider added the label RPC/REST/ZMQ on Oct 10, 2018

meshcollider added the label Wallet on Oct 10, 2018

DrahtBot commented at 4:59 am on October 10, 2018: member

#14303 (rpc: Early call once CWallet::MarkDirty in import calls by promag)
#14075 (Import watch only pubkeys to the keypool if private keys are disabled 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.

DocOBITCOIN approved

in src/wallet/rpcdump.cpp:1003 in 59f8e54713 outdated

1096+            if (!pubKey.IsFullyValid()) {
1097+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey is not a valid public key");
1098+            }
1099 
1100-                CTxDestination pubkey_dest = pubKey.GetID();
1101+            CTxDestination pubkey_dest = pubKey.GetID();

practicalswift commented at 1:48 pm on October 13, 2018:

No longer needed?

in src/wallet/rpcdump.cpp:901 in 59f8e54713 outdated

949-        } else {
950-            // Import public keys.
951-            if (pubKeys.size() && keys.size() == 0) {
952-                const std::string& strPubKey = pubKeys[0].get_str();
953+                // Generate the scripts
954+                std::vector<unsigned char> vData(ParseHex(strWitnessScript));

practicalswift commented at 1:50 pm on October 13, 2018:

Use another variable name to avoid shadowing existing local variable vData :-)

meshcollider commented at 0:04 am on October 14, 2018: contributor

Thanks for reviewing @practicalswift, both comments addressed :)

in src/wallet/rpcdump.cpp:862 in 3276b7e22a outdated

866-
867-        // Invalid P2SH redeemScript
868-        if (isP2SH && !IsHex(strRedeemScript)) {
869-            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid redeem script");
870+        // Force users to provide the witness script in its field rather than redeemscript
871+        if (!strRedeemScript.empty() && script.IsPayToWitnessScriptHash()) {

achow101 commented at 2:36 am on October 14, 2018:

Instead of checking that strRedeemScript is not empty, this should check that strWitnessScript is empty. Or it could check both: !strRedeemScript.empty() && strWitnessScript.empty().

meshcollider commented at 7:39 am on October 14, 2018:

It should be possible to provide a P2WSH scriptPubKey without providing the witness script for it, e.g. watch only, so I don’t think that’s right

achow101 commented at 0:46 am on October 15, 2018:

I think checking both covers that case.

meshcollider commented at 0:55 am on October 15, 2018:

Checking both would mean that if witnessScript and redeemScript were both present, it would allow it, which makes no sense if its not a P2WSH address

Sjors commented at 6:33 am on October 17, 2018:

P2WSH scriptPubKey without providing the witness script for it, e.g. watch only

Do we already support that? And test it? If not, maybe it’s easier to just treat it as P2SH until we know the witnessScript.

sipa commented at 9:40 pm on October 17, 2018:

@Sjors Yes, of course, just importaddress it. It probably works fine without this PR even.

Sjors commented at 8:04 am on October 18, 2018:

@sipa so we currently support importing a 3... address as watch-only, but the wallet doesn’t know or care if it’s SegWit or not. The new code here is for a scenario where you do know it’s SegWit script.IsPayToWitnessScriptHash(), which means you know the strRedeemScript, but you don’t know the strWitnessScript. That seems a strange situation. Why would anyone give you the legacy redeem script but not the Segwit witness script?

sipa commented at 8:14 am on October 18, 2018:

@Sjors I have no idea what you’re talking about. This is about the case where you’re importing a bech32 P2WSH address (no P2SH), without revealing the script inside - which is a totally reasonable thing if you don’t care about solvability.

Sjors commented at 7:19 am on October 22, 2018:

@sipa I see, I may have gotten confused because Github is showing if (isP2SH && !IsHex(strRedeemScript as the deleted line, but this thread is about an earlier code branch.

in src/wallet/rpcdump.cpp:877 in 3276b7e22a outdated

884+
885             // Import redeem script.
886             std::vector<unsigned char> vData(ParseHex(strRedeemScript));
887             CScript redeemScript = CScript(vData.begin(), vData.end());
888+            CScriptID redeem_id(redeemScript);
889+            CScript redeemDestination = GetScriptForDestination(redeem_id);

achow101 commented at 2:38 am on October 14, 2018:

nit: snake_case

in src/wallet/rpcdump.cpp:822 in 3276b7e22a outdated

823         }
824+        const std::string& output = isScript ? scriptPubKey.get_str() : scriptPubKey["address"].get_str();
825 
826         // Optional fields.
827         const std::string& strRedeemScript = data.exists("redeemscript") ? data["redeemscript"].get_str() : "";
828+        const std::string& strWitnessScript = data.exists("witnessscript") ? data["witnessscript"].get_str() : "";

achow101 commented at 2:38 am on October 14, 2018:

nit: snake_case

in src/wallet/rpcdump.cpp:902 in 3276b7e22a outdated

950-            // Import public keys.
951-            if (pubKeys.size() && keys.size() == 0) {
952-                const std::string& strPubKey = pubKeys[0].get_str();
953+                // Generate the scripts
954+                std::vector<unsigned char> vData(ParseHex(strWitnessScript));
955+                CScript witnessScript = CScript(vData.begin(), vData.end());

achow101 commented at 2:40 am on October 14, 2018:

nit: snake_case

in src/wallet/rpcdump.cpp:904 in 3276b7e22a outdated

952-                const std::string& strPubKey = pubKeys[0].get_str();
953+                // Generate the scripts
954+                std::vector<unsigned char> vData(ParseHex(strWitnessScript));
955+                CScript witnessScript = CScript(vData.begin(), vData.end());
956+                CScriptID witness_id(witnessScript);
957+                CScript witnessDestination = GetScriptForDestination(WitnessV0ScriptHash(witnessScript));

achow101 commented at 2:40 am on October 14, 2018:

nit: snake_case

in src/wallet/rpcdump.cpp:950 in 3276b7e22a outdated

1016-                if (!pwallet->AddWatchOnly(pubKeyScript, timestamp)) {
1017-                    throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
1018+        // P2PK/P2PKH/P2WPKH
1019+        } else if (dest.type() == typeid(CKeyID) || dest.type() == typeid(WitnessV0KeyHash)) {
1020+            if (keys.size() > 1 || pubKeys.size() > 1) {
1021+                throw JSONRPCError(RPC_INVALID_PARAMETER, "More than private key given for one address");

achow101 commented at 2:42 am on October 14, 2018:

This error wouldn’t make sense if only pubkeys were given.

in src/wallet/rpcdump.cpp:961 in 3276b7e22a outdated

1030+                if (pubkey.size() && pubkey_temp != pubkey) {
1031+                    throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Private key does not match public key for address");
1032                 }
1033+                pubkey = pubkey_temp;
1034+            }
1035+            if (pubkey.size()) {

achow101 commented at 2:44 am on October 14, 2018:

Instead of checking pubkey size, check validity? (e.g. use IsValid() or IsFullyValid())

meshcollider commented at 7:53 am on October 14, 2018:

IsValid() just performs the same size check anyway, but yep I’ll do that for the sake of clarity, I don’t think we need an IsFullyValid() check here

achow101 commented at 2:50 am on October 14, 2018: member

Concept ACK

59f8e547130fc1cb756da7d990f65445ca196f5f could be squashed into 24092b3402693d8a2c39b640db18748b168863e9

acdba74ee011bfcafc9f28b528918815b1de4920 seems to be an unrelated change

meshcollider commented at 9:22 am on October 14, 2018: contributor

Thanks @achow101, addressed all your points

sipa commented at 9:16 pm on October 15, 2018: member

@MeshCollider I don’t think it’s necessary to import the pubkeys involved separately anymore since #14424, and in fact that sounds very dangerous (you could be tricked into importing a 2-of-3 multisig where you have 2 of the keys, but then receiving a payment to a P2PKH of the third key, and seeing it treated towards your watch-only balance). NACK until that is fixed (or at least restricted to not have that issue, but I believe that rebasing on 14424 will be sufficient).

meshcollider commented at 0:15 am on October 16, 2018: contributor

@sipa good point, fixed, and rebased on master to include the public key fix

sipa commented at 0:32 am on October 17, 2018: member

I think there are a few things that need fixing:

There doesn’t seem to be code for P2SH-P2WPKH (which matters at least when passing in a pubkey in that case, which won’t be imported).
The added tests only cover whether importmulti returns true, not whether it actually imported anything.
The added tests are restricted to watchonly and spendable cases, but no solvable-but-not-spendable ones.

meshcollider commented at 2:50 am on October 17, 2018: contributor

I believe I’ve addressed the points, thanks for the feedback so far

in src/wallet/rpcdump.cpp:816 in 65ecf2d881 outdated

817-
818-        // Should have script or JSON with "address".
819-        if (!(scriptPubKey.getType() == UniValue::VOBJ && scriptPubKey.exists("address")) && !(scriptPubKey.getType() == UniValue::VSTR)) {
820+        bool isScript = scriptPubKey.getType() == UniValue::VSTR;
821+        if (!isScript && !(scriptPubKey.getType() == UniValue::VOBJ && scriptPubKey.exists("address"))) {
822             throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid scriptPubKey");

Sjors commented at 6:07 am on October 17, 2018:

0            throw JSONRPCError(RPC_INVALID_PARAMETER, "scriptPubKey must be string with script or JSON with address string");

Sjors commented at 6:10 am on October 17, 2018:

I’m not sure how the code suggestion feature works. I suspect it creates a new commit, in which case it’s better to just copy-paste the text and amend an existing commit.

instagibbs commented at 2:39 pm on October 17, 2018:

Good time to give a more descriptive error?

in src/wallet/rpcdump.cpp:858 in cd19a2e21e outdated

852@@ -858,32 +853,31 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
853             throw JSONRPCError(RPC_INVALID_PARAMETER, "Incompatibility found between watchonly and keys");
854         }
855 
856-        // Internal + Label
857+        // Internal addresses should not have a label
858         if (internal && data.exists("label")) {
859             throw JSONRPCError(RPC_INVALID_PARAMETER, "Incompatibility found between internal and label");

Sjors commented at 6:20 am on October 17, 2018:

Suggest using the same error message as the above comment: Internal addresses should not have a label

instagibbs commented at 2:42 pm on October 17, 2018:

Could we also get a test for this?

in src/wallet/rpcdump.cpp:926 in 65ecf2d881 outdated

931+            if (!pwallet->HaveCScript(witness_id) && !pwallet->AddCScript(witness_script)) {
932+                throw JSONRPCError(RPC_WALLET_ERROR, "Error adding p2wsh witnessScript to wallet");
933             }
934 
935-            // Import private keys.
936+        // P2PK/P2PKH/P2WPKH

sipa commented at 6:37 am on October 17, 2018:

I think it may make sense to repeat the trick from the P2SH block of ‘descending’ into scriptPubKey/address of the redeemscript, to the P2WSH block, and then do P2PK/P2PKH/P2WPKH as a totally new case rather than an else branch. This would make the code work correctly for P2PK/P2PKH nested inside P2WSH (maybe track that you don’t permit P2WPKH inside P2WSH though).

in src/wallet/rpcdump.cpp:809 in cd19a2e21e outdated

808@@ -809,29 +809,24 @@ UniValue dumpwallet(const JSONRPCRequest& request)
809 static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)

Sjors commented at 6:40 am on October 17, 2018:

Maybe move this function to a different file? Unless dump means dumping ground :-)

I understand if you prefer to keep this PR simple though.

sipa commented at 7:45 pm on October 17, 2018:

It’s specific to importing things, though, which is the scope of rpcdump.cpp?

in test/functional/wallet_importmulti.py:471 in 65ecf2d881 outdated

467@@ -458,6 +468,74 @@ def run_test (self):
468                 "timestamp": "",
469             }])
470 
471+        # Import P2WPKH address

sipa commented at 6:47 am on October 17, 2018:

There are a number more scenarios that make sense to test, I think:

Address based import (without key/script) of P2WSH-multisig
For all of P2WPKH, P2WSH-multisig, P2SH-P2WPKH, P2SH-P2WSH-multisig, versions with script/pubkey but no private key, and test that the result is solvable.
P2WPKH with private key, result must be spendable (not just watchonly)
P2WSH-multisig and P2SH-multisig with more than 1 key, where some of the keys are specified as private keys, and some as public keys.

Sjors commented at 7:07 am on October 17, 2018:

Re “Address based import”, should this result in the following?

0  "ismine": false,
1  "iswatchonly": true,
2  "issolvable": false,
3  "isscript": true,
4  "iswitness": true,
5  "witness_version": 0

Re “must be spendable”, this means "ismine": true and "iswatchonly": false?

sipa commented at 9:36 pm on October 17, 2018:

@Sjors Correct.

sipa commented at 6:48 am on October 17, 2018: member

Concept ACK

Sjors commented at 8:26 am on October 17, 2018: member

Concept ACK.

I tested 65ecf2d importing a watch-only bech32 address, which worked.

I also tested importing a watch-only p2sh-p2wpkh address like so: importmulti '[{"scriptPubKey": {"address": "2NFiDQiCdRYaRUdWMerff1qSTR4EC7zSBmB"}, "timestamp": "now", "pubkeys": ["03f33112792b07e9994933287ea71069d94176225f745822e47151d39e754d957f"]}]'. This is considered "issolvable": false. Once I add the private key it does know how to solve it.

Code looks good to me, but I don’t pretend to understand all the edge cases involved. The tests really help illustrate it, so the more the better, see Sipa’s suggestions: https://github.com/bitcoin/bitcoin/pull/14454/commits/480cba3425efece7704f6350c915268079675853#r225796932)

in src/wallet/rpcdump.cpp:863 in 65ecf2d881 outdated

862-        // Keys / PubKeys size check.
863-        if (!isP2SH && (keys.size() > 1 || pubKeys.size() > 1)) { // Address / scriptPubKey
864-            throw JSONRPCError(RPC_INVALID_PARAMETER, "More than private key given for one address");
865+        // Force users to provide the witness script in its field rather than redeemscript
866+        if (!strRedeemScript.empty() && script.IsPayToWitnessScriptHash()) {
867+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Provide witnessscript not redeemscript for P2WSH address");

instagibbs commented at 2:47 pm on October 17, 2018:

wording suggestion: “P2WSH addresses have an empty redeemscript. Please provide the witnessscript instead.”

in src/wallet/rpcdump.cpp:873 in 65ecf2d881 outdated

879         // P2SH
880-        if (isP2SH) {
881+        if (!strRedeemScript.empty() && script.IsPayToScriptHash()) {
882+            // Check the redeemScript is valid
883+            if (!IsHex(strRedeemScript)) {
884+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid redeem script");

instagibbs commented at 2:48 pm on October 17, 2018:

suggestion: “Invalid redeem script: must be hex string”

in src/wallet/rpcdump.cpp:818 in 65ecf2d881 outdated

819-        if (!(scriptPubKey.getType() == UniValue::VOBJ && scriptPubKey.exists("address")) && !(scriptPubKey.getType() == UniValue::VSTR)) {
820+        bool isScript = scriptPubKey.getType() == UniValue::VSTR;
821+        if (!isScript && !(scriptPubKey.getType() == UniValue::VOBJ && scriptPubKey.exists("address"))) {
822             throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid scriptPubKey");
823         }
824+        const std::string& output = isScript ? scriptPubKey.get_str() : scriptPubKey["address"].get_str();

instagibbs commented at 2:54 pm on October 17, 2018:

while we’re here could we rename this to something more self-documenting?

meshcollider commented at 0:12 am on October 18, 2018:

@instagibbs what do you suggest? script_or_address?

in src/wallet/rpcdump.cpp:893 in cd19a2e21e outdated

890             if (!pwallet->HaveCScript(redeem_id) && !pwallet->AddCScript(redeemScript)) {
891                 throw JSONRPCError(RPC_WALLET_ERROR, "Error adding p2sh redeemScript to wallet");
892             }
893 
894-            CScript redeemDestination = GetScriptForDestination(redeem_id);
895+            // Check for P2SH-P2WSH

instagibbs commented at 2:58 pm on October 17, 2018:

I understand this is a transient commit, but could there be a TODO P2SH-P2WPKH comment somewhere around here?

in src/wallet/rpcdump.cpp:919 in cd19a2e21e outdated

943-                    assert(key.VerifyPubKey(pubkey));
944-
945-                    CKeyID vchAddress = pubkey.GetID();
946-                    pwallet->MarkDirty();
947-                    pwallet->SetAddressBook(vchAddress, label, "receive");
948+        // P2WSH

instagibbs commented at 7:43 pm on October 17, 2018:

// (P2SH-)P2WSH

Makes it more clear to me that the above block changed the script

instagibbs commented at 7:51 pm on October 17, 2018:

And same thing with: // P2PK/P2PKH/P2WPKH

in src/wallet/rpcdump.cpp:866 in 65ecf2d881 outdated

871-        if (isP2SH && !IsHex(strRedeemScript)) {
872-            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid redeem script");
873-        }
874-
875-        // Process. //
876+        CScript original_script = script;

instagibbs commented at 7:47 pm on October 17, 2018:

scriptpubkey_script more self-explanatory? “original” is context dependent.

in src/wallet/rpcdump.cpp:867 in 65ecf2d881 outdated

872-            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid redeem script");
873-        }
874-
875-        // Process. //
876+        CScript original_script = script;
877+        CTxDestination original_dest = dest;

instagibbs commented at 7:47 pm on October 17, 2018:

scriptpubkey_dest more self-explanatory? “original” is context dependent.

meshcollider commented at 4:19 am on October 18, 2018: contributor

All comments so far are addressed, lots more tests added. Thanks @sipa, @instagibbs, @Sjors

Sjors commented at 7:57 am on October 18, 2018: member

@MeshCollider I’m still seeing the same behavior I described above when importing a p2sh-p2wpkh address using just the address and public key.

sipa commented at 8:00 am on October 18, 2018: member

@Sjors You need the redeemscript to make your example solvable (otherwise there is no way for the matcher to go from a P2SH hash to knowing what script the hash is of).

With the private key the weird-evil-but-necessary magic behaviour that causes us to automatically recognize P2WPKH and P2SH-P2WPKH payments to known private keys triggers.

Sjors commented at 8:12 am on October 18, 2018: member

Your wording suggests you’re not a fan of adding that weird-evil-but-optional magic behavior to the (very common) case of having one public key?

meshcollider commented at 8:19 am on October 18, 2018: contributor

@Sjors I’m not really a fan of it, this code is already edge-cased enough without adding more ifs to test for, but I’m happy to add it if you really think its worthwhile.

sipa commented at 8:19 am on October 18, 2018: member

@Sjors No, but I have suggested something else before, which would perhaps also solve your issue, namely that for all single-key schemes you can just provide the pubkey, and importmulti would automatically try the different schemes to see which matches your address/script.

At the very least, something for another PR, though.

meshcollider commented at 8:26 am on October 18, 2018: contributor

And such a case would be so easy with descriptors too ;)

Sjors commented at 9:00 am on October 18, 2018: member

I’m fine with focusing on descriptors, since those are an order of magnitude more intuitive than the current importmulti syntax.

jonasschnelli added this to the "Blockers" column in a project

meshcollider commented at 12:35 pm on October 19, 2018: contributor

Fixed a bug where a P2WSH multisig was not treated as IsMine Spendable because the scriptPubKey wasn’t being added to the wallet, and updated the test for it.

in test/functional/wallet_importmulti.py:481 in 06d5999b28 outdated

476@@ -458,6 +477,148 @@ def run_test (self):
477                 "timestamp": "",
478             }])
479 
480+        # Import P2WPKH address as watch only
481+        self.log.info("Should import a P2WPKH address was watch only")

sipa commented at 0:53 am on October 20, 2018:

Typo: was -> as

in src/wallet/rpcwallet.cpp:3541 in 06d5999b28 outdated

3537@@ -3538,6 +3538,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3538             "  \"address\" : \"address\",        (string) The bitcoin address validated\n"
3539             "  \"scriptPubKey\" : \"hex\",       (string) The hex-encoded scriptPubKey generated by the address\n"
3540             "  \"ismine\" : true|false,        (boolean) If the address is yours or not\n"
3541+            "  \"issolvable\" : true|false,    (boolean) If the address is solvable by the wallet\n"

sipa commented at 0:55 am on October 20, 2018:

Addressing a review comment, I’ve changed the implementation in #14477 to use solvable instead of issolvable. Perhaps you can do the same?

in test/functional/wallet_importmulti.py:565 in 06d5999b28 outdated

560+        result = self.nodes[1].importmulti([{
561+            "scriptPubKey": {
562+                "address": sig_address_1['address']
563+            },
564+            "timestamp": "now",
565+            "redeemscript": bytes_to_hex_str(pkscript)

sipa commented at 0:57 am on October 20, 2018:

redeemscript is not necessary if you don’t need solvability.

meshcollider commented at 10:47 am on October 20, 2018: contributor

Comments addressed + squashed some commits together

in src/wallet/rpcdump.cpp:919 in 7c0268acb5 outdated

924 
925-            // add to address book or update label
926-            if (IsValidDestination(dest)) {
927-                pwallet->SetAddressBook(dest, label, "receive");
928+            // Import into the wallet
929+            if (!pwallet->AddWatchOnly(witness_script, timestamp)) {

sipa commented at 1:34 am on October 23, 2018:

A small issue still: for P2SH-P2WSH it’s unnecessary (and undesirable, I think) to also import the inner P2WSH script as watch-only.

It’s not necessary because we’re already marking the P2SH-P2WSH script as watch-only, and for solvability all we need is AddCScript, not AddWatchOnly.

It’s undesirable because it will cause us to treat payments to the P2WSH address as watchonly IsMine (instead of just the P2SH-P2WSH version).

meshcollider commented at 2:19 am on October 23, 2018:

Good point, fixed

in test/functional/wallet_importmulti.py:492 in e4a66bb791 outdated

487+            "timestamp": "now",
488+        }])
489+        assert_equal(result[0]['success'], True)
490+        address_assert = self.nodes[1].getaddressinfo(address['address'])
491+        assert_equal(address_assert['iswatchonly'], True)
492+        assert_equal(address_assert['solvable'], False)

Sjors commented at 6:07 am on October 23, 2018:

@sipa can you remind me why a Native Pay-to-Witness-Public-Key-Hash isn’t solvable, even though legacy Pay-to-Public-Key-Hash addresses are? Is that just to reduce evil magic like in the P2SH-P2PKH case? If so, maybe add a comment in the test or in the RPC doc?

sipa commented at 7:13 am on October 23, 2018:

In this example it’s not solvable because the public key isn’t included. It has nothing to do with the script/witness hashing type.

In general, solvability is very easy. Start at the scriptPubKey. Whenever it includes a script hash, we must receive the actual script, and recurse into it. Whenever you encounter a pubkey hash, we must receive the actual public key. A scriptPubKey is solvable whenever you can descend all the way to scripts and pubkeys without any hashes of unknown things.

The only exception to that is that whenever you have a private key X, we also automatically import the P2WPKH and P2SH-P2WPKH versions of it, so no separate importing is needed in that case.

Sjors commented at 7:50 am on October 23, 2018:

In this example it’s not solvable because the public key isn’t included.

OK, that makes sense. Actually I misread the legacy test: it does add the public key, which why it’s solvable.

Sjors commented at 6:46 am on October 23, 2018: member

I added a few more questions inline, hopefully merely to reduce my own confusion.

My above concerns are addressed in e4a66bb, modulo two tests that @sipa requested and are either missing or I overlooked them.

The added tests only cover whether importmulti returns true, not whether it actually imported anything.

This is done now by checking properties via getaddressinfo.

The added tests are restricted to watchonly and spendable cases, but no solvable-but-not-spendable ones.

Some of the ['solvable'], True) tests now check that ['ismine'], False, but none check for iswatchonly'],False].

Address based import (without key/script) of P2WSH-multisig

Covered in # P2WSH multisig address without scripts or keys and # P2SH-P2WSH 1-of-1 multisig + redeemscript with no private key

For all of P2WPKH, P2WSH-multisig, P2SH-P2WPKH, P2SH-P2WSH-multisig, versions with script/pubkey but no private key, and test that the result is solvable.

I think this one is still missing for P2WSH multisig, between the # P2WSH multisig address without scripts or keys and # Same P2WSH multisig address as above, but now with witnessscript + private keys tests?

P2WPKH with private key, result must be spendable (not just watchonly)

Where “spendable” means ("ismine": true and "iswatchonly": false). This is done.

P2WSH-multisig and P2SH-multisig with more than 1 key, where some of the keys are specified as private keys, and some as public keys.

Missing?

I made a commit that checks solvable for all legacy scenarios as well: https://github.com/Sjors/bitcoin/commit/02d553b3e6fa78aa76cd825135a496590a243595

sipa commented at 8:02 pm on October 23, 2018: member

utACK e4a66bb791cc832b192f4fd780d3159b98f494e3. As @Sjors points out there are some combinations for which tests can be added, but I think that can be done later.

Please squash fixups?

meshcollider commented at 8:29 pm on October 24, 2018: contributor

Squashed fixups into their respective commits, thanks for the review :)

Add SegWit support to importmulti with some ProcessImport cleanup f6ed748cf0

Fix typo in test_framework/blocktools 353c064596

Add release notes for importmulti segwit change 1753d217ea

Make getaddressinfo return solvability 201451b1ca

Add segwit address tests for importmulti c11875c590

sipa commented at 6:58 pm on October 25, 2018: member

utACK c11875c5908a17314bb38caa911507dc6401ec49

in test/functional/wallet_importmulti.py:537 in c11875c590

532+            },
533+            "timestamp": "now"
534+        }])
535+        assert_equal(result[0]['success'], True)
536+        address_assert = self.nodes[1].getaddressinfo(multi_sig_script['address'])
537+        assert_equal(address_assert['solvable'], False)

instagibbs commented at 7:09 pm on October 25, 2018:

nit: if “sigsrequired” and “ismine” aren’t in results can we assert their absence here?

in test/functional/wallet_importmulti.py:620 in c11875c590

615+            "redeemscript": bytes_to_hex_str(redeem_script),
616+            "witnessscript": multi_sig_script['redeemScript']
617+        }])
618+        assert_equal(result[0]['success'], True)
619+        address_assert = self.nodes[1].getaddressinfo(multi_sig_script['address'])
620+        assert_equal(address_assert['solvable'], True)

instagibbs commented at 7:12 pm on October 25, 2018:

nit: ismine?

instagibbs approved

instagibbs commented at 7:12 pm on October 25, 2018: member

utACK

in src/wallet/rpcwallet.cpp:3541 in c11875c590

3537@@ -3538,6 +3538,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3538             "  \"address\" : \"address\",        (string) The bitcoin address validated\n"
3539             "  \"scriptPubKey\" : \"hex\",       (string) The hex-encoded scriptPubKey generated by the address\n"
3540             "  \"ismine\" : true|false,        (boolean) If the address is yours or not\n"
3541+            "  \"solvable\" : true|false,      (boolean) If the address is solvable by the wallet\n"

promag commented at 9:28 am on October 26, 2018:

Could have release note.

in src/wallet/rpcdump.cpp:934 in c11875c590

967+        }
968 
969-                    pwallet->UpdateTimeFirstKey(timestamp);
970-                }
971+        // (P2SH-)P2PK/P2PKH/P2WPKH
972+        if (dest.type() == typeid(CKeyID) || dest.type() == typeid(WitnessV0KeyHash)) {

promag commented at 9:53 am on October 26, 2018:

First time using RTTI? Maybe it could be avoided?

BTW, didn’t test but I don’t see variant::type() in boost 1.47 documentation.

meshcollider commented at 6:17 am on October 29, 2018:

@promag it seems to be there: https://www.boost.org/doc/libs/1_47_0/doc/html/boost/variant.html I could also do dest.which() == 1 || dest.which() == 4 but that’s very unclear, or define a new enum of the types, but that seemed over the top unless there’s a good reason to avoid RTTI?

promag commented at 12:25 pm on October 29, 2018:

Indeed it’s in 1.47. Anyway this is removed in #14565.

achow101 commented at 4:35 am on October 30, 2018: member

utACK 201451b1ca3c6db3b13f9491a81db5b120b864bb

laanwj commented at 4:44 pm on October 31, 2018: member

looks good to me, adding the solvable to getaddressinfo makes a lot of sense, and tests also look good to me utACK c11875c5908a17314bb38caa911507dc6401ec49

laanwj merged this on Oct 31, 2018

laanwj closed this on Oct 31, 2018

laanwj referenced this in commit b312579c69 on Oct 31, 2018

meshcollider deleted the branch on Oct 31, 2018

laanwj removed this from the "Blockers" column in a project

laanwj referenced this in commit 9127bd7aba on Feb 7, 2019

deadalnix referenced this in commit 41707be22c on May 15, 2020

DrahtBot locked this on Sep 8, 2021

Add SegWit support to importmulti #14454