Skip to content

Commit 8aac85d

Browse files
meshcollidermeshcollider
committed
Merge #17371: Refactor: Require scriptPubKey to get wallet SigningProvider
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 bitcoin/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
2 parents cef87f7 + d0dab89 commit 8aac85d

File tree

11 files changed

+137
-57
lines changed

11 files changed

+137
-57
lines changed

‎src/interfaces/wallet.cpp

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,22 @@ class WalletImpl : public Wallet
116116
std::string error;
117117
return m_wallet->GetNewDestination(type, label, dest, error);
118118
}
119-
bool getPubKey(const CKeyID& address, CPubKey& pub_key) override { return m_wallet->GetLegacyScriptPubKeyMan()->GetPubKey(address, pub_key); }
120-
bool getPrivKey(const CKeyID& address, CKey& key) override { return m_wallet->GetLegacyScriptPubKeyMan()->GetKey(address, key); }
119+
bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) override
120+
{
121+
const SigningProvider* provider = m_wallet->GetSigningProvider(script);
122+
if (provider) {
123+
return provider->GetPubKey(address, pub_key);
124+
}
125+
return false;
126+
}
127+
bool getPrivKey(const CScript& script, const CKeyID& address, CKey& key) override
128+
{
129+
const SigningProvider* provider = m_wallet->GetSigningProvider(script);
130+
if (provider) {
131+
return provider->GetKey(address, key);
132+
}
133+
return false;
134+
}
121135
bool isSpendable(const CTxDestination& dest) override { return m_wallet->IsMine(dest) & ISMINE_SPENDABLE; }
122136
bool haveWatchOnly() override
123137
{

‎src/interfaces/wallet.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,10 +81,10 @@ class Wallet
8181
virtual bool getNewDestination(const OutputType type, const std::string label, CTxDestination& dest) = 0;
8282

8383
//! Get public key.
84-
virtual bool getPubKey(const CKeyID& address, CPubKey& pub_key) = 0;
84+
virtual bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) = 0;
8585

8686
//! Get private key.
87-
virtual bool getPrivKey(const CKeyID& address, CKey& key) = 0;
87+
virtual bool getPrivKey(const CScript& script, const CKeyID& address, CKey& key) = 0;
8888

8989
//! Return whether wallet has private key.
9090
virtual bool isSpendable(const CTxDestination& dest) = 0;

‎src/qt/coincontroldialog.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -468,7 +468,7 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog)
468468
{
469469
CPubKey pubkey;
470470
PKHash *pkhash = boost::get<PKHash>(&address);
471-
if (pkhash && model->wallet().getPubKey(CKeyID(*pkhash), pubkey))
471+
if (pkhash && model->wallet().getPubKey(out.txout.scriptPubKey, CKeyID(*pkhash), pubkey))
472472
{
473473
nBytesInputs += (pubkey.IsCompressed() ? 148 : 180);
474474
}

‎src/qt/signverifymessagedialog.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ void SignVerifyMessageDialog::on_signMessageButton_SM_clicked()
136136
}
137137

138138
CKey key;
139-
if (!model->wallet().getPrivKey(CKeyID(*pkhash), key))
139+
if (!model->wallet().getPrivKey(GetScriptForDestination(destination), CKeyID(*pkhash), key))
140140
{
141141
ui->statusLabel_SM->setStyleSheet("QLabel { color: red; }");
142142
ui->statusLabel_SM->setText(tr("Private key for the entered address is not available."));

‎src/rpc/rawtransaction.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -763,7 +763,9 @@ static UniValue signrawtransactionwithkey(const JSONRPCRequest& request)
763763
// Parse the prevtxs array
764764
ParsePrevouts(request.params[2], &keystore, coins);
765765

766-
return SignTransaction(mtx, &keystore, coins, request.params[3]);
766+
UniValue result(UniValue::VOBJ);
767+
SignTransaction(mtx, &keystore, coins, request.params[3], result);
768+
return result;
767769
}
768770

