Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Basic multiwallet support #8694

Merged
merged 13 commits into from Jun 12, 2017
Merged

Basic multiwallet support #8694

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f28eb80
Bugfix: wallet: Increment "update counter" only after actually making…
luke-jr Mar 9, 2017
9d15d55
Bugfix: wallet: Increment "update counter" when modifying account stuff
luke-jr Mar 9, 2017
23fb9ad
wallet: Move nAccountingEntryNumber from static/global to CWallet
luke-jr Mar 9, 2017
74e8738
Bugfix: ForceSetArg should replace entr(ies) in mapMultiArgs, not append
luke-jr Jan 3, 2017
19b3648
CWalletDB: Store the update counter per wallet
luke-jr Mar 9, 2017
b124cf0
Wallet: Replace pwalletMain with a vector of wallet pointers
luke-jr Sep 9, 2016
0f08575
Wallet: Support loading multiple wallets if -wallet used more than once
luke-jr Sep 9, 2016
008c360
Wallet: Move multiwallet sanity checks to CWallet::Verify, and do oth…
luke-jr Mar 9, 2017
84dcb45
Bugfix: wallet: Fix warningStr, errorStr argument order
luke-jr Jun 5, 2017
b823a4c
wallet: Include actual backup filename in recovery warning message
luke-jr Jun 5, 2017
a2a5f3f
wallet: Base backup filenames on original wallet filename
luke-jr Jun 5, 2017
9cbe8c8
wallet: Forbid -salvagewallet, -zapwallettxes, and -upgradewallet wit…
luke-jr Jun 5, 2017
c237bd7
wallet: Update formatting
luke-jr Jun 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 13 additions & 8 deletions src/init.cpp
View file
Open in desktop
Expand Up @@ -198,8 +198,9 @@ void Shutdown()
StopRPC();
StopHTTPServer();
#ifdef ENABLE_WALLET
if (pwalletMain)
pwalletMain->Flush(false);
for (CWalletRef pwallet : vpwallets) {
pwallet->Flush(false);
}
#endif
MapPort(false);
UnregisterValidationInterface(peerLogic.get());
Expand Down Expand Up @@ -239,8 +240,9 @@ void Shutdown()
pblocktree = NULL;
}
#ifdef ENABLE_WALLET
if (pwalletMain)
pwalletMain->Flush(true);
for (CWalletRef pwallet : vpwallets) {
pwallet->Flush(true);
}
#endif

#if ENABLE_ZMQ
Expand All @@ -260,8 +262,10 @@ void Shutdown()
#endif
UnregisterAllValidationInterfaces();
#ifdef ENABLE_WALLET
delete pwalletMain;
pwalletMain = NULL;
for (CWalletRef pwallet : vpwallets) {
delete pwallet;
}
vpwallets.clear();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you still need to delete each wallet here?

#endif
globalVerifyHandle.reset();
ECC_Stop();
Expand Down Expand Up @@ -1673,8 +1677,9 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
uiInterface.InitMessage(_("Done loading"));

#ifdef ENABLE_WALLET
if (pwalletMain)
pwalletMain->postInitProcess(scheduler);
for (CWalletRef pwallet : vpwallets) {
pwallet->postInitProcess(scheduler);
}
#endif

return !fRequestShutdown;
Expand Down
5 changes: 3 additions & 2 deletions src/qt/bitcoin.cpp
View file
Open in desktop
Expand Up @@ -474,9 +474,10 @@ void BitcoinApplication::initializeResult(bool success)
window->setClientModel(clientModel);

#ifdef ENABLE_WALLET
if(pwalletMain)
// TODO: Expose secondary wallets
if (!vpwallets.empty())
{
walletModel = new WalletModel(platformStyle, pwalletMain, optionsModel);
walletModel = new WalletModel(platformStyle, vpwallets[0], optionsModel);

window->addWallet(BitcoinGUI::DEFAULT_WALLET, walletModel);
window->setCurrentWallet(BitcoinGUI::DEFAULT_WALLET);
Expand Down
1 change: 1 addition & 0 deletions src/util.cpp
View file
Open in desktop
Expand Up @@ -477,6 +477,7 @@ void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strV
{
LOCK(cs_args);
mapArgs[strArg] = strValue;
mapMultiArgs[strArg].clear();
mapMultiArgs[strArg].push_back(strValue);
}

Expand Down
5 changes: 5 additions & 0 deletions src/wallet/db.cpp
View file
Open in desktop
Expand Up @@ -434,6 +434,11 @@ void CDB::Flush()
env->dbenv->txn_checkpoint(nMinutes ? GetArg("-dblogsize", DEFAULT_WALLET_DBLOGSIZE) * 1024 : 0, nMinutes, 0);
}

