This is a very simple pull requests that removes the global pwalletMain pointer and replaces the calls with CWallets::defaultWallet().
This PR leverages the internal global single wallet container to a vector. We have similar functionality on the Qt layer.
This will allow a step-by-step transition towards multiwallet support like proposed by @luke-jr in #8694.
229 | @@ -231,8 +230,8 @@ void Shutdown() 230 | pblocktree = NULL; 231 | } 232 | #ifdef ENABLE_WALLET 233 | - if (pwalletMain) 234 | - pwalletMain->Flush(true); 235 | + if (CWallets::defaultWallet())
CWallets::FlushAllWallets() ?
Yes. Changed it to flushAllWallets(bool shutdown);
998 | + } 999 | + static void addWallet(CWallet *newWallet) 1000 | + { 1001 | + LOCK(cs_wallets); 1002 | + wallets.push_back(newWallet); 1003 | + int i = 0;
i looks unused here 8)
That's black magic! :-)
Will remove it, had it there for debug purposes.
1530 | @@ -1535,9 +1531,9 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) 1531 | uiInterface.InitMessage(_("Done loading")); 1532 | 1533 | #ifdef ENABLE_WALLET 1534 | - if (pwalletMain) { 1535 | + if (CWallets::defaultWallet()) { 1536 | // Run a thread to flush wallet periodically 1537 | - threadGroup.create_thread(boost::bind(&ThreadFlushWalletDB, boost::ref(pwalletMain->strWalletFile))); 1538 | + threadGroup.create_thread(boost::bind(&ThreadFlushWalletDB, boost::ref(CWallets::defaultWallet()->strWalletFile)));
We keep the ThreadFlushWalletDB with "capacity" for a single wallet file for now.
Why?
ThreadFlushWalletDB is currently getting touched in different PRs).
Fine, can be done later. Though, I think in case it is just a simple for loop, it should be part of this pull.
I think we don't want a flush wallet thread per wallet. IMO it should be a single flush thread that loops though the wallet, check last-flush-time and optionally flushes.
Jup, @luke-jr did this. (Create a loop in ThreadFlushWalletDB over all wallets)
983 | @@ -986,4 +984,36 @@ class CAccount 984 | } 985 | }; 986 | 987 | +static CCriticalSection cs_wallets; 988 | +class CWallets 989 | +{ 990 | +public: 991 | + static std::vector<CWallet *> wallets;
Why public?
class CWallets
{
-public:
+private:
static std::vector<CWallet *> wallets;
+public:
static CWallet* defaultWallet()
{
compiles for me
Yes. I had a Namespace at the beginning and though that there is no big need for a Class or even private static class variables. But yes, why not private. Will update.
141 | @@ -142,7 +142,7 @@ void SignVerifyMessageDialog::on_signMessageButton_SM_clicked() 142 | } 143 | 144 | CKey key; 145 | - if (!pwalletMain->GetKey(keyID, key)) 146 | + if (!CWallets::defaultWallet()->GetKey(keyID, key))
There is no if (pwalletMain) test here in the original code?
@paveljanik The dialog does not exist when there is no wallet.
269 | @@ -270,7 +270,7 @@ UniValue importprunedfunds(const UniValue& params, bool fHelp) 270 | if (!DecodeHexTx(tx, params[0].get_str())) 271 | throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); 272 | uint256 hashTx = tx.GetHash(); 273 | - CWalletTx wtx(pwalletMain,tx); 274 | + CWalletTx wtx(CWallets::defaultWallet(),tx);
SPC after , :-)
Or just git diff HEAD~ -U0|cfd.
alias cfd='~/workspace/bitcoin/contrib/devtools/clang-format-diff.py -p1 -i -v'
I'm all for supporting multiple wallets, so concept ACK.
But isn't this only the part of @luke-jr 's work (64bcde7c5535cda12fc20ded718558ca74b28191)? Can you please coordinate the work to save your time? ;-)
Fixed nits.
1009 | + pWallet->Flush(shutdown); 1010 | + } 1011 | + static void destruct() 1012 | + { 1013 | + LOCK(cs_wallets); 1014 | + for(CWallet *pWallet : wallets)
μNit
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 656d344..d690e48 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -61 +61 @@ const uint256 CMerkleTx::ABANDON_HASH(uint256S("00000000000000000000000000000000
-std::vector<CWallet *> CWallets::wallets = {};
+std::vector<CWallet*> CWallets::wallets = {};
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 390210d..4fb69dd 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -991 +991,2 @@ private:
- static std::vector<CWallet *> wallets;
+ static std::vector<CWallet*> wallets;
+
@@ -1000 +1001 @@ public:
- static void addWallet(CWallet *newWallet)
+ static void addWallet(CWallet* newWallet)
@@ -1008 +1009 @@ public:
- for(CWallet *pWallet : wallets)
+ for (CWallet* pWallet : wallets)
@@ -1014 +1015 @@ public:
- for(CWallet *pWallet : wallets)
+ for (CWallet* pWallet : wallets)
utACK 9cc96b59f4bcf708ae370679ef41c6f8f2af3a14
1004 | + } 1005 | + static void flushAllWallets(bool shutdown) 1006 | + { 1007 | + LOCK(cs_wallets); 1008 | + for(CWallet *pWallet : wallets) 1009 | + pWallet->Flush(shutdown);
Does cs_wallets need to be held for access to (flushing) the wallet itself, or only for the wallets vector? If the former, then defaultWallet will fail to guard it; if the latter, the lock here is unnecessarily long-held.
I decided to hold the lock because it would be possible that during iteration a addWallet (or later removeWallet) happens. But right,.. it results in holding the lock for probably a long time.
I first though doing a copy of the wallets vector would be good, but the CWallet instance where the copy points to might be gone.
Maybe we keep this for now because the flushAllWallets happens only on shutdown. Or what would be a better solution?
Rebased and fixed code formatting (@MarcoFalkes micro-nit).
The diff is relatively large, but it would finally eliminate the global pwalletMain in favor of a more flexible handler that would be "ready" for multiwallet.
991 | + static std::vector<CWallet*> wallets; 992 | +public: 993 | + static CWallet* defaultWallet() 994 | + { 995 | + LOCK(cs_wallets); 996 | + if (!wallets.size())
Nit: I'd prefer if (wallets.empty()).
Needs rebase
Rebased (now also covers importmulti).
Closing. Since #8775, this is to far away from being useful. Maybe a different similar approach can be worked out.