769771
static UniValue sendrawtransaction(const JSONRPCRequest& request)

‎src/rpc/rawtransaction_util.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keyst
268268
}
269269
}
270270

271-
UniValue SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, const UniValue& hashType)
271+
void SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, const UniValue& hashType, UniValue& result)
272272
{
273273
int nHashType = ParseSighashString(hashType);
274274

@@ -319,12 +319,12 @@ UniValue SignTransaction(CMutableTransaction& mtx, const SigningProvider* keysto
319319
}
320320
bool fComplete = vErrors.empty();
321321

322-
UniValue result(UniValue::VOBJ);
323322
result.pushKV("hex", EncodeHexTx(CTransaction(mtx)));
324323
result.pushKV("complete", fComplete);
325324
if (!vErrors.empty()) {
325+
if (result.exists("errors")) {
326+
vErrors.push_backV(result["errors"].getValues());
327+
}
326328
result.pushKV("errors", vErrors);
327329
}
328-
329-
return result;
330330
}

‎src/rpc/rawtransaction_util.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@ class SigningProvider;
2121
* @param keystore Temporary keystore containing signing keys
2222
* @param coins Map of unspent outputs
2323
* @param hashType The signature hash type
24-
* @returns JSON object with details of signed transaction
24+
* @param result JSON object where signed transaction results accumulate
2525
*/
26-
UniValue SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, const UniValue& hashType);
26+
void SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, const std::map<COutPoint, Coin>& coins, const UniValue& hashType, UniValue& result);
2727

