mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-23 07:15:29 +01:00
Code Issues Projects Releases Wiki Activity

Merge #18067: wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition

Browse source 
a304a3632f Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky)
eb7d8a5b07 [test] check for addmultisigaddress regression (Sjors Provoost)
005f8a92cc wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky)

Pull request description:

  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 #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 4a7e43e846 "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 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible

ACKs for top commit:
  Sjors:
    re-ACK a304a3632f (rebase, slight text changes and my test)
  achow101:
    re-ACK a304a3632f
  meshcollider:
    utACK a304a3632f

Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e
This commit is contained in:
Samuel Dobson 2020-02-19 14:14:19 +13:00
commit 68e841e0af
No known key found for this signature in database
GPG key ID: D300116E1C875A3D
6 changed files with 123 additions and 21 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
src/Makefile.test.include
View file

@ -209,7 +209,8 @@ BITCOIN_TESTS += \
wallet/test/wallet_crypto_tests.cpp \
wallet/test/coinselector_tests.cpp \
wallet/test/init_tests.cpp \
wallet/test/ismine_tests.cpp
wallet/test/ismine_tests.cpp \
wallet/test/scriptpubkeyman_tests.cpp
BITCOIN_TEST_SUITE += \
wallet/test/wallet_test_fixture.cpp \

14
src/outputtype.cpp
View file