void CWalletDBWrapper::IncrementUpdateCounter()
{
++nUpdateCounter;
}

void CDB::Close()
{
if (!pdb)
Expand Down
13 changes: 10 additions & 3 deletions src/wallet/db.h
View file
Open in desktop
Expand Up @@ -93,13 +93,13 @@ class CWalletDBWrapper
friend class CDB;
public:
/** Create dummy DB handle */
CWalletDBWrapper(): env(nullptr)
CWalletDBWrapper() : nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0), env(nullptr)
{
}

/** Create DB handle to real database */
CWalletDBWrapper(CDBEnv *env_in, const std::string &strFile_in):
env(env_in), strFile(strFile_in)
CWalletDBWrapper(CDBEnv *env_in, const std::string &strFile_in) :
nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0), env(env_in), strFile(strFile_in)
{
}

Expand All @@ -119,6 +119,13 @@ class CWalletDBWrapper
*/
void Flush(bool shutdown);

void IncrementUpdateCounter();

std::atomic<unsigned int> nUpdateCounter;
unsigned int nLastSeen;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "Wallet: Replace pwalletMain with a vector of wallet pointers"

Should initialize these values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should initialize these values.

Note: comment no longer applies. This is now done in two CWalletDBWrapper constructors.

unsigned int nLastFlushed;
int64_t nLastWalletUpdate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "Wallet: Replace pwalletMain with a vector of wallet pointers"

It would be nice if conversion of these values from static variables to per-wallet members happened in the previous commit ("CWalletDB: Store the update counter per wallet") instead of this one. Would make both commits easier to follow and avoid the unusual intermediate state where globals and per-wallet members are compared to each other.


private:
/** BerkeleyDB specific */
CDBEnv *env;
Expand Down
3 changes: 2 additions & 1 deletion src/wallet/rpcwallet.cpp
View file
Open in desktop
Expand Up @@ -33,7 +33,8 @@

CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
{
return pwalletMain;
// TODO: Some way to access secondary wallets
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I throwing an RPC error like RPC_METHOD_NOT_FOUND is better than returning NULL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not always an error for there to be no wallet (eg, signrawtransaction).

return vpwallets.empty() ? nullptr : vpwallets[0];
}

std::string HelpRequiringPassphrase(CWallet * const pwallet)
Expand Down
2 changes: 2 additions & 0 deletions src/wallet/test/wallet_test_fixture.cpp
View file
Open in desktop
Expand Up @@ -8,6 +8,8 @@
#include "wallet/db.h"
#include "wallet/wallet.h"

CWallet *pwalletMain;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why define pwalletMain here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to modify the tests to use vpwallets in most cases?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "Wallet: Replace pwalletMain with a vector of wallet pointers":

Maybe add a TODO comment for updating the tests to stop referencing pwalletMain, since this will be confusing to someone seeing this who isn't familiar with the pre-multiwallet history of the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on the reason why they are setting pwalletMain. Generally that is to communicate with stuff in wallet.cpp/walletdb.cpp I'd say.


WalletTestingSetup::WalletTestingSetup(const std::string& chainName):
TestingSetup(chainName)
{
Expand Down
14 changes: 7 additions & 7 deletions src/wallet/test/wallet_tests.cpp
View file
Open in desktop
Expand Up @@ -19,6 +19,8 @@
#include <boost/test/unit_test.hpp>
#include <univalue.h>

extern CWallet* pwalletMain;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "Replace pwalletMain with a vector of wallet"

This can probably be removed after the importwallet_rescan test is updated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can probably be removed after the importwallet_rescan test is updated.

Never mind, it's used by the receiverequests test.


extern UniValue importmulti(const JSONRPCRequest& request);
extern UniValue dumpwallet(const JSONRPCRequest& request);
extern UniValue importwallet(const JSONRPCRequest& request);
Expand Down Expand Up @@ -402,8 +404,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
// after.
{
CWallet wallet;
CWallet *backup = ::pwalletMain;
::pwalletMain = &wallet;
vpwallets.insert(vpwallets.begin(), &wallet);
UniValue keys;
keys.setArray();
UniValue key;
Expand Down Expand Up @@ -434,7 +435,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
"downloading and rescanning the relevant blocks (see -reindex and -rescan "
"options).\"}},{\"success\":true}]",
0, oldTip->GetBlockTimeMax(), TIMESTAMP_WINDOW));
::pwalletMain = backup;
vpwallets.erase(vpwallets.begin());
}
}