2828
/**
2929
* Parse a prevtxs UniValue array and get the map of coins from it

‎src/wallet/psbtwallet.cpp

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,35 @@ TransactionError FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& ps
3939
return TransactionError::SIGHASH_MISMATCH;
4040
}
4141

42-
complete &= SignPSBTInput(HidingSigningProvider(pwallet->GetSigningProvider(), !sign, !bip32derivs), psbtx, i, sighash_type);
42+
// Get the scriptPubKey to know which SigningProvider to use
43+
CScript script;
44+
if (!input.witness_utxo.IsNull()) {
45+
script = input.witness_utxo.scriptPubKey;
46+
} else if (input.non_witness_utxo) {
47+
script = input.non_witness_utxo->vout[txin.prevout.n].scriptPubKey;
48+
} else {
49+
// There's no UTXO so we can just skip this now
50+
complete = false;
51+
continue;
52+
}
53+
SignatureData sigdata;
54+
input.FillSignatureData(sigdata);
55+
const SigningProvider* provider = pwallet->GetSigningProvider(script, sigdata);
56+
if (!provider) {
57+
complete = false;
58+
continue;
59+
}
60+
61+
complete &= SignPSBTInput(HidingSigningProvider(provider, !sign, !bip32derivs), psbtx, i, sighash_type);
4362
}
4463

4564
// Fill in the bip32 keypaths and redeemscripts for the outputs so that hardware wallets can identify change
4665
for (unsigned int i = 0; i < psbtx.tx->vout.size(); ++i) {
47-
UpdatePSBTOutput(HidingSigningProvider(pwallet->GetSigningProvider(), true, !bip32derivs), psbtx, i);
66+
const CTxOut& out = psbtx.tx->vout.at(i);
67+
const SigningProvider* provider = pwallet->GetSigningProvider(out.scriptPubKey);
68+
if (provider) {
69+
UpdatePSBTOutput(HidingSigningProvider(provider, true, !bip32derivs), psbtx, i);
70+
}
4871
}
4972

5073
return TransactionError::OK;

‎src/wallet/rpcwallet.cpp

Lines changed: 60 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -560,7 +560,11 @@ static UniValue signmessage(const JSONRPCRequest& request)
560560
throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to key");
561561
}
562562

563-
const SigningProvider* provider = pwallet->GetSigningProvider();
563+
CScript script_pub_key = GetScriptForDestination(*pkhash);
564+
const SigningProvider* provider = pwallet->GetSigningProvider(script_pub_key);
565+
if (!provider) {
566+
throw JSONRPCError(RPC_WALLET_ERROR, "Private key not available");
567+
}
564568

565569
CKey key;
566570
CKeyID keyID(*pkhash);
@@ -2940,34 +2944,36 @@ static UniValue listunspent(const JSONRPCRequest& request)
29402944
entry.pushKV("label", i->second.name);
29412945
}
29422946

2943-
const SigningProvider* provider = pwallet->GetSigningProvider();
2944-
if (scriptPubKey.IsPayToScriptHash()) {
2945-
const CScriptID& hash = CScriptID(boost::get<ScriptHash>(address));
2946-
CScript redeemScript;
2947-
if (provider->GetCScript(hash, redeemScript)) {
2948-
entry.pushKV("redeemScript", HexStr(redeemScript.begin(), redeemScript.end()));
2949-
// Now check if the redeemScript is actually a P2WSH script
2950-
CTxDestination witness_destination;
2951-
if (redeemScript.IsPayToWitnessScriptHash()) {
2952-
bool extracted = ExtractDestination(redeemScript, witness_destination);
2953-
CHECK_NONFATAL(extracted);
2954-
// Also return the witness script
2955-
const WitnessV0ScriptHash& whash = boost::get<WitnessV0ScriptHash>(witness_destination);
2956-
CScriptID id;
2957-
CRIPEMD160().Write(whash.begin(), whash.size()).Finalize(id.begin());
2958-
CScript witnessScript;
2959-
if (provider->GetCScript(id, witnessScript)) {
2960-
entry.pushKV("witnessScript", HexStr(witnessScript.begin(), witnessScript.end()));
2947+
const SigningProvider* provider = pwallet->GetSigningProvider(scriptPubKey);
2948+
if (provider) {
2949+
if (scriptPubKey.IsPayToScriptHash()) {
2950+
const CScriptID& hash = CScriptID(boost::get<ScriptHash>(address));
2951+
CScript redeemScript;
2952+
if (provider->GetCScript(hash, redeemScript)) {
2953+
entry.pushKV("redeemScript", HexStr(redeemScript.begin(), redeemScript.end()));
2954+
// Now check if the redeemScript is actually a P2WSH script
2955+
CTxDestination witness_destination;
2956+
if (redeemScript.IsPayToWitnessScriptHash()) {
2957+
bool extracted = ExtractDestination(redeemScript, witness_destination);
2958+
CHECK_NONFATAL(extracted);
2959+
// Also return the witness script
2960+
const WitnessV0ScriptHash& whash = boost::get<WitnessV0ScriptHash>(witness_destination);
2961+
CScriptID id;
2962+
CRIPEMD160().Write(whash.begin(), whash.size()).Finalize(id.begin());
2963+
CScript witnessScript;
2964+
if (provider->GetCScript(id, witnessScript)) {
2965+
entry.pushKV("witnessScript", HexStr(witnessScript.begin(), witnessScript.end()));
2966+
}
29612967
}
29622968
}
2963-
}
2964-
} else if (scriptPubKey.IsPayToWitnessScriptHash()) {
2965-
const WitnessV0ScriptHash& whash = boost::get<WitnessV0ScriptHash>(address);
2966-
CScriptID id;
2967-
CRIPEMD160().Write(whash.begin(), whash.size()).Finalize(id.begin());
2968-
CScript witnessScript;
2969-
if (provider->GetCScript(id, witnessScript)) {
2970-
entry.pushKV("witnessScript", HexStr(witnessScript.begin(), witnessScript.end()));
2969+
} else if (scriptPubKey.IsPayToWitnessScriptHash()) {
2970+
const WitnessV0ScriptHash& whash = boost::get<WitnessV0ScriptHash>(address);
2971+
CScriptID id;
2972+
CRIPEMD160().Write(whash.begin(), whash.size()).Finalize(id.begin());
2973+
CScript witnessScript;
2974+
if (provider->GetCScript(id, witnessScript)) {
2975+
entry.pushKV("witnessScript", HexStr(witnessScript.begin(), witnessScript.end()));
2976+
}
29712977
}
29722978
}
29732979
}
@@ -2978,8 +2984,11 @@ static UniValue listunspent(const JSONRPCRequest& request)
29782984
entry.pushKV("spendable", out.fSpendable);
29792985
entry.pushKV("solvable", out.fSolvable);
29802986
if (out.fSolvable) {
2981-
auto descriptor = InferDescriptor(scriptPubKey, *pwallet->GetLegacyScriptPubKeyMan());
2982-
entry.pushKV("desc", descriptor->ToString());
2987+
const SigningProvider* provider = pwallet->GetSigningProvider(scriptPubKey);
2988+
if (provider) {
2989+
auto descriptor = InferDescriptor(scriptPubKey, *provider);
2990+
entry.pushKV("desc", descriptor->ToString());
2991+
}
29832992
}
29842993
if (avoid_reuse) entry.pushKV("reused", reused);
29852994
entry.pushKV("safe", out.fSafe);
@@ -3288,7 +3297,23 @@ UniValue signrawtransactionwithwallet(const JSONRPCRequest& request)
32883297
// Parse the prevtxs array
32893298
ParsePrevouts(request.params[1], nullptr, coins);
32903299

3291-
return SignTransaction(mtx, &*pwallet->GetLegacyScriptPubKeyMan(), coins, request.params[2]);
3300+
std::set<const SigningProvider*> providers;
3301+
for (const std::pair<COutPoint, Coin> coin_pair : coins) {
3302+
const SigningProvider* provider = pwallet->GetSigningProvider(coin_pair.second.out.scriptPubKey);
3303+
if (provider) {
3304+
providers.insert(std::move(provider));
3305+
}
3306+
}
3307+
if (providers.size() == 0) {
3308+
// When there are no available providers, use DUMMY_SIGNING_PROVIDER so we can check if the tx is complete
3309+
providers.insert(&DUMMY_SIGNING_PROVIDER);
3310+
}
3311+
3312+
UniValue result(UniValue::VOBJ);
3313+
for (const SigningProvider* provider : providers) {
3314+
SignTransaction(mtx, provider, coins, request.params[2], result);
3315+
}
3316+
return result;
32923317
}
32933318

32943319
static UniValue bumpfee(const JSONRPCRequest& request)
@@ -3653,9 +3678,10 @@ static UniValue DescribeWalletAddress(CWallet* pwallet, const CTxDestination& de
36533678
{
36543679
UniValue ret(UniValue::VOBJ);
36553680
UniValue detail = DescribeAddress(dest);
3681+
CScript script = GetScriptForDestination(dest);
36563682
const SigningProvider* provider = nullptr;
36573683
if (pwallet) {
3658-
provider = pwallet->GetSigningProvider();
3684+
provider = pwallet->GetSigningProvider(script);
36593685
}
36603686
ret.pushKVs(detail);
36613687
ret.pushKVs(boost::apply_visitor(DescribeWalletAddressVisitor(provider), dest));
@@ -3747,11 +3773,11 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
37473773

37483774
CScript scriptPubKey = GetScriptForDestination(dest);
37493775
ret.pushKV("scriptPubKey", HexStr(scriptPubKey.begin(), scriptPubKey.end()));
3750-
const SigningProvider* provider = pwallet->GetSigningProvider();
3776+
const SigningProvider* provider = pwallet->GetSigningProvider(scriptPubKey);
37513777

37523778
isminetype mine = pwallet->IsMine(dest);
37533779
ret.pushKV("ismine", bool(mine & ISMINE_SPENDABLE));
3754-
bool solvable = IsSolvable(*provider, scriptPubKey);
3780+
bool solvable = provider && IsSolvable(*provider, scriptPubKey);
37553781
ret.pushKV("solvable", solvable);
37563782
if (solvable) {
37573783
ret.pushKV("desc", InferDescriptor(scriptPubKey, *provider)->ToString());
@@ -3764,7 +3790,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
37643790
}
37653791
ret.pushKV("ischange", pwallet->IsChange(scriptPubKey));
37663792

3767-
ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan();
3793+
ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan(scriptPubKey);
37683794
if (spk_man) {
37693795
if (const CKeyMetadata* meta = spk_man->GetMetadata(dest)) {
37703796
ret.pushKV("timestamp", meta->nCreateTime);

‎src/wallet/wallet.cpp

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1363,7 +1363,11 @@ bool CWallet::DummySignInput(CTxIn &tx_in, const CTxOut &txout, bool use_max_sig
13631363
const CScript& scriptPubKey = txout.scriptPubKey;
13641364
SignatureData sigdata;
13651365

1366-
const SigningProvider* provider = GetSigningProvider();
1366+
const SigningProvider* provider = GetSigningProvider(scriptPubKey);
1367+
if (!provider) {
1368+
// We don't know about this scriptpbuKey;
1369+
return false;
1370+
}
13671371

13681372
if (!ProduceSignature(*provider, use_max_sig ? DUMMY_MAXIMUM_SIGNATURE_CREATOR : DUMMY_SIGNATURE_CREATOR, scriptPubKey, sigdata)) {
13691373
return false;
@@ -2125,7 +2129,7 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector<
21252129
continue;
21262130
}
21272131

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

21302134
bool solvable = provider ? IsSolvable(*provider, wtx.tx->vout[i].scriptPubKey) : false;
21312135
bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable));
@@ -2378,8 +2382,9 @@ bool CWallet::SignTransaction(CMutableTransaction& tx)
23782382
const CAmount& amount = mi->second.tx->vout[input.prevout.n].nValue;
23792383
SignatureData sigdata;
23802384

2381-
const SigningProvider* provider = GetSigningProvider();
2385+
const SigningProvider* provider = GetSigningProvider(scriptPubKey);
23822386
if (!provider) {
2387+
// We don't know about this scriptpbuKey;
23832388
return false;
23842389
}
23852390

@@ -2846,7 +2851,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
28462851
const CScript& scriptPubKey = coin.txout.scriptPubKey;
28472852
SignatureData sigdata;
28482853

2849-
const SigningProvider* provider = GetSigningProvider();
2854+
const SigningProvider* provider = GetSigningProvider(scriptPubKey);
28502855
if (!provider || !ProduceSignature(*provider, MutableTransactionSignatureCreator(&txNew, nIn, coin.txout.nValue, SIGHASH_ALL), scriptPubKey, sigdata))
28512856
{
28522857
strFailReason = _("Signing transaction failed").translated;
@@ -4043,12 +4048,17 @@ bool CWallet::Lock()
40434048
return true;
40444049
}
40454050

4046-
ScriptPubKeyMan* CWallet::GetScriptPubKeyMan() const
4051+
ScriptPubKeyMan* CWallet::GetScriptPubKeyMan(const CScript& script) const
4052+
{
4053+
return m_spk_man.get();
4054+
}
4055+
4056+
const SigningProvider* CWallet::GetSigningProvider(const CScript& script) const
40474057
{
40484058
return m_spk_man.get();
40494059
}
40504060

4051-
const SigningProvider* CWallet::GetSigningProvider() const
4061+
const SigningProvider* CWallet::GetSigningProvider(const CScript& script, SignatureData& sigdata) const
40524062
{
40534063
return m_spk_man.get();
40544064
}

0 commit comments

Comments
 (0)