@ -82,30 +82,22 @@ CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore,
{
// Add script to keystore
keystore.AddCScript(script);
ScriptHash sh(script);
// Note that scripts over 520 bytes are not yet supported.
switch (type) {
case OutputType::LEGACY:
keystore.AddCScript(GetScriptForDestination(sh));
return sh;
return ScriptHash(script);
case OutputType::P2SH_SEGWIT:
case OutputType::BECH32: {
CTxDestination witdest = WitnessV0ScriptHash(script);
CScript witprog = GetScriptForDestination(witdest);
// Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
if (!IsSolvable(keystore, witprog)) {
// Since the wsh is invalid, add and return the sh instead.
keystore.AddCScript(GetScriptForDestination(sh));
return sh;
}
if (!IsSolvable(keystore, witprog)) return ScriptHash(script);
// Add the redeemscript, so that P2WSH and P2SH-P2WSH outputs are recognized as ours.
keystore.AddCScript(witprog);
if (type == OutputType::BECH32) {
return witdest;
} else {
ScriptHash sh_w = ScriptHash(witprog);
keystore.AddCScript(GetScriptForDestination(sh_w));
return sh_w;
return ScriptHash(witprog);
}
}
default: assert(false);

46
src/script/signingprovider.h
View file

@ -66,7 +66,53 @@ protected:
using KeyMap = std::map<CKeyID, CKey>;
using ScriptMap = std::map<CScriptID, CScript>;
/**
* Map of key id to unencrypted private keys known by the signing provider.
* Map may be empty if the provider has another source of keys, like an
* encrypted store.
*/
KeyMap mapKeys GUARDED_BY(cs_KeyStore);
/**
* Map of script id to scripts known by the signing provider.
*
* This map originally just held P2SH redeemScripts, and was used by wallet
* code to look up script ids referenced in "OP_HASH160 <script id>
* OP_EQUAL" P2SH outputs. Later in 605e8473a7d it was extended to hold
* P2WSH witnessScripts as well, and used to look up nested scripts
* referenced in "OP_0 <script hash>" P2WSH outputs. Later in commits
* f4691ab3a9d and 248f3a76a82, it was extended once again to hold segwit
* "OP_0 <key or script hash>" scriptPubKeys, in order to give the wallet a
* way to distinguish between segwit outputs that it generated addresses for
* and wanted to receive payments from, and segwit outputs that it never
* generated addresses for, but it could spend just because of having keys.
* (Before segwit activation it was also important to not treat segwit
* outputs to arbitrary wallet keys as payments, because these could be
* spent by anyone without even needing to sign with the keys.)
*
* Some of the scripts stored in mapScripts are memory-only and
* intentionally not saved to disk. Specifically, scripts added by
* ImplicitlyLearnRelatedKeyScripts(pubkey) calls are not written to disk so
* future wallet code can have flexibility to be more selective about what
* transaction outputs it recognizes as payments, instead of having to treat
* all outputs spending to keys it knows as payments. By contrast,
* mapScripts entries added by AddCScript(script),
* LearnRelatedScripts(pubkey, type), and LearnAllRelatedScripts(pubkey)
* calls are saved because they are all intentionally used to receive
* payments.
*
* The FillableSigningProvider::mapScripts script map should not be confused
* with LegacyScriptPubKeyMan::setWatchOnly script set. The two collections
* can hold the same scripts, but they serve different purposes. The
* setWatchOnly script set is intended to expand the set of outputs the
* wallet considers payments. Every output with a script it contains is
* considered to belong to the wallet, regardless of whether the script is
* solvable or signable. By contrast, the scripts in mapScripts are only
* used for solving, and to restrict which outputs are considered payments
* by the wallet. An output with a script in mapScripts, unlike
* setWatchOnly, is not automatically considered to belong to the wallet if
* it can't be solved and signed for.
*/
ScriptMap mapScripts GUARDED_BY(cs_KeyStore);
void ImplicitlyLearnRelatedKeyScripts(const CPubKey& pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore);

24
src/wallet/scriptpubkeyman.cpp
View file

@ -70,7 +70,15 @@ bool HaveKeys(const std::vector<valtype>& pubkeys, const LegacyScriptPubKeyMan&
return true;
}
IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion)
//! Recursively solve script and return spendable/watchonly/invalid status.
//!
//! @param keystore legacy key and script store
//! @param script script to solve
//! @param sigversion script type (top-level / redeemscript / witnessscript)
//! @param recurse_scripthash whether to recurse into nested p2sh and p2wsh
//! scripts or simply treat any script that has been
//! stored in the keystore as spendable
IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion, bool recurse_scripthash=true)
{
IsMineResult ret = IsMineResult::NO;
@ -129,7 +137,7 @@ IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& s
CScriptID scriptID = CScriptID(uint160(vSolutions[0]));
CScript subscript;
if (keystore.GetCScript(scriptID, subscript)) {
ret = std::max(ret, IsMineInner(keystore, subscript, IsMineSigVersion::P2SH));
ret = std::max(ret, recurse_scripthash ? IsMineInner(keystore, subscript, IsMineSigVersion::P2SH) : IsMineResult::SPENDABLE);
}
break;
}
@ -147,7 +155,7 @@ IsMineResult IsMineInner(const LegacyScriptPubKeyMan& keystore, const CScript& s
CScriptID scriptID = CScriptID(hash);
CScript subscript;
if (keystore.GetCScript(scriptID, subscript)) {
ret = std::max(ret, IsMineInner(keystore, subscript, IsMineSigVersion::WITNESS_V0));
ret = std::max(ret, recurse_scripthash ? IsMineInner(keystore, subscript, IsMineSigVersion::WITNESS_V0) : IsMineResult::SPENDABLE);
}
break;
}
@ -476,11 +484,11 @@ std::unique_ptr<SigningProvider> LegacyScriptPubKeyMan::GetSigningProvider(const
bool LegacyScriptPubKeyMan::CanProvide(const CScript& script, SignatureData& sigdata)
{
if (IsMine(script) != ISMINE_NO) {
// If it IsMine, we can always provide in some way
return true;
} else if (HaveCScript(CScriptID(script))) {
// We can still provide some stuff if we have the script, but IsMine failed because we don't have keys
IsMineResult ismine = IsMineInner(*this, script, IsMineSigVersion::TOP, /* recurse_scripthash= */ false);
if (ismine == IsMineResult::SPENDABLE || ismine == IsMineResult::WATCH_ONLY) {
// If ismine, it means we recognize keys or script ids in the script, or
// are watching the script itself, and we can at least provide metadata
// or solving information, even if not able to sign fully.
return true;
} else {
// If, given the stuff in sigdata, we could make a valid sigature, then we can provide for this script

43
src/wallet/test/scriptpubkeyman_tests.cpp Normal file
View file

@ -0,0 +1,43 @@
// Copyright (c) 2020 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <key.h>
#include <script/standard.h>
#include <test/util/setup_common.h>
#include <wallet/scriptpubkeyman.h>
#include <wallet/wallet.h>
#include <boost/test/unit_test.hpp>
BOOST_FIXTURE_TEST_SUITE(scriptpubkeyman_tests, BasicTestingSetup)
// Test LegacyScriptPubKeyMan::CanProvide behavior, making sure it returns true
// for recognized scripts even when keys may not be available for signing.
BOOST_AUTO_TEST_CASE(CanProvide)
{
// Set up wallet and keyman variables.
NodeContext node;
std::unique_ptr<interfaces::Chain> chain = interfaces::MakeChain(node);
CWallet wallet(chain.get(), WalletLocation(), WalletDatabase::CreateDummy());
LegacyScriptPubKeyMan& keyman = *wallet.GetOrCreateLegacyScriptPubKeyMan();
// Make a 1 of 2 multisig script
std::vector<CKey> keys(2);
std::vector<CPubKey> pubkeys;
for (CKey& key : keys) {
key.MakeNewKey(true);
pubkeys.emplace_back(key.GetPubKey());
}
CScript multisig_script = GetScriptForMultisig(1, pubkeys);
CScript p2sh_script = GetScriptForDestination(ScriptHash(multisig_script));
SignatureData data;
// Verify the p2sh(multisig) script is not recognized until the multisig
// script is added to the keystore to make it solvable
BOOST_CHECK(!keyman.CanProvide(p2sh_script, data));
keyman.AddCScript(multisig_script);
BOOST_CHECK(keyman.CanProvide(p2sh_script, data));
}
BOOST_AUTO_TEST_SUITE_END()

14
test/functional/feature_backwards_compatibility.py
View file

@ -122,6 +122,9 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
info = wallet.getwalletinfo()
assert info['private_keys_enabled']
assert info['keypoolsize'] > 0
# Use addmultisigaddress (see #18075)
address_18075 = wallet.addmultisigaddress(1, ["0296b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52", "037211a824f55b505228e4c3d5194c1fcfaa15a456abdf37f9b9d97a4040afc073"], "", "legacy")["address"]
assert wallet.getaddressinfo(address_18075)["solvable"]
# w1_v18: regular wallet, created with v0.18
node_v18.createwallet(wallet_name="w1_v18")
@ -319,7 +322,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
hdkeypath = info["hdkeypath"]
pubkey = info["pubkey"]
# Copy the wallet to the last Bitcoin Core version and open it:
# Copy the 0.17 wallet to the last Bitcoin Core version and open it:
node_v17.unloadwallet("u1_v17")
shutil.copytree(
os.path.join(node_v17_wallets_dir, "u1_v17"),
@ -331,5 +334,14 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
descriptor = "wpkh([" + info["hdmasterfingerprint"] + hdkeypath[1:] + "]" + pubkey + ")"
assert_equal(info["desc"], descsum_create(descriptor))
# Copy the 0.19 wallet to the last Bitcoin Core version and open it:
shutil.copytree(
os.path.join(node_v19_wallets_dir, "w1_v19"),
os.path.join(node_master_wallets_dir, "w1_v19")
)
node_master.loadwallet("w1_v19")
wallet = node_master.get_wallet_rpc("w1_v19")
assert wallet.getaddressinfo(address_18075)["solvable"]
if __name__ == '__main__':
BackwardsCompatibilityTest().main()