Skip to content

Commit ed88ba7

Browse files
achow101PastaPastaPasta
authored andcommitted
Merge bitcoin#17261: Make ScriptPubKeyMan an actual interface and the wallet to have multiple
3f37365 Refactor: Replace SigningProvider pointers with unique_ptrs (Andrew Chow) 3afe53c Cleanup: Drop unused GUI learnRelatedScripts method (Andrew Chow) e2f02aa Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan (Andrew Chow) c729afd Box the wallet: Add multiple keyman maps and loops (Andrew Chow) 4977c30 refactor: define a UINT256_ONE global constant (Andrew Chow) 415afcc HD Split: Avoid redundant upgrades (Andrew Chow) 01b4511 Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan (Andrew Chow) 4a7e43e Store p2sh scripts in AddAndGetDestinationForScript (Andrew Chow) 501acb5 Always try to sign for all pubkeys in multisig (Andrew Chow) 81610ed List output types in an array in order to be iterated over (Andrew Chow) eb81fc3 Refactor: Allow LegacyScriptPubKeyMan to be null (Andrew Chow) fadc08a Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman (Andrew Chow) f5be479 wallet: Improve CWallet:MarkDestinationsDirty (João Barbosa) Pull request description: Continuation of wallet boxes project. Actually makes ScriptPubKeyMan an interface which LegacyScriptPubkeyMan. Moves around functions and things from CWallet into LegacyScriptPubKeyMan so that they are actually separate things without circular dependencies. *** Introducing the `ScriptPubKeyMan` (short for ScriptPubKeyManager) for managing scriptPubKeys and their associated scripts and keys. This functionality is moved over from `CWallet`. Instead, `CWallet` will have a pointer to a `ScriptPubKeyMan` for every possible address type, internal and external. It will fetch the correct `ScriptPubKeyMan` as necessary. When fetching new addresses, it chooses the `ScriptPubKeyMan` based on address type and whether it is change. For signing, it takes the script and asks each `ScriptPubKeyMan` for whether that `ScriptPubKeyMan` considers that script `IsMine`, whether it has that script, or whether it is able to produce a signature for it. If so, the `ScriptPubKeyMan` will provide a `SigningProvider` to the caller which will use that in order to sign. There is currently one `ScriptPubKeyMan` - the `LegacyScriptPubKeyMan`. Each `CWallet` will have only one `LegacyScriptPubKeyMan` with the pointers for all of the address types and change pointing to this `LegacyScriptPubKeyMan`. It is created when the wallet is loaded and all keys and metadata are loaded into it instead of `CWallet`. The `LegacyScriptPubKeyMan` is primarily made up of all of the key and script management that used to be in `CWallet`. For convenience, `CWallet` has a `GetLegacyScriptPubKeyMan` which will return the `LegacyScriptPubKeyMan` or a `nullptr` if it does not have one (not yet implemented, but callers will check for the `nullptr`). For purposes of signing, `LegacyScriptPubKeyMan`'s `GetSigningProvider` will return itself rather than a separate `SigningProvider`. This will be different for future `ScriptPubKeyMan`s. The `LegacyScriptPubKeyMan` will also handle the importing and exporting of keys and scripts instead of `CWallet`. As such, a number of RPCs have been limited to work only if a `LegacyScriptPubKeyMan` can be retrieved from the wallet. These RPCs are `sethdseed`, `addmultisigaddress`, `importaddress`, `importprivkey`, `importpubkey`, `importmulti`, `dumpprivkey`, and `dumpwallet`. Other RPCs which relied on the wallet for scripts and keys have been modified in order to take the `SigningProvider` retrieved from the `ScriptPubKeyMan` for a given script. Overall, these changes should not effect how everything actually works and the user should experience no difference between having this change and not having it. As such, no functional tests were changed, and the only unit tests changed were those that were directly accessing `CWallet` functions that have been removed. This PR is the last step in the [Wallet Structure Changes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes). ACKs for top commit: instagibbs: re-utACK bitcoin@3f37365 Sjors: re-utACK 3f37365 (it still compiles on macOS after bitcoin#17261 (comment)) meshcollider: Tested re-ACK 3f37365 Tree-SHA512: f8e2b8d9efa750b617691e8702d217ec4c33569ec2554a060141d9eb9b9a3a5323e4216938e2485c44625d7a6e0925d40dea1362b3af9857cf08860c2f344716
1 parent 0615c9f commit ed88ba7

