Skip to content

Commit 005f8a9

Browse files
ryanofskyryanofsky
committed
wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition
Make LegacyScriptPubKeyMan::CanProvide method able to recognize p2sh scripts when the redeem script is present in the mapScripts map without the p2sh script also having to be added to the mapScripts map. This restores behavior prior to bitcoin/bitcoin#17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before #17261 as solvable. The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files. This change adds a lot of comments and allows reverting commit 4a7e43e "Store p2sh scripts in AddAndGetDestinationForScript", so the AddAndGetDestinationForScript() function, CanProvide() method, and mapScripts map should all be more comprehensible
1 parent 2bdc476 commit 005f8a9

File tree

4 files changed

+107
-9
lines changed

4 files changed

+107
-9
lines changed

‎src/Makefile.test.include

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,8 @@ BITCOIN_TESTS += \
209209
wallet/test/wallet_crypto_tests.cpp \
210210
wallet/test/coinselector_tests.cpp \
211211
wallet/test/init_tests.cpp \
212-
wallet/test/ismine_tests.cpp
212+
wallet/test/ismine_tests.cpp \
213+
wallet/test/scriptpubkeyman_tests.cpp
213214

214215
BITCOIN_TEST_SUITE += \
215216
wallet/test/wallet_test_fixture.cpp \

‎src/script/signingprovider.h

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,53 @@ class FillableSigningProvider : public SigningProvider
6666
using KeyMap = std::map<CKeyID, CKey>;
6767
using ScriptMap = std::map<CScriptID, CScript>;
6868

69+
/**
70+
* Map of key id to unencrypted private keys known by the signing provider.
71+
* Map may be empty if the provider has another source of keys, like an
72+
* encrypted store.
73+
*/
6974
KeyMap mapKeys GUARDED_BY(cs_KeyStore);
75+
76+
/**
77+
* Map of script id to scripts known by the signing provider.
78+
*
79+
* This map originally just held P2SH redeemScripts, and was used by wallet
80+
* code to look up script ids referenced in "OP_HASH160 <script id>
81+
* OP_EQUAL" P2SH outputs. Later in 605e8473a7d it was extended to hold
82+
* P2WSH witnessScripts as well, and used to look up nested scripts
83+
* referenced in "OP_0 <script hash>" P2WSH outputs. Later in commits
84+
* f4691ab3a9d and 248f3a76a82, it was extended once again to hold segwit
85+
* "OP_0 <key or script hash>" scriptPubKeys, in order to give the wallet a
86+
* way to distinguish between segwit outputs that it generated addresses for
87+
* and wanted to receive payments from, and segwit outputs that it never
88+
* generated addresses for, but it could spend just because of having keys.
89+
* (Before segwit activation it was also important to not treat segwit
90+
* outputs to arbitrary wallet keys as payments, because these could be
91+
* spent by anyone without even needing to sign with the keys.)
92+
*
93+
* Some of the scripts stored in mapScripts are memory-only and
94+
* intentionally not saved to disk. Specifically, scripts added by
95+
* ImplicitlyLearnRelatedKeyScripts(pubkey) calls are not written to disk so
96+
* future wallet code can have flexibility to be more selective about what
97+
* transaction outputs it recognizes as payments, instead of having to treat
98+
* all outputs spending to keys it knows as payments. By contrast,
99+
* mapScripts entries added by AddCScript(script),
100+
* LearnRelatedScripts(pubkey, type), and LearnAllRelatedScripts(pubkey)
101+
* calls are saved because they are all intentionally used to receive
102+
* payments.
103+
*
104+
* The FillableSigningProvider::mapScripts script map should not be confused
105+
* with LegacyScriptPubKeyMan::setWatchOnly script set. The two collections
106+
* can hold the same scripts, but they serve different purposes. The
107+
* setWatchOnly script set is intended to expand the set of outputs the
108+
* wallet considers payments. Every output with a script it contains is
109+
* considered to belong to the wallet, regardless of whether the script is
110+
* solvable or signable. By contrast, the scripts in mapScripts are only
111+
* used for solving, and to restrict which outputs are considered payments
112+
* by the wallet. An output with a script in mapScripts, unlike
113+
* setWatchOnly, is not automatically considered to belong to the wallet if
114+
* it can't be solved and signed for.
115+
*/
70116
ScriptMap mapScripts GUARDED_BY(cs_KeyStore);
71117

72118
void ImplicitlyLearnRelatedKeyScripts(const CPubKey& pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore);

‎src/wallet/scriptpubkeyman.cpp

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,15 @@ bool HaveKeys(const std::vector<valtype>& pubkeys, const LegacyScriptPubKeyMan&
7070
return true;
7171
}
7272

