Refactor: Require scriptPubKey to get wallet SigningProvider #17371

achow101 · 2019-11-04T22:03:45Z

Easier to review ignoring whitespace:

git log -p -n1 -w

This commit does not change behavior. It passes new CScript arguments to
signing functions, but the arguments aren't currently used.

Split from #17261

DrahtBot · 2019-11-04T23:27:28Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17369 (Refactor: Move encryption code between KeyMan and Wallet by achow101)
#17283 (rpc: improve getaddressinfo test coverage, help, code docs by jonatack)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
#12911 (wallet: Show fee in results for signrawtransaction* for segwit inputs by kallewoof)

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.

src/wallet/psbtwallet.cpp

src/wallet/rpcwallet.cpp

ryanofsky

Code review ACK f7510c1. Pretty straightforward to verify this doesn't change behavior.

ryanofsky · 2019-11-05T17:18:59Z

src/wallet/rpcwallet.cpp

@@ -550,7 +550,11 @@ static UniValue signmessage(const JSONRPCRequest& request)
        throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to key");
    }

-    const SigningProvider* provider = pwallet->GetSigningProvider();
+    CScript script_pub_key = GetScriptForDestination(*pkhash);


In commit "Refactor: Require scriptPubKey to get wallet SigningProvider" (f7510c1)

It would seem more direct to replace *pkhash with dest here and call GetScriptForDestination(dest)

I don't think it really matters as we still need pkhash since signmessage is supposed to only work on p2pkh addresses.

we still need pkhash since signmessage is supposed to only work on p2pkh addresses

That will change in https://github.com/bitcoin/bitcoin/pull/16440/files#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR559

src/wallet/rpcwallet.cpp

SignTransaction will be called multiple times in the future. Pass it a result UniValue so that it can accumulate the results of multiple SignTransaction passes.

Easier to review ignoring whitespace: git log -p -n1 -w This commit does not change behavior. It passes new CScript arguments to signing functions, but the arguments aren't currently used.

ryanofsky

Code review ACK d0dab89. Thanks for the SignTransaction update. No other changes since last review

Sjors

Code review ACK d0dab89

Sjors · 2019-11-19T11:45:17Z

src/wallet/rpcwallet.cpp

@@ -550,7 +550,11 @@ static UniValue signmessage(const JSONRPCRequest& request)
        throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to key");
    }

-    const SigningProvider* provider = pwallet->GetSigningProvider();
+    CScript script_pub_key = GetScriptForDestination(*pkhash);


we still need pkhash since signmessage is supposed to only work on p2pkh addresses

That will change in https://github.com/bitcoin/bitcoin/pull/16440/files#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR559

ryanofsky · 2019-11-21T15:27:47Z

@instagibbs and @meshcollider, you may be interested to review this. This is code you previously acked when it was part of #16341

promag

Code review ACK d0dab89.

promag · 2019-11-21T15:52:51Z

src/rpc/rawtransaction_util.h

 */
-UniValue SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, const UniValue& hashType);
+void SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, const UniValue& hashType, UniValue& result);


4b0c718

An overload could be added, like

UniValue SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, const UniValue& hashType) { UniValue result(UniValue::VOBJ); SignTransaction(mtx, &keystore, coins, hashType, result); return result; }

But I see that in the next commit this refactor is handy in signrawtransactionwithwallet RPC.

instagibbs · 2019-11-22T16:36:36Z

src/interfaces/wallet.cpp

@@ -117,8 +117,22 @@ class WalletImpl : public Wallet
        std::string error;
        return m_wallet->GetNewDestination(type, label, dest, error);
    }
-    bool getPubKey(const CKeyID& address, CPubKey& pub_key) override { return m_wallet->GetLegacyScriptPubKeyMan()->GetPubKey(address, pub_key); }
-    bool getPrivKey(const CKeyID& address, CKey& key) override { return m_wallet->GetLegacyScriptPubKeyMan()->GetKey(address, key); }
+    bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) override


note for future work(TM): address is a horrible name for this argument

instagibbs · 2019-11-22T16:38:15Z

src/wallet/wallet.cpp

@@ -2096,7 +2100,7 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector<
                continue;
            }

-            const SigningProvider* provider = GetSigningProvider();
+            const SigningProvider* provider = GetSigningProvider(wtx.tx->vout[i].scriptPubKey);


~~do we need to check for knowledge of provider here?~~

oh i cannot read.

instagibbs · 2019-11-22T16:39:54Z

src/wallet/rpcwallet.cpp