32 files changed

+555
-294
lines changed

src/Makefile.am

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,7 @@ BITCOIN_CORE_H = \
246246
node/transaction.h \
247247
node/utxo_snapshot.h \
248248
noui.h \
249+
outputtype.h \
249250
policy/feerate.h \
250251
policy/fees.h \
251252
policy/policy.h \
@@ -673,6 +674,7 @@ libbitcoin_common_a_SOURCES = \
673674
netaddress.cpp \
674675
netbase.cpp \
675676
net_permissions.cpp \
677+
outputtype.cpp \
676678
policy/feerate.cpp \
677679
policy/policy.cpp \
678680
protocol.cpp \

src/bench/coin_selection.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,8 @@ static void CoinSelection(benchmark::Bench& bench)
3131
{
3232
NodeContext node;
3333
auto chain = interfaces::MakeChain(node);
34-
const CWallet wallet(chain.get(), "", CreateDummyWalletDatabase());
34+
CWallet wallet(chain.get(), "", CreateDummyWalletDatabase());
35+
wallet.SetupLegacyScriptPubKeyMan();
3536
std::vector<std::unique_ptr<CWalletTx>> wtxs;
3637
LOCK(wallet.cs_wallet);
3738

@@ -63,7 +64,7 @@ static void CoinSelection(benchmark::Bench& bench)
6364
typedef std::set<CInputCoin> CoinSet;
6465
static NodeContext testNode;
6566
static auto testChain = interfaces::MakeChain(testNode);
66-
static const CWallet testWallet(testChain.get(), "", CreateDummyWalletDatabase());
67+
static CWallet testWallet(testChain.get(), "", CreateDummyWalletDatabase());
6768
std::vector<std::unique_ptr<CWalletTx>> wtxn;
6869

6970
// Copied from src/wallet/test/coinselector_tests.cpp
@@ -92,6 +93,7 @@ static CAmount make_hard_case(int utxos, std::vector<OutputGroup>& utxo_pool)
9293
static void BnBExhaustion(benchmark::Bench& bench)
9394
{
9495
// Setup
96+
testWallet.SetupLegacyScriptPubKeyMan();
9597
std::vector<OutputGroup> utxo_pool;
9698
CoinSet selection;
9799
CAmount value_ret = 0;

src/bench/wallet_balance.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const b
2121
std::unique_ptr<interfaces::Chain> chain = interfaces::MakeChain(node);
2222
CWallet wallet{chain.get(), "", CreateMockWalletDatabase()};
2323
{
24+
wallet.SetupLegacyScriptPubKeyMan();
2425
bool first_run;
2526
if (wallet.LoadWallet(first_run) != DBErrors::LOAD_OK) assert(false);
2627
}

src/coinjoin/util.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ CTransactionBuilderOutput::CTransactionBuilderOutput(CTransactionBuilder* pTxBui
9494
{
9595
assert(pTxBuilder);
9696
CTxDestination txdest;
97+
LOCK(pwalletIn->cs_wallet);
9798
dest.GetReservedDestination(txdest, false);
9899
script = ::GetScriptForDestination(txdest);
99100
}

src/interfaces/wallet.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -192,15 +192,15 @@ class WalletImpl : public Wallet
192192
}
193193
bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) override
194194
{
195-
const SigningProvider* provider = m_wallet->GetSigningProvider(script);
195+
std::unique_ptr<SigningProvider> provider = m_wallet->GetSigningProvider(script);
196196
if (provider) {
197197
return provider->GetPubKey(address, pub_key);
198198
}
199199
return false;
200200
}
201201
bool getPrivKey(const CScript& script, const CKeyID& address, CKey& key) override
202202
{
203-
const SigningProvider* provider = m_wallet->GetSigningProvider(script);
203+
std::unique_ptr<SigningProvider> provider = m_wallet->GetSigningProvider(script);
204204
if (provider) {
205205
return provider->GetKey(address, key);
206206
}
@@ -254,7 +254,6 @@ class WalletImpl : public Wallet
254254
}
255255
return result;
256256
}
257-
//void learnRelatedScripts(const CPubKey& key, OutputType type) override { if (spk_man) m_wallet->GetLegacyScriptPubKeyMan()->LearnRelatedScripts(key, type); }
258257
bool addDestData(const CTxDestination& dest, const std::string& key, const std::string& value) override
259258
{
260259
LOCK(m_wallet->cs_wallet);

src/outputtype.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// Copyright (c) 2009-2010 Satoshi Nakamoto
2+
// Copyright (c) 2009-2019 The Bitcoin Core developers
3+
// Distributed under the MIT software license, see the accompanying
4+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+
#include <outputtype.h>
7+
8+
#include <script/signingprovider.h>
9+
#include <script/standard.h>
10+
11+
#include <assert.h>
12+
13+
CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, const CScript& script)
14+
{
15+
// Add script to keystore
16+
keystore.AddCScript(script);
17+
ScriptHash sh(script);
18+
// Note that scripts over 520 bytes are not yet supported.
19+
keystore.AddCScript(GetScriptForDestination(sh));
20+
return sh;
21+
}