Expand All @@ -444,7 +445,6 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
// than or equal to key birthday.
BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
{
CWallet *pwalletMainBackup = ::pwalletMain;
LOCK(cs_main);

// Create two blocks with same timestamp to verify that importwallet rescan
Expand All @@ -470,7 +470,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
JSONRPCRequest request;
request.params.setArray();
request.params.push_back("wallet.backup");
::pwalletMain = &wallet;
vpwallets.insert(vpwallets.begin(), &wallet);
::dumpwallet(request);
}

Expand All @@ -482,7 +482,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
JSONRPCRequest request;
request.params.setArray();
request.params.push_back("wallet.backup");
::pwalletMain = &wallet;
vpwallets[0] = &wallet;
::importwallet(request);

BOOST_CHECK_EQUAL(wallet.mapWallet.size(), 3);
Expand All @@ -495,7 +495,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
}

SetMockTime(0);
::pwalletMain = pwalletMainBackup;
vpwallets.erase(vpwallets.begin());
}

// Check that GetImmatureCredit() returns a newly calculated value instead of
Expand Down
9 changes: 4 additions & 5 deletions src/wallet/wallet.cpp
View file
Open in desktop
Expand Up @@ -35,7 +35,7 @@
#include <boost/algorithm/string/replace.hpp>
#include <boost/thread.hpp>

CWallet* pwalletMain = NULL;
std::vector<CWalletRef> vpwallets;
/** Transaction fee set by the user */
CFeeRate payTxFee(DEFAULT_TRANSACTION_FEE);
unsigned int nTxConfirmTarget = DEFAULT_TX_CONFIRM_TARGET;
Expand Down Expand Up @@ -2871,7 +2871,7 @@ bool CWallet::AddAccountingEntry(const CAccountingEntry& acentry)

bool CWallet::AddAccountingEntry(const CAccountingEntry& acentry, CWalletDB *pwalletdb)
{
if (!pwalletdb->WriteAccountingEntry_Backend(acentry))
if (!pwalletdb->WriteAccountingEntry(++nAccountingEntryNumber, acentry))
return false;

laccentries.push_back(acentry);
Expand Down Expand Up @@ -3884,7 +3884,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
walletInstance->ScanForWalletTransactions(pindexRescan, true);
LogPrintf(" rescan %15dms\n", GetTimeMillis() - nStart);
walletInstance->SetBestChain(chainActive.GetLocator());
CWalletDB::IncrementUpdateCounter();
walletInstance->dbw->IncrementUpdateCounter();

// Restore wallet transaction metadata after -zapwallettxes=1
if (GetBoolArg("-zapwallettxes", false) && GetArg("-zapwallettxes", "1") != "2")
Expand Down Expand Up @@ -3926,7 +3926,6 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
bool CWallet::InitLoadWallet()
{
if (GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)) {
pwalletMain = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vpwallets.clear() ?

EDIT: Meh, not really needed, just thought it would be more coherent.

LogPrintf("Wallet disabled!\n");
return true;
}
Expand All @@ -3943,7 +3942,7 @@ bool CWallet::InitLoadWallet()
if (!pwallet) {
return false;
}
pwalletMain = pwallet;
vpwallets.push_back(pwallet);

return true;
}
Expand Down
5 changes: 4 additions & 1 deletion src/wallet/wallet.h
View file
Open in desktop
Expand Up @@ -29,7 +29,8 @@
#include <utility>
#include <vector>

extern CWallet* pwalletMain;
typedef CWallet* CWalletRef;
extern std::vector<CWalletRef> vpwallets;

/**
* Settings
Expand Down Expand Up @@ -785,6 +786,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
nMasterKeyMaxID = 0;
pwalletdbEncryption = NULL;
nOrderPosNext = 0;
nAccountingEntryNumber = 0;
nNextResend = 0;
nLastResend = 0;
nTimeFirstKey = 0;
Expand All @@ -802,6 +804,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
TxItems wtxOrdered;

int64_t nOrderPosNext;
uint64_t nAccountingEntryNumber;
std::map<uint256, int> mapRequestCount;

std::map<CTxDestination, CAddressBookData> mapAddressBook;
Expand Down