Refactor: Require scriptPubKey to get wallet SigningProvider

achow101 commented at 10:03 pm on November 4, 2019: member

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

fanquake added the label Refactoring on Nov 4, 2019

fanquake added the label Wallet on Nov 4, 2019

DrahtBot commented at 11:27 pm on November 4, 2019: member

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.

in src/wallet/psbtwallet.cpp:58 in fd8ab05148 outdated

52+        }
53+        SignatureData sigdata;
54+        input.FillSignatureData(sigdata);
55+        const SigningProvider* provider = pwallet->GetSigningProvider(script, sigdata);
56+        if (!provider) {
57+            continue;

Sjors commented at 2:15 pm on November 5, 2019:

Adding complete = false would seem more consistent with previous behavior which sets complete to false if it can’t sign one of the inputs.

achow101 commented at 7:23 pm on November 5, 2019:

Done

in src/wallet/psbtwallet.cpp:49 in fd8ab05148 outdated

45+        if (input.non_witness_utxo) {
46+            script = input.non_witness_utxo->vout[txin.prevout.n].scriptPubKey;
47+        } else if (!input.witness_utxo.IsNull()) {
48+            script = input.witness_utxo.scriptPubKey;
49+        } else {
50+            // There's no UTXO so we can just skip this now

Sjors commented at 2:21 pm on November 5, 2019:

Add complete = false?

achow101 commented at 7:23 pm on November 5, 2019:

Done

in src/wallet/psbtwallet.cpp:45 in fd8ab05148 outdated

38@@ -39,12 +39,33 @@ TransactionError FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& ps
39             return TransactionError::SIGHASH_MISMATCH;
40         }
41 
42-        complete &= SignPSBTInput(HidingSigningProvider(pwallet->GetSigningProvider(), !sign, !bip32derivs), psbtx, i, sighash_type);
43+        // Get the scriptPubKey to know which SigningProvider to use
44+        CScript script;
45+        if (input.non_witness_utxo) {
46+            script = input.non_witness_utxo->vout[txin.prevout.n].scriptPubKey;

Sjors commented at 2:21 pm on November 5, 2019:

Check for the witness UTXO first? (given the above The signing code will switch to the smaller witness_utxo if this is ok.). It seems unlikely they’d have different signing providers, but still.

achow101 commented at 7:23 pm on November 5, 2019:

Done

in src/wallet/rpcwallet.cpp:3307 in fd8ab05148 outdated

3303+        providers.insert(&DUMMY_SIGNING_PROVIDER);
3304+    }
3305+
3306+    UniValue result;
3307+    for (const SigningProvider* provider : providers) {
3308+        result = SignTransaction(mtx, provider, coins, request.params[2]);

Sjors commented at 2:37 pm on November 5, 2019:

This seems brittle. I’m assuming the goal, if multiple signing providers match, is to have them all sign (part of) the transaction? What if the first signer succeeds, but the second returns an error object because it didn’t have the key (in which it shouldn’t have matched, but still…)?

achow101 commented at 7:23 pm on November 5, 2019:

There shouldn’t ever be a SigningProvider that doesn’t match.

ryanofsky commented at 7:29 pm on November 18, 2019:

In commit “Refactor: Require scriptPubKey to get wallet SigningProvider” (f7510c144b2262e1ee114eaa740b889c24ec7abb)

According to RPC documentation result has three fields: “hex” “complete” and “errors”. I guess for “hex” and “complete” fields it makes sense to take the last values, but errors field should probably accumulate? In any case, this code is not very future proof in case new fields are added. Would suggest changing SignTransaction to take a UniValue& result input/output parameter, so it can be responsible for updating the result at each step, and this code isn’t just blindly overwriting previous results.

achow101 commented at 8:43 pm on November 18, 2019:

I’ve added a commit which changes SignTransaction to void and has a result parameter to accumulate the errors array.

ryanofsky added this to the "PRs" column in a project

in src/wallet/rpcwallet.cpp:553 in fd8ab05148 outdated

549@@ -550,7 +550,11 @@ static UniValue signmessage(const JSONRPCRequest& request)
550         throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to key");
551     }
552 
553-    const SigningProvider* provider = pwallet->GetSigningProvider();
554+    CScript script_pub_key = GetScriptForDestination(*pkhash);

ryanofsky commented at 5:18 pm on November 5, 2019:

In commit “Refactor: Require scriptPubKey to get wallet SigningProvider” (f7510c144b2262e1ee114eaa740b889c24ec7abb)

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

achow101 commented at 7:49 pm on November 18, 2019:

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

Sjors commented at 11:45 am on November 19, 2019:

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

achow101 force-pushed on Nov 5, 2019

in src/wallet/rpcwallet.cpp:3773 in f7510c144b outdated

3770+    const SigningProvider* provider = pwallet->GetSigningProvider(scriptPubKey);
3771 
3772     isminetype mine = pwallet->IsMine(dest);
3773     ret.pushKV("ismine", bool(mine & ISMINE_SPENDABLE));
3774-    bool solvable = IsSolvable(*provider, scriptPubKey);
3775+    bool solvable = provider ? IsSolvable(*provider, scriptPubKey) : false;

ryanofsky commented at 7:32 pm on November 18, 2019:

In commit “Refactor: Require scriptPubKey to get wallet SigningProvider” (f7510c144b2262e1ee114eaa740b889c24ec7abb)

Would be slightly simpler to replace ternary with solvable = provider && IsSolvable(*provider)

achow101 commented at 8:42 pm on November 18, 2019:

Done

ryanofsky approved

ryanofsky commented at 7:38 pm on November 18, 2019: member

Code review ACK f7510c144b2262e1ee114eaa740b889c24ec7abb. Pretty straightforward to verify this doesn’t change behavior.

Accumulate result UniValue in SignTransaction

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.

4b0c718f8f

achow101 force-pushed on Nov 18, 2019

Refactor: Require scriptPubKey to get wallet SigningProvider

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.

d0dab897af

achow101 force-pushed on Nov 18, 2019

ryanofsky approved

ryanofsky commented at 9:10 pm on November 18, 2019: member

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

Sjors approved

Sjors commented at 11:50 am on November 19, 2019: member

Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a

ryanofsky commented at 3:27 pm on November 21, 2019: member

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

in src/rpc/rawtransaction_util.h:26 in 4b0c718f8f outdated

20@@ -21,9 +21,9 @@ class SigningProvider;
21  * @param  keystore      Temporary keystore containing signing keys
22  * @param  coins         Map of unspent outputs
23  * @param  hashType      The signature hash type
24- * @returns JSON object with details of signed transaction
25+ * @param result         JSON object where signed transaction results accumulate
26  */
27-UniValue SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, const UniValue& hashType);
28+void SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, const UniValue& hashType, UniValue& result);