73-
IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion)
73+
//! Recursively solve script and return spendable/watchonly/invalid status.
74+
//!
75+
//! @param keystore legacy key and script store
76+
//! @param script script to solve
77+
//! @param sigversion script type (top-level / redeemscript / witnessscript)
78+
//! @param recurse_scripthash whether to recurse into nested p2sh and p2wsh
79+
//! scripts or simply treat any script that has been
80+
//! stored in the keystore as spendable
81+
IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion, bool recurse_scripthash=true)
7482
{
7583
IsMineResult ret = IsMineResult::NO;
7684

@@ -129,7 +137,7 @@ IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& s
129137
CScriptID scriptID = CScriptID(uint160(vSolutions[0]));
130138
CScript subscript;
131139
if (keystore.GetCScript(scriptID, subscript)) {
132-
ret = std::max(ret, IsMineInner(keystore, subscript, IsMineSigVersion::P2SH));
140+
ret = std::max(ret, recurse_scripthash ? IsMineInner(keystore, subscript, IsMineSigVersion::P2SH) : IsMineResult::SPENDABLE);
133141
}
134142
break;
135143
}
@@ -147,7 +155,7 @@ IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& s
147155
CScriptID scriptID = CScriptID(hash);
148156
CScript subscript;
149157
if (keystore.GetCScript(scriptID, subscript)) {
150-
ret = std::max(ret, IsMineInner(keystore, subscript, IsMineSigVersion::WITNESS_V0));
158+
ret = std::max(ret, recurse_scripthash ? IsMineInner(keystore, subscript, IsMineSigVersion::WITNESS_V0) : IsMineResult::SPENDABLE);
151159
}
152160
break;
153161
}
@@ -476,11 +484,11 @@ std::unique_ptr<SigningProvider> LegacyScriptPubKeyMan::GetSigningProvider(const
476484

477485
bool LegacyScriptPubKeyMan::CanProvide(const CScript& script, SignatureData& sigdata)
478486
{
479-
if (IsMine(script) != ISMINE_NO) {
480-
// If it IsMine, we can always provide in some way
481-
return true;
482-
} else if (HaveCScript(CScriptID(script))) {
483-
// We can still provide some stuff if we have the script, but IsMine failed because we don't have keys
487+
IsMineResult ismine = IsMineInner(*this, script, IsMineSigVersion::TOP, /* recurse_scripthash= */ false);
488+
if (ismine == IsMineResult::SPENDABLE || ismine == IsMineResult::WATCH_ONLY) {
489+
// If ismine, it means we recognize keys or script ids in the script, or
490+
// are watching the script itself, and we can at least provide metadata
491+
// or solving information, even if not able to sign fully.
484492
return true;
485493
} else {
486494
// If, given the stuff in sigdata, we could make a valid sigature, then we can provide for this script

‎src/wallet/test/scriptpubkeyman_tests.cpp

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// Copyright (c) 2020 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <key.h>
6+
#include <script/standard.h>
7+
#include <test/util/setup_common.h>
8+
#include <wallet/scriptpubkeyman.h>
9+
#include <wallet/wallet.h>
10+
11+
#include <boost/test/unit_test.hpp>
12+
13+
BOOST_FIXTURE_TEST_SUITE(scriptpubkeyman_tests, BasicTestingSetup)
14+
15+
// Test LegacyScriptPubKeyMan::CanProvide behavior, making sure it returns true
16+
// for recognized scripts even when keys may not be available for signing.
17+
BOOST_AUTO_TEST_CASE(CanProvide)
18+
{
19+
// Set up wallet and keyman variables.
20+
NodeContext node;
21+
std::unique_ptr<interfaces::Chain> chain = interfaces::MakeChain(node);
22+
CWallet wallet(chain.get(), WalletLocation(), WalletDatabase::CreateDummy());
23+
LegacyScriptPubKeyMan& keyman = *wallet.GetOrCreateLegacyScriptPubKeyMan();
24+
25+
// Make a 1 of 2 multisig script
26+
std::vector<CKey> keys(2);
27+
std::vector<CPubKey> pubkeys;
28+
for (CKey& key : keys) {
29+
key.MakeNewKey(true);
30+
pubkeys.emplace_back(key.GetPubKey());
31+
}
32+
CScript multisig_script = GetScriptForMultisig(1, pubkeys);
33+
CScript p2sh_script = GetScriptForDestination(ScriptHash(multisig_script));
34+
SignatureData data;
35+
36+
// Verify the p2sh(multisig) script is not recognized until the multisig
37+
// script is added to the keystore to make it solvable
38+
BOOST_CHECK(!keyman.CanProvide(p2sh_script, data));
39+
keyman.AddCScript(multisig_script);
40+
BOOST_CHECK(keyman.CanProvide(p2sh_script, data));
41+
}
42+
43+
BOOST_AUTO_TEST_SUITE_END()

0 commit comments

Comments
 (0)