src/outputtype.h

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// Copyright (c) 2009-2010 Satoshi Nakamoto
2+
// Copyright (c) 2009-2019 The Bitcoin Core developers
3+
// Distributed under the MIT software license, see the accompanying
4+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+
#ifndef BITCOIN_OUTPUTTYPE_H
7+
#define BITCOIN_OUTPUTTYPE_H
8+
9+
#include <script/signingprovider.h>
10+
#include <script/standard.h>
11+
12+
/**
13+
* Get a destination of the requested type (if possible) to the specified script.
14+
* This function will automatically add the script (and any other
15+
* necessary scripts) to the keystore.
16+
*/
17+
CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, const CScript& script);
18+
19+
#endif // BITCOIN_OUTPUTTYPE_H

src/qt/test/addressbooktests.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
6060
TestChain100Setup test;
6161
node.setContext(&test.m_node);
6262
std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), "", CreateMockWalletDatabase());
63+
wallet->SetupLegacyScriptPubKeyMan();
6364
bool firstRun;
6465
wallet->LoadWallet(firstRun);
6566

src/qt/test/wallettests.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -114,9 +114,8 @@ void TestGUI(interfaces::Node& node)
114114
bool firstRun;
115115
wallet->LoadWallet(firstRun);
116116
{
117-
auto spk_man = wallet->GetLegacyScriptPubKeyMan();
118-
LOCK(wallet->cs_wallet);
119-
AssertLockHeld(spk_man->cs_wallet);
117+
auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan();
118+
LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore);
120119
wallet->SetAddressBook(PKHash(test.coinbaseKey.GetPubKey()), "", "receive");
121120
spk_man->AddKeyPubKey(test.coinbaseKey, test.coinbaseKey.GetPubKey());
122121
wallet->SetLastBlockProcessed(105, ::ChainActive().Tip()->GetBlockHash());

src/rpc/misc.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -279,8 +279,9 @@ static UniValue createmultisig(const JSONRPCRequest& request)
279279
}
280280

281281
// Construct using pay-to-script-hash:
282+
FillableSigningProvider keystore;
282283
CScript inner;
283-
const CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, inner);
284+
const CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, keystore, inner);
284285

285286
UniValue result(UniValue::VOBJ);
286287
result.pushKV("address", EncodeDestination(dest));

0 commit comments

Comments
 (0)