Import pubkeys when importing p2sh with importmulti #14019

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:import-multi-pubkeys changing 2 files +62 −0
  1. achow101 commented at 9:29 pm on August 21, 2018: member
    This PR has importmulti import public keys if specified instead of ignoring them when the thing being imported is p2sh.
  2. fanquake added the label Wallet on Aug 21, 2018
  3. fanquake added the label RPC/REST/ZMQ on Aug 21, 2018
  4. in src/wallet/rpcdump.cpp:950 in 54ab69ce78 outdated 
    951+                }
952+
953+                // TODO Is this necessary?
954+                CScript scriptRawPubKey = GetScriptForRawPubKey(pubKey);
955+
956+                if (::IsMine(*pwallet, scriptRawPubKey) == ISMINE_SPENDABLE) {
    instagibbs commented at 1:45 pm on August 22, 2018:
    Curious why we aren’t calling HaveKey here and in similar locations?
    achow101 commented at 2:31 pm on August 22, 2018:
    Dunno. I just copied this from below
  5. in src/wallet/rpcdump.cpp:936 in 54ab69ce78 outdated 
    937+
938+                if (::IsMine(*pwallet, pubKeyScript) == ISMINE_SPENDABLE) {
939+                    throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script");
940+                }
941+
942+                pwallet->MarkDirty();
    instagibbs commented at 1:48 pm on August 22, 2018:
    why the MarkDirty here? Needs a comment imo.
    achow101 commented at 2:33 pm on August 22, 2018:
    Copied from below. It seems that we always do MarkDirty before importing stuff. This pattern appears throughout the other import stuff.
    promag commented at 11:23 pm on September 23, 2018:
    If #14303 is correct, these can be removed. Please review.
  6. in src/wallet/rpcdump.cpp:954 in 54ab69ce78 outdated 
    955+
956+                if (::IsMine(*pwallet, scriptRawPubKey) == ISMINE_SPENDABLE) {
957+                    throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script");
958+                }
959+
960+                pwallet->MarkDirty();
    instagibbs commented at 1:48 pm on August 22, 2018:
    why the MarkDirty here? Needs a comment imo.
  7. in src/wallet/rpcdump.cpp:948 in 54ab69ce78 outdated 
    949+                if (IsValidDestination(pubkey_dest)) {
950+                    pwallet->SetAddressBook(pubkey_dest, label, "receive");
951+                }
952+
953+                // TODO Is this necessary?
954+                CScript scriptRawPubKey = GetScriptForRawPubKey(pubKey);
    instagibbs commented at 1:53 pm on August 22, 2018:
    Seems kind of expected until we stop abusing keys sadly.
  8. DrahtBot commented at 4:40 am on August 23, 2018: member
    • #14454 (Add SegWit support to importmulti by MeshCollider)
    • #14075 (Import watch only pubkeys to the keypool if private keys are disabled by achow101)
    • #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.

  9. instagibbs commented at 2:08 pm on August 25, 2018: member

    utACK 54ab69ce78a88f24c96ebcab09c63da27db2ee9d

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

  10. achow101 force-pushed on Aug 26, 2018
  11. luke-jr commented at 5:46 pm on August 29, 2018: member
    Why? If you’re importing p2sh, the wallet shouldn’t recognise lone keys…
  12. achow101 commented at 8:29 pm on August 29, 2018: member
    @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.
  13. sipa commented at 8:34 pm on August 29, 2018: member

    @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.

  14. laanwj added this to the "Blockers" column in a project
  15. instagibbs commented at 7:56 pm on September 14, 2018: member

    tACK https://github.com/bitcoin/bitcoin/pull/14019/commits/7bcf2cbbf89389d05bc66fb254d9775e846a2f0c

    In use on active wallets.

  16. jb55 approved
  17. jb55 commented at 7:55 pm on September 22, 2018: member

    utACK 7bcf2cbbf89389d05bc66fb254d9775e846a2f0c

    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.

  18. in test/functional/wallet_importmulti.py:464 in 7bcf2cbbf8 outdated 
    459+                'redeemscript' : ms['redeemScript'],
460+                'pubkeys' : [pub1, pub2],
461+                "timestamp": "now",
462+            }]
463+        )
464+        assert result[0]['success']
    promag commented at 10:03 pm on September 23, 2018:
    nit, assert_equal(result[0]['success'], True).
    achow101 commented at 3:53 am on September 27, 2018:
    Done
  19. in src/wallet/rpcdump.cpp:922 in 7bcf2cbbf8 outdated 
    916@@ -917,6 +917,53 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