+            providers.insert(std::move(provider));
+        }
+    }
+    if (providers.size() == 0) {


non-blocking-nit: .empty()

Not fixing to avoid invalidating ACKs.

instagibbs · 2019-11-22T16:43:07Z

utACK d0dab89

instagibbs · 2019-11-22T17:13:21Z

ready for merge? @meshcollider

meshcollider

Code review ACK d0dab89

meshcollider · 2019-11-22T19:18:45Z

src/interfaces/wallet.cpp

+    {
+        const SigningProvider* provider = m_wallet->GetSigningProvider(script);
+        if (provider) {
+            return provider->GetPubKey(address, pub_key);


Its kinda not that nice that there is no check the script and address have anything to do with each other. You could end up requesting the wrong SPKM for an address quite easily. Maybe move some of the ExtractDestination stuff inside here instead?

meshcollider · 2019-11-22T19:29:01Z

src/wallet/wallet.cpp

-    const SigningProvider* provider = GetSigningProvider();
+    const SigningProvider* provider = GetSigningProvider(scriptPubKey);
+    if (!provider) {
+        // We don't know about this scriptpbuKey;


nit: typo (same below)

meshcollider · 2019-11-22T19:32:26Z

src/wallet/rpcwallet.cpp

@@ -2971,8 +2977,11 @@ static UniValue listunspent(const JSONRPCRequest& request)
        entry.pushKV("spendable", out.fSpendable);
        entry.pushKV("solvable", out.fSolvable);
        if (out.fSolvable) {
-            auto descriptor = InferDescriptor(scriptPubKey, *pwallet->GetLegacyScriptPubKeyMan());
-            entry.pushKV("desc", descriptor->ToString());
+            const SigningProvider* provider = pwallet->GetSigningProvider(scriptPubKey);


This can probably be combined with above to simplify

…vider d0dab89 Refactor: Require scriptPubKey to get wallet SigningProvider (Andrew Chow) 4b0c718 Accumulate result UniValue in SignTransaction (Andrew Chow) Pull request description: Easier to review ignoring whitespace: git log -p -n1 -w This commit does not change behavior. It passes new CScript arguments to signing functions, but the arguments aren't currently used. Split from #17261 ACKs for top commit: instagibbs: utACK d0dab89 ryanofsky: Code review ACK d0dab89. Thanks for the SignTransaction update. No other changes since last review Sjors: Code review ACK d0dab89 promag: Code review ACK d0dab89. meshcollider: Code review ACK d0dab89 Tree-SHA512: c3f52df20fd9d6b3b5aa65562cf5f7dce7b7f44c148b0f988f8b578fce2a28e9b7bf010f5f04bb5bf60f5272b2899f1dbbfb8aee81579c21c9cba559d1d2bb70

…ningProvider d0dab89 Refactor: Require scriptPubKey to get wallet SigningProvider (Andrew Chow) 4b0c718 Accumulate result UniValue in SignTransaction (Andrew Chow) Pull request description: Easier to review ignoring whitespace: git log -p -n1 -w This commit does not change behavior. It passes new CScript arguments to signing functions, but the arguments aren't currently used. Split from bitcoin#17261 ACKs for top commit: instagibbs: utACK bitcoin@d0dab89 ryanofsky: Code review ACK d0dab89. Thanks for the SignTransaction update. No other changes since last review Sjors: Code review ACK d0dab89 promag: Code review ACK d0dab89. meshcollider: Code review ACK d0dab89 Tree-SHA512: c3f52df20fd9d6b3b5aa65562cf5f7dce7b7f44c148b0f988f8b578fce2a28e9b7bf010f5f04bb5bf60f5272b2899f1dbbfb8aee81579c21c9cba559d1d2bb70

… within bounds f5fb7fc psbt: check output index is within bounds before accessing (Andrew Chow) 1cf77a2 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow) Pull request description: Backport of #17156, non-trivial due to crossing the refactor in #17371 ACKs for top commit: laanwj: ACK f5fb7fc Tree-SHA512: 7aabf9a6b8a8e287a26dfbf73a437a3bc55177bef8fc5149d822ef81b8ef2458e1d718c3a19c73532c5cef0f9bd8144574c7fad90ca89f13a08b44edf3a2656d

Summary: SignTransaction will be called multiple times in the future. Pass it a result UniValue so that it can accumulate the results of multiple SignTransaction passes. --- bitcoin/bitcoin@4b0c718 Partial backport of Core [[bitcoin/bitcoin#17371 | PR17371]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7686

…ningProvider Summary: Easier to review ignoring whitespace: git log -p -n1 -w This commit does not change behavior. It passes new CScript arguments to signing functions, but the arguments aren't currently used. --- bitcoin/bitcoin@d0dab89 Depends on D7686 Concludes backport of Core [[bitcoin/bitcoin#17371 | PR17371]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7687

…ningProvider d0dab89 Refactor: Require scriptPubKey to get wallet SigningProvider (Andrew Chow) 4b0c718 Accumulate result UniValue in SignTransaction (Andrew Chow) Pull request description: Easier to review ignoring whitespace: git log -p -n1 -w This commit does not change behavior. It passes new CScript arguments to signing functions, but the arguments aren't currently used. Split from bitcoin#17261 ACKs for top commit: instagibbs: utACK bitcoin@d0dab89 ryanofsky: Code review ACK d0dab89. Thanks for the SignTransaction update. No other changes since last review Sjors: Code review ACK d0dab89 promag: Code review ACK d0dab89. meshcollider: Code review ACK d0dab89 Tree-SHA512: c3f52df20fd9d6b3b5aa65562cf5f7dce7b7f44c148b0f988f8b578fce2a28e9b7bf010f5f04bb5bf60f5272b2899f1dbbfb8aee81579c21c9cba559d1d2bb70

…nts are within bounds f5fb7fc psbt: check output index is within bounds before accessing (Andrew Chow) 1cf77a2 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow) Pull request description: Backport of bitcoin#17156, non-trivial due to crossing the refactor in bitcoin#17371 ACKs for top commit: laanwj: ACK f5fb7fc Tree-SHA512: 7aabf9a6b8a8e287a26dfbf73a437a3bc55177bef8fc5149d822ef81b8ef2458e1d718c3a19c73532c5cef0f9bd8144574c7fad90ca89f13a08b44edf3a2656d

…and amounts are within bounds f5fb7fc psbt: check output index is within bounds before accessing (Andrew Chow) 1cf77a2 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow) Pull request description: Backport of bitcoin#17156, non-trivial due to crossing the refactor in bitcoin#17371 ACKs for top commit: laanwj: ACK f5fb7fc Tree-SHA512: 7aabf9a6b8a8e287a26dfbf73a437a3bc55177bef8fc5149d822ef81b8ef2458e1d718c3a19c73532c5cef0f9bd8144574c7fad90ca89f13a08b44edf3a2656d

fanquake added Refactoring Wallet labels Nov 4, 2019

achow101 mentioned this pull request Nov 4, 2019

Make ScriptPubKeyMan an actual interface and the wallet to have multiple #17261

Merged

Sjors reviewed Nov 5, 2019

View reviewed changes

src/wallet/psbtwallet.cpp Show resolved Hide resolved

src/wallet/psbtwallet.cpp Show resolved Hide resolved

src/wallet/psbtwallet.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the wallet-box-pr-5 branch from fd8ab05 to f7510c1 Compare November 5, 2019 19:23

ryanofsky approved these changes Nov 18, 2019

View reviewed changes

Accumulate result UniValue in SignTransaction

4b0c718

SignTransaction will be called multiple times in the future. Pass it a result UniValue so that it can accumulate the results of multiple SignTransaction passes.

achow101 force-pushed the wallet-box-pr-5 branch from f7510c1 to b2e509c Compare November 18, 2019 20:41

Refactor: Require scriptPubKey to get wallet SigningProvider

d0dab89

Easier to review ignoring whitespace: git log -p -n1 -w This commit does not change behavior. It passes new CScript arguments to signing functions, but the arguments aren't currently used.

achow101 force-pushed the wallet-box-pr-5 branch from b2e509c to d0dab89 Compare November 18, 2019 20:42

ryanofsky approved these changes Nov 18, 2019

View reviewed changes

Sjors approved these changes Nov 19, 2019

View reviewed changes

promag reviewed Nov 21, 2019

View reviewed changes

instagibbs reviewed Nov 22, 2019

View reviewed changes

meshcollider reviewed Nov 22, 2019

View reviewed changes

meshcollider merged commit d0dab89 into bitcoin:master Nov 22, 2019

This was referenced Feb 5, 2020

[0.19] psbt: check that various indexes and amounts are within bounds #18078

Closed

[0.19] psbt: check that various indexes and amounts are within bounds #18079

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Refactor: Require scriptPubKey to get wallet SigningProvider #17371

Refactor: Require scriptPubKey to get wallet SigningProvider #17371

Uh oh!

Conversation

achow101 commented Nov 4, 2019

Uh oh!

DrahtBot commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky 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!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Nov 21, 2019

Uh oh!

promag 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!

instagibbs Nov 22, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Nov 22, 2019

Uh oh!

instagibbs commented Nov 22, 2019

Uh oh!

meshcollider 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!

Uh oh!

DrahtBot commented Nov 4, 2019 •

edited

Loading

instagibbs Nov 22, 2019 •

edited

Loading