promag commented at 3:52 pm on November 21, 2019:

4b0c718f8f48c678cbe4575e9a9cf9e62a30f0da

An overload could be added, like

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

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

promag commented at 4:07 pm on November 21, 2019: member

Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a.

in src/interfaces/wallet.cpp:120 in d0dab897af

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

instagibbs commented at 4:36 pm on November 22, 2019:

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

in src/wallet/wallet.cpp:2103 in d0dab897af

2099@@ -2096,7 +2100,7 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector<
2100                 continue;
2101             }
2102 
2103-            const SigningProvider* provider = GetSigningProvider();
2104+            const SigningProvider* provider = GetSigningProvider(wtx.tx->vout[i].scriptPubKey);

instagibbs commented at 4:38 pm on November 22, 2019:

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

instagibbs commented at 4:42 pm on November 22, 2019:

oh i cannot read.

in src/wallet/rpcwallet.cpp:3300 in d0dab897af

3295+        const SigningProvider* provider = pwallet->GetSigningProvider(coin_pair.second.out.scriptPubKey);
3296+        if (provider) {
3297+            providers.insert(std::move(provider));
3298+        }
3299+    }
3300+    if (providers.size() == 0) {

instagibbs commented at 4:39 pm on November 22, 2019:

non-blocking-nit: .empty()

achow101 commented at 4:58 pm on November 22, 2019:

Not fixing to avoid invalidating ACKs.

instagibbs commented at 4:43 pm on November 22, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/17371/commits/d0dab897afaac0a18aa47d3ce673a4a43a69178a

instagibbs commented at 5:13 pm on November 22, 2019: member

ready for merge? @meshcollider

in src/interfaces/wallet.cpp:124 in d0dab897af

121-    bool getPrivKey(const CKeyID& address, CKey& key) override { return m_wallet->GetLegacyScriptPubKeyMan()->GetKey(address, key); }
122+    bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) override
123+    {
124+        const SigningProvider* provider = m_wallet->GetSigningProvider(script);
125+        if (provider) {
126+            return provider->GetPubKey(address, pub_key);

meshcollider commented at 7:18 pm on November 22, 2019:

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?

in src/wallet/wallet.cpp:1347 in d0dab897af

1341@@ -1342,7 +1342,11 @@ bool CWallet::DummySignInput(CTxIn &tx_in, const CTxOut &txout, bool use_max_sig
1342     const CScript& scriptPubKey = txout.scriptPubKey;
1343     SignatureData sigdata;
1344 
1345-    const SigningProvider* provider = GetSigningProvider();
1346+    const SigningProvider* provider = GetSigningProvider(scriptPubKey);
1347+    if (!provider) {
1348+        // We don't know about this scriptpbuKey;

meshcollider commented at 7:29 pm on November 22, 2019:

nit: typo (same below)

in src/wallet/rpcwallet.cpp:2980 in d0dab897af

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

meshcollider commented at 7:32 pm on November 22, 2019:

This can probably be combined with above to simplify

meshcollider commented at 7:32 pm on November 22, 2019: contributor

Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a

meshcollider referenced this in commit 8aac85d71e on Nov 22, 2019

meshcollider merged this on Nov 22, 2019

meshcollider closed this on Nov 22, 2019

sidhujag referenced this in commit fa0c6c83dd on Nov 22, 2019

ryanofsky moved this from the "PRs" to the "Done" column in a project

laanwj referenced this in commit ba0b7e1296 on Feb 10, 2020

deadalnix referenced this in commit 7fd581bc77 on Sep 30, 2020

deadalnix referenced this in commit 149452d5ee on Sep 30, 2020

sidhujag referenced this in commit d50b1ba76f on Nov 10, 2020

Munkybooty referenced this in commit 6696b971e2 on Dec 9, 2021

Munkybooty referenced this in commit 5c1e04fb24 on Dec 9, 2021

Munkybooty referenced this in commit 86284f11f4 on Dec 9, 2021

Munkybooty referenced this in commit b66b23919d on Dec 9, 2021

Munkybooty referenced this in commit 2688da5330 on Dec 9, 2021

Munkybooty referenced this in commit 1ad87663be on Dec 9, 2021

MarcoFalke locked this on Dec 16, 2021

Refactor: Require scriptPubKey to get wallet SigningProvider #17371

Conflicts