917                 pwallet->SetAddressBook(dest, label, "receive");
918             }
919 
920+            // Import public keys
921+            for (size_t i = 0; i < pubKeys.size(); ++i) {
922+                const std::string& pubkey = pubKeys[i].get_str();
    promag commented at 10:07 pm on September 23, 2018:

    Use HexToPubKey:

    0CPubKey pubKey = HexToPubKey(pubKeys[i].get_str())
    achow101 commented at 3:53 am on September 27, 2018:
    Done
  20. in src/wallet/rpcdump.cpp:922 in 7bcf2cbbf8 outdated 
    916@@ -917,6 +917,53 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
917                 pwallet->SetAddressBook(dest, label, "receive");
918             }
919 
920+            // Import public keys
921+            for (size_t i = 0; i < pubKeys.size(); ++i) {
    promag commented at 10:10 pm on September 23, 2018:
    nit, use range-based for-loop?
    achow101 commented at 3:50 am on September 27, 2018:
    This does not work as UniValue does not have an end() function apparently.
  21. in test/functional/wallet_importmulti.py:458 in 7bcf2cbbf8 outdated 
    453+        pub1 = self.nodes[1].getaddressinfo(addr1)['pubkey']
454+        pub2 = self.nodes[1].getaddressinfo(addr2)['pubkey']
455+        ms = self.nodes[1].createmultisig(2, [pub1, pub2])
456+        result = self.nodes[0].importmulti(
457+            [{
458+                'scriptPubKey' : { 'address' : ms['address']},
    promag commented at 10:16 pm on September 23, 2018:

    nit, space before }?

    we should style lint python :trollface:

    achow101 commented at 3:53 am on September 27, 2018:
    Done
  22. Import pubkeys when importing p2sh with importmulti 6412265b59
  23. achow101 force-pushed on Sep 27, 2018
  24. in src/wallet/rpcdump.cpp:929 in 6412265b59 
    924+
925+                if (!pubKey.IsFullyValid()) {
926+                    throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey is not a valid public key");
927+                }
928+
929+                CTxDestination pubkey_dest = pubKey.GetID();
    etscrivner commented at 3:40 am on September 28, 2018:
    nit: pubkey_dest => pubKeyDest?
    achow101 commented at 4:45 am on September 28, 2018:
    No. The style guide says to use snake case for variable names.
    etscrivner commented at 7:08 pm on September 28, 2018:
    Any reason why the other local variables don’t follow this naming convention? (For example, pubKey, pubKeyScript, and scriptRawPubKey)
    achow101 commented at 7:13 pm on September 28, 2018:
    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.
    etscrivner commented at 8:57 pm on September 28, 2018:
    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.
  25. etscrivner commented at 4:17 am on September 28, 2018: contributor
    tACK 6412265b5935173fcaa02b20195d0e282cb2d804
  27. meshcollider commented at 1:25 pm on October 10, 2018: contributor
    this PR is replaced by #14454, please review that instead :)
  28. fanquake commented at 11:29 am on October 14, 2018: member
    @achow101 Is #14454 a full replacement for this PR? If so, can you close this.
  29. achow101 commented at 9:06 pm on October 15, 2018: member
    Superseded by #14454. Also, from discussion on IRC just now, it seems that this feature is no longer necessary following #14424
  30. achow101 closed this on Oct 15, 2018
  31. meshcollider removed this from the "Blockers" column in a project
  32. laanwj referenced this in commit b312579c69 on Oct 31, 2018
  33. DrahtBot locked this on Sep 8, 2021


achow101 instagibbs promag DrahtBot luke-jr sipa jb55 etscrivner meshcollider fanquake

Labels
Wallet RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me