From 8d8928eeeb995da2a5c0fea774be42e3d96c949f Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Tue, 9 Feb 2021 17:48:28 -0300 Subject: [PATCH 01/63] Encapsulate tx status in a Confirmation struct Adaptation of btc@a31be09bfd77eed497a8e251d31358e16e2f2eb1 Instead of relying on combination of hashBlock and nIndex values to manage tx in its lifecycle, we introduce 4 status : CONFIRMED, UNCONFIRMED, CONFLICTED, ABANDONED. hashBlock and nIndex magic values should only be used at serialization/deserialization for backward-compatibility. At block disconnection, we know flag txn as UNCONFIRMED where previously they kept their states until being override by a block connection or abandontransaction call. This is a change in behavior for which user may have to call abandon twice if transaction is disconnected and not accepted back in the mempool. We assert status transitioning right in AddToWallet. Doing so flagged a misbehavior in ComputeTimeSmart unit test where same tx is confirmed twice in different block. To avoid inconsistencies we unconfirmed tx before new connection in different block. We also remove a cs_main lock in test, as AddToWallet and its callees don't rely on locked chain. --- src/qt/transactionrecord.cpp | 2 +- src/test/librust/sapling_rpc_wallet_tests.cpp | 2 +- src/test/librust/sapling_wallet_tests.cpp | 14 +-- src/wallet/rpcwallet.cpp | 12 +-- .../test/wallet_shielded_balances_tests.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/wallet.cpp | 89 ++++++++++--------- src/wallet/wallet.h | 82 +++++++++++++---- 8 files changed, 129 insertions(+), 76 deletions(-) diff --git a/src/qt/transactionrecord.cpp b/src/qt/transactionrecord.cpp index 2d3639c40705..c616cfb8b4e7 100644 --- a/src/qt/transactionrecord.cpp +++ b/src/qt/transactionrecord.cpp @@ -569,7 +569,7 @@ void TransactionRecord::updateStatus(const CWalletTx& wtx) CBlockIndex *pindex = nullptr; // Find the block the tx is in - BlockMap::iterator mi = mapBlockIndex.find(wtx.hashBlock); + BlockMap::iterator mi = mapBlockIndex.find(wtx.m_confirm.hashBlock); if (mi != mapBlockIndex.end()) pindex = (*mi).second; diff --git a/src/test/librust/sapling_rpc_wallet_tests.cpp b/src/test/librust/sapling_rpc_wallet_tests.cpp index b08cc8b8735c..82ec94bb9072 100644 --- a/src/test/librust/sapling_rpc_wallet_tests.cpp +++ b/src/test/librust/sapling_rpc_wallet_tests.cpp @@ -432,7 +432,7 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_taddr_to_sapling) chainActive.SetTip(&fakeIndex); BOOST_CHECK(chainActive.Contains(&fakeIndex)); BOOST_CHECK_EQUAL(1, chainActive.Height()); - wtx.SetMerkleBranch(blockHash, 0); + wtx.SetConf(CWalletTx::Status::CONFIRMED, blockHash, 0); pwalletMain->LoadToWallet(wtx); BOOST_CHECK_MESSAGE(pwalletMain->GetAvailableBalance() > 0, "tx not confirmed"); diff --git a/src/test/librust/sapling_wallet_tests.cpp b/src/test/librust/sapling_wallet_tests.cpp index b675b89219d7..667372b5aaee 100644 --- a/src/test/librust/sapling_wallet_tests.cpp +++ b/src/test/librust/sapling_wallet_tests.cpp @@ -232,7 +232,7 @@ BOOST_AUTO_TEST_CASE(GetConflictedSaplingNotes) { auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); - wtx.SetMerkleBranch(block.GetHash(), 0); + wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); BOOST_CHECK(wallet.LoadToWallet(wtx)); // Simulate receiving new block and ChainTip signal @@ -351,7 +351,7 @@ BOOST_AUTO_TEST_CASE(SaplingNullifierIsSpent) { BOOST_CHECK(chainActive.Contains(&fakeIndex)); BOOST_CHECK_EQUAL(0, chainActive.Height()); - wtx.SetMerkleBranch(block.GetHash(), 0); + wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); wallet.LoadToWallet(wtx); // Verify note has been spent @@ -414,7 +414,7 @@ BOOST_AUTO_TEST_CASE(NavigateFromSaplingNullifierToNote) { BOOST_CHECK_EQUAL(0, chainActive.Height()); // Simulate SyncTransaction which calls AddToWalletIfInvolvingMe - wtx.SetMerkleBranch(block.GetHash(), 0); + wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); @@ -512,7 +512,7 @@ BOOST_AUTO_TEST_CASE(SpentSaplingNoteIsFromMe) { auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); - wtx.SetMerkleBranch(block.GetHash(), 0); + wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); wallet.LoadToWallet(wtx); // Simulate receiving new block and ChainTip signal. @@ -588,7 +588,7 @@ BOOST_AUTO_TEST_CASE(SpentSaplingNoteIsFromMe) { auto saplingNoteData2 = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx2.tx).first; BOOST_CHECK(saplingNoteData2.size() > 0); wtx2.SetSaplingNoteData(saplingNoteData2); - wtx2.SetMerkleBranch(block2.GetHash(), 0); + wtx2.SetConf(CWalletTx::Status::CONFIRMED, block2.GetHash(), 0); wallet.LoadToWallet(wtx2); // Verify note B is spent. AddToWallet invokes AddToSpends which updates mapTxSaplingNullifiers @@ -967,7 +967,7 @@ BOOST_AUTO_TEST_CASE(UpdatedSaplingNoteData) { auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() == 1); // wallet only has key for change output wtx.SetSaplingNoteData(saplingNoteData); - wtx.SetMerkleBranch(block.GetHash(), 0); + wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); wallet.LoadToWallet(wtx); // Simulate receiving new block and ChainTip signal @@ -1083,7 +1083,7 @@ BOOST_AUTO_TEST_CASE(MarkAffectedSaplingTransactionsDirty) { auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); - wtx.SetMerkleBranch(block.GetHash(), 0); + wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); wallet.LoadToWallet(wtx); // Simulate receiving new block and ChainTip signal diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 03a153f5ebcd..2d013f27f65b 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -62,9 +62,9 @@ void WalletTxToJSON(const CWalletTx& wtx, UniValue& entry) if (wtx.IsCoinBase() || wtx.IsCoinStake()) entry.pushKV("generated", true); if (confirms > 0) { - entry.pushKV("blockhash", wtx.hashBlock.GetHex()); - entry.pushKV("blockindex", wtx.nIndex); - entry.pushKV("blocktime", mapBlockIndex[wtx.hashBlock]->GetBlockTime()); + entry.pushKV("blockhash", wtx.m_confirm.hashBlock.GetHex()); + entry.pushKV("blockindex", wtx.m_confirm.nIndex); + entry.pushKV("blocktime", mapBlockIndex[wtx.m_confirm.hashBlock]->GetBlockTime()); } else { entry.pushKV("trusted", wtx.IsTrusted()); } @@ -2563,9 +2563,9 @@ UniValue listreceivedbyshieldaddress(const JSONRPCRequest& request) if (pwalletMain->mapWallet.count(entry.op.hash)) { const CWalletTx& wtx = pwalletMain->mapWallet.at(entry.op.hash); - if (!wtx.hashBlock.IsNull()) - height = mapBlockIndex[wtx.hashBlock]->nHeight; - index = wtx.nIndex; + if (!wtx.m_confirm.hashBlock.IsNull()) + height = mapBlockIndex[wtx.m_confirm.hashBlock]->nHeight; + index = wtx.m_confirm.nIndex; time = wtx.GetTxTime(); } diff --git a/src/wallet/test/wallet_shielded_balances_tests.cpp b/src/wallet/test/wallet_shielded_balances_tests.cpp index 7db6cc02e1ce..32a18649551e 100644 --- a/src/wallet/test/wallet_shielded_balances_tests.cpp +++ b/src/wallet/test/wallet_shielded_balances_tests.cpp @@ -306,7 +306,7 @@ FakeBlock SimpleFakeMine(CWalletTx& wtx, SaplingMerkleTree& currentTree) fakeBlock.pindex->phashBlock = &mapBlockIndex.find(fakeBlock.block.GetHash())->first; chainActive.SetTip(fakeBlock.pindex); BOOST_CHECK(chainActive.Contains(fakeBlock.pindex)); - wtx.SetMerkleBranch(fakeBlock.pindex->GetBlockHash(), 0); + wtx.SetConf(CWalletTx::Status::CONFIRMED, fakeBlock.pindex->GetBlockHash(), 0); return fakeBlock; } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index d4016d4008ac..961ab0e9d2b2 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -332,7 +332,7 @@ CBlockIndex* SimpleFakeMine(CWalletTx& wtx, CBlockIndex* pprev = nullptr) fakeIndex->phashBlock = &mapBlockIndex.find(block.GetHash())->first; chainActive.SetTip(fakeIndex); BOOST_CHECK(chainActive.Contains(fakeIndex)); - wtx.SetMerkleBranch(fakeIndex->GetBlockHash(), 0); + wtx.SetConf(CWalletTx::Status::CONFIRMED, fakeIndex->GetBlockHash(), 0); removeTxFromMempool(wtx); wtx.fInMempool = false; return fakeIndex; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index f54d4077f7e3..173d42f0d841 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -926,21 +926,15 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) bool fUpdated = false; if (!fInsertedNew) { - // Merge - if (!wtxIn.hashUnset() && wtxIn.hashBlock != wtx.hashBlock) { - wtx.hashBlock = wtxIn.hashBlock; + if (wtxIn.m_confirm.status != wtx.m_confirm.status) { + wtx.m_confirm.status = wtxIn.m_confirm.status; + wtx.m_confirm.nIndex = wtxIn.m_confirm.nIndex; + wtx.m_confirm.hashBlock = wtxIn.m_confirm.hashBlock; wtx.UpdateTimeSmart(); fUpdated = true; - } - // If no longer abandoned, update - if (wtxIn.hashBlock.IsNull() && wtx.isAbandoned()) { - wtx.hashBlock = wtxIn.hashBlock; - if (!fUpdated) wtx.UpdateTimeSmart(); - fUpdated = true; - } - if (wtxIn.nIndex != -1 && wtxIn.nIndex != wtx.nIndex) { - wtx.nIndex = wtxIn.nIndex; - fUpdated = true; + } else { + assert(wtx.m_confirm.nIndex == wtxIn.m_confirm.nIndex); + assert(wtx.m_confirm.hashBlock == wtxIn.m_confirm.hashBlock); } if (HasSaplingSPKM() && m_sspk_man->UpdatedNoteData(wtxIn, wtx)) { fUpdated = true; @@ -990,8 +984,8 @@ bool CWallet::LoadToWallet(const CWalletTx& wtxIn) auto it = mapWallet.find(txin.prevout.hash); if (it != mapWallet.end()) { CWalletTx& prevtx = it->second; - if (prevtx.nIndex == -1 && !prevtx.hashUnset()) { - MarkConflicted(prevtx.hashBlock, wtx.GetHash()); + if (prevtx.isConflicted()) { + MarkConflicted(prevtx.m_confirm.hashBlock, wtx.GetHash()); } } } @@ -1049,7 +1043,7 @@ void CWallet::AddExternalNotesDataToTx(CWalletTx& wtx) const * Abandoned state should probably be more carefully tracked via different * posInBlock signals or by checking mempool presence when necessary. */ -bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256& blockHash, int posInBlock, bool fUpdate) +bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Status status, const uint256& blockHash, int posInBlock, bool fUpdate) { const CTransaction& tx = *ptx; { @@ -1103,10 +1097,9 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256 if (isFromMe) AddExternalNotesDataToTx(wtx); } - // Get merkle branch if transaction was found in a block - if (!blockHash.IsNull()) { - wtx.SetMerkleBranch(blockHash, posInBlock); - } + // Block disconnection override an abandoned tx as unconfirmed + // which means user may have to call abandontransaction again + wtx.SetConf(status, blockHash, posInBlock); return AddToWallet(wtx, false); } @@ -1147,7 +1140,7 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) if (currentconfirm == 0 && !wtx.isAbandoned()) { // If the orig tx was not in block/mempool, none of its spends can be in mempool assert(!wtx.InMempool()); - wtx.nIndex = -1; + wtx.m_confirm.nIndex = 0; wtx.setAbandoned(); wtx.MarkDirty(); walletdb.WriteTx(wtx); @@ -1212,8 +1205,9 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) if (conflictconfirms < currentconfirm) { // Block is 'more conflicted' than current confirm; update. // Mark transaction as conflicted with this block. - wtx.nIndex = -1; - wtx.hashBlock = hashBlock; + wtx.m_confirm.nIndex = 0; + wtx.m_confirm.hashBlock = hashBlock; + wtx.setConflicted(); wtx.MarkDirty(); walletdb.WriteTx(wtx); // Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too @@ -1236,9 +1230,10 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) } } -void CWallet::SyncTransaction(const CTransactionRef& ptx, const CBlockIndex *pindexBlockConnected, int posInBlock) +void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Status status, const CBlockIndex *pindexBlockConnected, int posInBlock) { if (!AddToWalletIfInvolvingMe(ptx, + status, (pindexBlockConnected) ? pindexBlockConnected->GetBlockHash() : uint256(), posInBlock, true)) { @@ -1251,7 +1246,7 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, const CBlockIndex *pin void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) { LOCK2(cs_main, cs_wallet); - SyncTransaction(ptx, NULL, -1); + SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, nullptr, -1); auto it = mapWallet.find(ptx->GetHash()); if (it != mapWallet.end()) { @@ -1279,11 +1274,11 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const // the notification that the conflicted transaction was evicted. for (const CTransactionRef& ptx : vtxConflicted) { - SyncTransaction(ptx, nullptr, -1); + SyncTransaction(ptx, CWalletTx::Status::CONFLICTED, nullptr, -1); TransactionRemovedFromMempool(ptx); } for (size_t i = 0; i < pblock->vtx.size(); i++) { - SyncTransaction(pblock->vtx[i], pindex, i); + SyncTransaction(pblock->vtx[i], CWalletTx::Status::CONFIRMED, pindex, i); TransactionRemovedFromMempool(pblock->vtx[i]); } @@ -1309,8 +1304,13 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const void CWallet::BlockDisconnected(const std::shared_ptr& pblock, int nBlockHeight) { LOCK2(cs_main, cs_wallet); + + // At block disconnection, this will change an abandoned transaction to + // be unconfirmed, whether or not the transaction is added back to the mempool. + // User may have to call abandontransaction again. It may be addressed in the + // future with a stickier abandoned state or even removing abandontransaction call. for (const CTransactionRef& ptx : pblock->vtx) { - SyncTransaction(ptx, NULL, -1); + SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, nullptr, -1); } if (Params().GetConsensus().NetworkUpgradeActive(nBlockHeight, Consensus::UPGRADE_V5_0)) { @@ -1476,11 +1476,11 @@ int64_t CWalletTx::GetTxTime() const void CWalletTx::UpdateTimeSmart() { nTimeSmart = nTimeReceived; - if (!hashBlock.IsNull()) { - if (mapBlockIndex.count(hashBlock)) { - nTimeSmart = mapBlockIndex.at(hashBlock)->GetBlockTime(); + if (!m_confirm.hashBlock.IsNull()) { + if (mapBlockIndex.count(m_confirm.hashBlock)) { + nTimeSmart = mapBlockIndex.at(m_confirm.hashBlock)->GetBlockTime(); } else - LogPrintf("%s : found %s in block %s not in index\n", __func__, GetHash().ToString(), hashBlock.ToString()); + LogPrintf("%s : found %s in block %s not in index\n", __func__, GetHash().ToString(), m_confirm.hashBlock.ToString()); } } @@ -1837,7 +1837,7 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate, b } for (int posInBlock = 0; posInBlock < (int) block.vtx.size(); posInBlock++) { const auto& tx = block.vtx[posInBlock]; - if (AddToWalletIfInvolvingMe(tx, pindex->GetBlockHash(), posInBlock, fUpdate)) { + if (AddToWalletIfInvolvingMe(tx, CWalletTx::Status::CONFIRMED, pindex->GetBlockHash(), posInBlock, fUpdate)) { myTxHashes.push_back(tx->GetHash()); ret++; } @@ -3749,7 +3749,7 @@ void CWallet::GetKeyBirthTimes(std::map& mapKeyBirth) const for (std::map::const_iterator it = mapWallet.begin(); it != mapWallet.end(); it++) { // iterate over all wallet transactions... const CWalletTx& wtx = (*it).second; - BlockMap::const_iterator blit = mapBlockIndex.find(wtx.hashBlock); + BlockMap::const_iterator blit = mapBlockIndex.find(wtx.m_confirm.hashBlock); if (blit != mapBlockIndex.end() && chainActive.Contains(blit->second)) { // ... which are already in a block int nHeight = blit->second->nHeight; @@ -4283,13 +4283,16 @@ CWalletKey::CWalletKey(int64_t nExpires) nTimeExpires = nExpires; } -void CWalletTx::SetMerkleBranch(const uint256& blockHash, int posInBlock) +void CWalletTx::SetConf(Status status, const uint256& blockHash, int posInBlock) { + // Update tx status + m_confirm.status = status; + // Update the tx's hashBlock - hashBlock = blockHash; + m_confirm.hashBlock = blockHash; // set the position of the transaction in the block - nIndex = posInBlock; + m_confirm.nIndex = posInBlock; } int CWalletTx::GetDepthInMainChain() const @@ -4300,13 +4303,12 @@ int CWalletTx::GetDepthInMainChain() const int CWalletTx::GetDepthInMainChain(const CBlockIndex*& pindexRet) const { - if (hashUnset()) - return 0; + if (isUnconfirmed() || isAbandoned()) return 0; AssertLockHeld(cs_main); int nResult; // Find the block it claims to be in - BlockMap::iterator mi = mapBlockIndex.find(hashBlock); + BlockMap::iterator mi = mapBlockIndex.find(m_confirm.hashBlock); if (mi == mapBlockIndex.end()) { nResult = 0; } else { @@ -4315,7 +4317,7 @@ int CWalletTx::GetDepthInMainChain(const CBlockIndex*& pindexRet) const nResult = 0; } else { pindexRet = pindex; - nResult = ((nIndex == -1) ? (-1) : 1) * (chainActive.Height() - pindex->nHeight + 1); + nResult = (isConflicted() ? (-1) : 1) * (chainActive.Height() - pindex->nHeight + 1); } } @@ -4604,9 +4606,7 @@ bool CWallet::LoadSaplingPaymentAddress( //////////////////////////////////////////////////////////// CWalletTx::CWalletTx(const CWallet* pwalletIn, CTransactionRef arg) - : tx(std::move(arg)), - hashBlock(uint256()), - nIndex(-1) + : tx(std::move(arg)) { Init(pwalletIn); } @@ -4628,6 +4628,7 @@ void CWalletTx::Init(const CWallet* pwalletIn) fShieldedChangeCached = false; nShieldedChangeCached = 0; nOrderPos = -1; + m_confirm = Confirmation{}; } bool CWalletTx::IsTrusted() const diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c01e445e5ff0..52a339b5dd6e 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -312,7 +312,9 @@ class CWalletTx private: const CWallet* pwallet; - /** Constant used in hashBlock to indicate tx has been abandoned */ + /** Constant used in hashBlock to indicate tx has been abandoned, only used at + * serialization/deserialization to avoid ambiguity with conflicted. + */ static const uint256 ABANDON_HASH; public: @@ -346,13 +348,33 @@ class CWalletTx void Init(const CWallet* pwalletIn); CTransactionRef tx; - uint256 hashBlock; - /* An nIndex == -1 means that hashBlock (in nonzero) refers to the earliest - * block in the chain we know this or any in-wallet dependency conflicts - * with. Older clients interpret nIndex == -1 as unconfirmed for backward - * compatibility. + + /* New transactions start as UNCONFIRMED. At BlockConnected, + * they will transition to CONFIRMED. In case of reorg, at BlockDisconnected, + * they roll back to UNCONFIRMED. If we detect a conflicting transaction at + * block connection, we update conflicted tx and its dependencies as CONFLICTED. + * If tx isn't confirmed and outside of mempool, the user may switch it to ABANDONED + * by using the abandontransaction call. This last status may be override by a CONFLICTED + * or CONFIRMED transition. + */ + enum Status { + UNCONFIRMED, + CONFIRMED, + CONFLICTED, + ABANDONED + }; + + /* Confirmation includes tx status and a pair of {block hash/tx index in block} at which tx has been confirmed. + * This pair is both 0 if tx hasn't confirmed yet. Meaning of these fields changes with CONFLICTED state + * where they instead point to block hash and index of the deepest conflicting tx. */ - int nIndex; + struct Confirmation { + Status status = UNCONFIRMED; + uint256 hashBlock = uint256(); + int nIndex = 0; + }; + + Confirmation m_confirm; template void Serialize(Stream& s) const @@ -368,7 +390,9 @@ class CWalletTx std::vector dummy_vector1; //!< Used to be vMerkleBranch std::vector dummy_vector2; //!< Used to be vtxPrev char dummy_char = false; //!< Used to be fSpent - s << tx << hashBlock << dummy_vector1 << nIndex << dummy_vector2 << mapValueCopy << vOrderForm << fTimeReceivedIsTxTime << nTimeReceived << fFromMe << dummy_char; + uint256 serializedHash = isAbandoned() ? ABANDON_HASH : m_confirm.hashBlock; + int serializedIndex = isAbandoned() || isConflicted() ? -1 : m_confirm.nIndex; + s << tx << serializedHash << dummy_vector1 << serializedIndex << dummy_vector2 << mapValueCopy << vOrderForm << fTimeReceivedIsTxTime << nTimeReceived << fFromMe << dummy_char; if (this->tx->isSaplingVersion()) { s << mapSaplingNoteData; @@ -383,12 +407,30 @@ class CWalletTx std::vector dummy_vector1; //!< Used to be vMerkleBranch std::vector dummy_vector2; //!< Used to be vtxPrev char dummy_char; //! Used to be fSpent - s >> tx >> hashBlock >> dummy_vector1 >> nIndex >> dummy_vector2 >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> dummy_char; + int serializedIndex; + s >> tx >> m_confirm.hashBlock >> dummy_vector1 >> serializedIndex >> dummy_vector2 >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> fFromMe >> dummy_char; if (this->tx->isSaplingVersion()) { s >> mapSaplingNoteData; } + /* At serialization/deserialization, an nIndex == -1 means that hashBlock refers to + * the earliest block in the chain we know this or any in-wallet ancestor conflicts + * with. If nIndex == -1 and hashBlock is ABANDON_HASH, it means transaction is abandoned. + * In same context, an nIndex >= 0 refers to a confirmed transaction (if hashBlock set) or + * unconfirmed one. Older clients interpret nIndex == -1 as unconfirmed for backward + * compatibility (pre-commit 9ac63d6). + */ + if (serializedIndex == -1 && m_confirm.hashBlock == ABANDON_HASH) { + m_confirm.hashBlock = uint256(); + setAbandoned(); + } else if (serializedIndex == -1) { + setConflicted(); + } else if (!m_confirm.hashBlock.IsNull()) { + m_confirm.nIndex = serializedIndex; + setConfirmed(); + } + ReadOrderPos(nOrderPos, mapValue); nTimeSmart = mapValue.count("timesmart") ? (unsigned int)atoi64(mapValue["timesmart"]) : 0; @@ -470,7 +512,7 @@ class CWalletTx void RelayWalletTransaction(CConnman* connman); std::set GetConflicts() const; - void SetMerkleBranch(const uint256& blockHash, int posInBlock); + void SetConf(Status status, const uint256& blockHash, int posInBlock); /** * Return depth of transaction in blockchain: @@ -483,9 +525,19 @@ class CWalletTx bool IsInMainChain() const; bool IsInMainChainImmature() const; int GetBlocksToMaturity() const; - bool hashUnset() const { return (hashBlock.IsNull() || hashBlock == ABANDON_HASH); } - bool isAbandoned() const { return (hashBlock == ABANDON_HASH); } - void setAbandoned() { hashBlock = ABANDON_HASH; } + + bool isAbandoned() const { return m_confirm.status == CWalletTx::ABANDONED; } + void setAbandoned() + { + m_confirm.status = CWalletTx::ABANDONED; + m_confirm.hashBlock = UINT256_ZERO; + m_confirm.nIndex = 0; + } + bool isConflicted() const { return m_confirm.status == CWalletTx::CONFLICTED; } + void setConflicted() { m_confirm.status = CWalletTx::CONFLICTED; } + bool isUnconfirmed() const { return m_confirm.status == CWalletTx::UNCONFIRMED; } + void setUnconfirmed() { m_confirm.status = CWalletTx::UNCONFIRMED; } + void setConfirmed() { m_confirm.status = CWalletTx::CONFIRMED; } const uint256& GetHash() const { return tx->GetHash(); } bool IsCoinBase() const { return tx->IsCoinBase(); } @@ -552,7 +604,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface void ChainTipAdded(const CBlockIndex *pindex, const CBlock *pblock, SaplingMerkleTree saplingTree); /* Used by TransactionAddedToMemorypool/BlockConnected/Disconnected */ - void SyncTransaction(const CTransactionRef& tx, const CBlockIndex *pindexBlockConnected, int posInBlock); + void SyncTransaction(const CTransactionRef& tx, CWalletTx::Status status, const CBlockIndex *pindexBlockConnected, int posInBlock); bool IsKeyUsed(const CPubKey& vchPubKey); @@ -854,7 +906,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface void TransactionAddedToMempool(const CTransactionRef& tx) override; void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) override; void BlockDisconnected(const std::shared_ptr& pblock, int nBlockHeight) override; - bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, const uint256& blockHash, int posInBlock, bool fUpdate); + bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, CWalletTx::Status status, const uint256& blockHash, int posInBlock, bool fUpdate); void EraseFromWallet(const uint256& hash); /** From f7baeaf8776b41d227e3ccfa8b4fcc9a45366c68 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Tue, 9 Feb 2021 17:56:28 -0300 Subject: [PATCH 02/63] Remove SyncTransaction for conflicted txn in CWallet::BlockConnected We shouldn't rely on this sync call to get an accurate view of txn state, if a tx conflicts with one in mapTx we are going to update our wallet dependencies in AddToWalletIfInvolvingMe while conflicting txn get connected. If it doesn't conflict with one of our dependencies we are not going to track it anyway. This is a cleanup, as this SyncTransaction is redundant with the following one for confirmation which is triggering the MarkConflicted logic. We keep the loop because set of conflicted txn isn't same as txn included in block. --- src/wallet/wallet.cpp | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 173d42f0d841..bfeb276c3396 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1265,22 +1265,14 @@ void CWallet::TransactionRemovedFromMempool(const CTransactionRef &ptx) { void CWallet::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) { LOCK2(cs_main, cs_wallet); - // TODO: Tempoarily ensure that mempool removals are notified before - // connected transactions. This shouldn't matter, but the abandoned - // state of transactions in our wallet is currently cleared when we - // receive another notification and there is a race condition where - // notification of a connected conflict might cause an outside process - // to abandon a transaction and then have it inadvertently cleared by - // the notification that the conflicted transaction was evicted. - for (const CTransactionRef& ptx : vtxConflicted) { - SyncTransaction(ptx, CWalletTx::Status::CONFLICTED, nullptr, -1); - TransactionRemovedFromMempool(ptx); - } for (size_t i = 0; i < pblock->vtx.size(); i++) { SyncTransaction(pblock->vtx[i], CWalletTx::Status::CONFIRMED, pindex, i); TransactionRemovedFromMempool(pblock->vtx[i]); } + for (const CTransactionRef& ptx : vtxConflicted) { + TransactionRemovedFromMempool(ptx); + } // Sapling: notify about the connected block // Get prev block tree anchor From ff04fa64d635a92c07f1009d7313243124f050a5 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 9 Feb 2021 18:07:40 -0300 Subject: [PATCH 03/63] Modify wallet tx status if has been reorged out At tx loading, we rely on chain to know if hashBlock of tx is still in main chain. If not, we set its status to unconfirmed and reset its hashBlock/nIndex. We take lock prematurely in CWallet::LoadWallet and CWallet::Verify to ensure that lock order is respected between cs_main an cs_wallet. Inspired on btc@40ede992d97df38282919693dfe851c975c3b1d8 --- src/test/librust/sapling_rpc_wallet_tests.cpp | 3 ++- src/wallet/test/wallet_tests.cpp | 3 ++- src/wallet/wallet.cpp | 18 +++++++++++++++++- src/wallet/wallet.h | 2 +- 4 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/test/librust/sapling_rpc_wallet_tests.cpp b/src/test/librust/sapling_rpc_wallet_tests.cpp index 82ec94bb9072..8ddc53dca318 100644 --- a/src/test/librust/sapling_rpc_wallet_tests.cpp +++ b/src/test/librust/sapling_rpc_wallet_tests.cpp @@ -416,7 +416,8 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_taddr_to_sapling) CMutableTransaction mtx; mtx.vout.emplace_back(5 * COIN, GetScriptForDestination(taddr)); // Add to wallet and get the updated wtx - pwalletMain->LoadToWallet({pwalletMain, MakeTransactionRef(mtx)}); + CWalletTx wtxIn(pwalletMain, MakeTransactionRef(mtx)); + pwalletMain->LoadToWallet(wtxIn); CWalletTx& wtx = pwalletMain->mapWallet.at(mtx.GetHash()); // Fake-mine the transaction diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 961ab0e9d2b2..fc5595be3586 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -353,7 +353,8 @@ CWalletTx& BuildAndLoadTxToWallet(const std::vector& vin, mTx.vin = vin; mTx.vout = vout; CTransaction tx(mTx); - wallet.LoadToWallet({&wallet, MakeTransactionRef(tx)}); + CWalletTx wtx(&wallet, MakeTransactionRef(tx)); + wallet.LoadToWallet(wtx); return wallet.mapWallet.at(tx.GetHash()); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index bfeb276c3396..472199a8f698 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -971,8 +971,19 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) return true; } -bool CWallet::LoadToWallet(const CWalletTx& wtxIn) +bool CWallet::LoadToWallet(CWalletTx& wtxIn) { + LOCK2(cs_main, cs_wallet); + // If tx hasn't been reorged out of chain while wallet being shutdown + // change tx status to UNCONFIRMED and reset hashBlock/nIndex. + if (!wtxIn.m_confirm.hashBlock.IsNull()) { + CBlockIndex* pindex = mapBlockIndex[wtxIn.m_confirm.hashBlock]; + if (!pindex || !chainActive.Contains(pindex)) { + wtxIn.setUnconfirmed(); + wtxIn.m_confirm.hashBlock = UINT256_ZERO; + wtxIn.m_confirm.nIndex = 0; + } + } const uint256& hash = wtxIn.GetHash(); CWalletTx& wtx = mapWallet.emplace(hash, wtxIn).first->second; wtx.BindWallet(this); @@ -1948,6 +1959,11 @@ bool CWallet::Verify() if (gArgs.GetBoolArg("-salvagewallet", false)) { // Recover readable keypairs: CWallet dummyWallet; + // Even if we don't use this lock in this function, we want to preserve + // lock order in LoadToWallet if query of chain state is needed to know + // tx status. If lock can't be taken, tx confirmation status may be not + // reliable. + LOCK(cs_main); if (!CWalletDB::Recover(walletFile, (void *)&dummyWallet, CWalletDB::RecoverKeysOnlyFilter)) return false; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 52a339b5dd6e..470b1d6941cc 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -902,7 +902,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface void MarkDirty(); bool AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose = true); - bool LoadToWallet(const CWalletTx& wtxIn); + bool LoadToWallet(CWalletTx& wtxIn); void TransactionAddedToMempool(const CTransactionRef& tx) override; void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) override; void BlockDisconnected(const std::shared_ptr& pblock, int nBlockHeight) override; From da780393c83a7f49748b6131b6a16ccda7c3423b Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 10 Feb 2021 12:56:38 -0300 Subject: [PATCH 04/63] [Wallet] Adapting TransactionAddedToMempool and BlockDisconnected to the new wtx confirmation status Status::UNCONFIRMED always must have the index in 0, a negative index means that the transaction was abandoned. --- src/wallet/wallet.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 472199a8f698..3a52765e3cc3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1257,7 +1257,7 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Status stat void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) { LOCK2(cs_main, cs_wallet); - SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, nullptr, -1); + SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, nullptr, 0); auto it = mapWallet.find(ptx->GetHash()); if (it != mapWallet.end()) { @@ -1313,7 +1313,7 @@ void CWallet::BlockDisconnected(const std::shared_ptr& pblock, int // User may have to call abandontransaction again. It may be addressed in the // future with a stickier abandoned state or even removing abandontransaction call. for (const CTransactionRef& ptx : pblock->vtx) { - SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, nullptr, -1); + SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, nullptr, 0); } if (Params().GetConsensus().NetworkUpgradeActive(nBlockHeight, Consensus::UPGRADE_V5_0)) { From 46f0e30a6ae114234ee990ae2d6fd26c0284fb68 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Feb 2021 15:31:08 -0300 Subject: [PATCH 05/63] Fixing reindex problem, use mapBlockIndex.find() and not mapBlockIndex[] --- src/wallet/wallet.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3a52765e3cc3..f1ec02f1750f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -977,7 +977,8 @@ bool CWallet::LoadToWallet(CWalletTx& wtxIn) // If tx hasn't been reorged out of chain while wallet being shutdown // change tx status to UNCONFIRMED and reset hashBlock/nIndex. if (!wtxIn.m_confirm.hashBlock.IsNull()) { - CBlockIndex* pindex = mapBlockIndex[wtxIn.m_confirm.hashBlock]; + auto it = mapBlockIndex.find(wtxIn.m_confirm.hashBlock); + CBlockIndex* pindex = it == mapBlockIndex.end() ? nullptr : it->second; if (!pindex || !chainActive.Contains(pindex)) { wtxIn.setUnconfirmed(); wtxIn.m_confirm.hashBlock = UINT256_ZERO; From 370c20056706e4012fb9abd61467454f3706adaf Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Feb 2021 15:33:11 -0300 Subject: [PATCH 06/63] wallet: simplifying pindexRescan set + added an AssertLockHeld on FindForkInGlobalIndex --- src/validation.cpp | 1 + src/wallet/wallet.cpp | 8 ++------ 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index dcee17ea8a0e..a2375f782c4b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -177,6 +177,7 @@ std::set setDirtyFileInfo; CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator) { + AssertLockHeld(cs_main); // Find the first block the caller has in the main chain for (const uint256& hash : locator.vHave) { BlockMap::iterator mi = mapBlockIndex.find(hash); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index f1ec02f1750f..deef7d15c953 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4188,16 +4188,12 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile) LogPrintf("Wallet completed loading in %15dms\n", GetTimeMillis() - nStart); - CBlockIndex* pindexRescan = chainActive.Tip(); - if (gArgs.GetBoolArg("-rescan", false)) - pindexRescan = chainActive.Genesis(); - else { + CBlockIndex* pindexRescan = chainActive.Genesis(); + if (!gArgs.GetBoolArg("-rescan", false)) { CWalletDB walletdb(*walletInstance->dbw); CBlockLocator locator; if (walletdb.ReadBestBlock(locator)) pindexRescan = FindForkInGlobalIndex(chainActive, locator); - else - pindexRescan = chainActive.Genesis(); } walletInstance->m_last_block_processed = chainActive.Tip(); From 9e51a4886031cd8669a688bc388efd9f014fe8e0 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Thu, 11 Feb 2021 16:10:12 -0300 Subject: [PATCH 07/63] Add a test wallet_reorgsrestore Test we change tx status at loading in case of reorgs while wallet was shutdown. --- test/functional/test_runner.py | 1 + test/functional/wallet_reorgsrestore.py | 107 ++++++++++++++++++++++++ 2 files changed, 108 insertions(+) create mode 100755 test/functional/wallet_reorgsrestore.py diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index ff54527838f1..af87ac550cf6 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -58,6 +58,7 @@ # Longest test should go first, to favor running tests in parallel 'wallet_basic.py', # ~ 498 sec 'wallet_backup.py', # ~ 477 sec + 'wallet_reorgsrestore.py', # ~ 391 sec 'mempool_persist.py', # ~ 417 sec # vv Tests less than 5m vv diff --git a/test/functional/wallet_reorgsrestore.py b/test/functional/wallet_reorgsrestore.py new file mode 100755 index 000000000000..6d254c784bb2 --- /dev/null +++ b/test/functional/wallet_reorgsrestore.py @@ -0,0 +1,107 @@ +#!/usr/bin/env python3 +# Copyright (c) 2019 The Bitcoin Core developers +# Copyright (c) 2021 The PIVX Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or https://www.opensource.org/licenses/mit-license.php. + +"""Test tx status in case of reorgs while wallet being shutdown. + +Wallet txn status rely on block connection/disconnection for its +accuracy. In case of reorgs happening while wallet being shutdown +block updates are not going to be received. At wallet loading, we +check against chain if confirmed txn are still in chain and change +their status if block in which they have been included has been +disconnected. +""" + +from decimal import Decimal +import os +import shutil + +from test_framework.test_framework import PivxTestFramework +from test_framework.util import ( + assert_equal, + connect_nodes, + disconnect_nodes, + sync_blocks, +) + +class ReorgsRestoreTest(PivxTestFramework): + def set_test_params(self): + self.num_nodes = 3 + + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + + def run_test(self): + # Send a tx from which to conflict outputs later + txid_conflict_from = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("10")) + self.nodes[0].generate(1) + sync_blocks(self.nodes) + + # Disconnect node1 from others to reorg its chain later + disconnect_nodes(self.nodes[0], 1) + disconnect_nodes(self.nodes[1], 2) + connect_nodes(self.nodes[0], 2) + + # Send a tx to be unconfirmed later + txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("10")) + tx = self.nodes[0].gettransaction(txid) + self.nodes[0].generate(4) + tx_before_reorg = self.nodes[0].gettransaction(txid) + assert_equal(tx_before_reorg["confirmations"], 4) + + # Disconnect node0 from node2 to broadcast a conflict on their respective chains + disconnect_nodes(self.nodes[0], 2) + nA = next(tx_out["vout"] for tx_out in self.nodes[0].gettransaction(txid_conflict_from)["details"] if tx_out["amount"] == Decimal("10")) + inputs = [] + inputs.append({"txid": txid_conflict_from, "vout": nA}) + outputs_1 = {} + outputs_2 = {} + + # Create a conflicted tx broadcast on node0 chain and conflicting tx broadcast on node2 chain. Both spend from txid_conflict_from + outputs_1[self.nodes[0].getnewaddress()] = Decimal("9.99998") + outputs_2[self.nodes[0].getnewaddress()] = Decimal("9.99998") + conflicted = self.nodes[0].signrawtransaction(self.nodes[0].createrawtransaction(inputs, outputs_1)) + conflicting = self.nodes[0].signrawtransaction(self.nodes[0].createrawtransaction(inputs, outputs_2)) + + conflicted_txid = self.nodes[0].sendrawtransaction(conflicted["hex"]) + self.nodes[0].generate(1) + conflicting_txid = self.nodes[2].sendrawtransaction(conflicting["hex"]) + self.nodes[2].generate(9) + + # Reconnect node0 and node2 and check that conflicted_txid is effectively conflicted + connect_nodes(self.nodes[0], 2) + sync_blocks([self.nodes[0], self.nodes[2]]) + conflicted = self.nodes[0].gettransaction(conflicted_txid) + conflicting = self.nodes[0].gettransaction(conflicting_txid) + assert_equal(conflicted["confirmations"], -9) + assert_equal(conflicted["walletconflicts"][0], conflicting["txid"]) + + # Node0 wallet is shutdown + self.stop_node(0) + self.start_node(0) + + # The block chain re-orgs and the tx is included in a different block + self.nodes[1].generate(9) + self.nodes[1].sendrawtransaction(tx["hex"]) + self.nodes[1].generate(1) + self.nodes[1].sendrawtransaction(conflicted["hex"]) + self.nodes[1].generate(1) + + # Node0 wallet file is loaded on longest sync'ed node1 + self.stop_node(1) + self.nodes[0].backupwallet(os.path.join(self.nodes[0].datadir, 'wallet.bak')) + shutil.copyfile(os.path.join(self.nodes[0].datadir, 'wallet.bak'), os.path.join(self.nodes[1].datadir, 'regtest', 'wallet.dat')) + self.start_node(1) + tx_after_reorg = self.nodes[1].gettransaction(txid) + # Check that normal confirmed tx is confirmed again but with different blockhash + assert_equal(tx_after_reorg["confirmations"], 2) + assert(tx_before_reorg["blockhash"] != tx_after_reorg["blockhash"]) + conflicted_after_reorg = self.nodes[1].gettransaction(conflicted_txid) + # Check that conflicted tx is confirmed again with blockhash different than previously conflicting tx + assert_equal(conflicted_after_reorg["confirmations"], 1) + assert(conflicting["blockhash"] != conflicted_after_reorg["blockhash"]) + +if __name__ == '__main__': + ReorgsRestoreTest().main() From e7c8ca62fb1c92366078d15f2636f8ec38fed32b Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Feb 2021 16:12:03 -0300 Subject: [PATCH 08/63] wallet: remove unused IsInMainChain method --- src/wallet/wallet.cpp | 5 ----- src/wallet/wallet.h | 1 - 2 files changed, 6 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index deef7d15c953..79de939574dd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4337,11 +4337,6 @@ int CWalletTx::GetBlocksToMaturity() const return std::max(0, (Params().GetConsensus().nCoinbaseMaturity + 1) - GetDepthInMainChain()); } -bool CWalletTx::IsInMainChain() const -{ - return GetDepthInMainChain() > 0; -} - bool CWalletTx::IsInMainChainImmature() const { if (!IsCoinBase() && !IsCoinStake()) return false; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 470b1d6941cc..ce79dfa3ebe8 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -522,7 +522,6 @@ class CWalletTx */ int GetDepthInMainChain(const CBlockIndex*& pindexRet) const; int GetDepthInMainChain() const; - bool IsInMainChain() const; bool IsInMainChainImmature() const; int GetBlocksToMaturity() const; From 33f4788affa4282a7ea36ccf891a78819da0d2cc Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Feb 2021 16:18:00 -0300 Subject: [PATCH 09/63] wallet: refactor GetDepthInMainChain to not return the block index. It's only used for coin stake inputs selection and nowhere else. --- src/wallet/wallet.cpp | 22 +++++----------------- src/wallet/wallet.h | 1 - 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 79de939574dd..3bdd67027b34 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2247,14 +2247,14 @@ void CWallet::GetAvailableP2CSCoins(std::vector& vCoins) const { /** * Test if the transaction is spendable. */ -bool CheckTXAvailability(const CWalletTx* pcoin, bool fOnlyConfirmed, int& nDepth, const CBlockIndex*& pindexRet) +bool CheckTXAvailability(const CWalletTx* pcoin, bool fOnlyConfirmed, int& nDepth) { AssertLockHeld(cs_main); if (!CheckFinalTx(pcoin->tx)) return false; if (fOnlyConfirmed && !pcoin->IsTrusted()) return false; if (pcoin->GetBlocksToMaturity() > 0) return false; - nDepth = pcoin->GetDepthInMainChain(pindexRet); + nDepth = pcoin->GetDepthInMainChain(); // We should not consider coins which aren't at least in our mempool // It's possible for these to be conflicted via ancestors which we may never be able to detect @@ -2263,12 +2263,6 @@ bool CheckTXAvailability(const CWalletTx* pcoin, bool fOnlyConfirmed, int& nDept return true; } -bool CheckTXAvailability(const CWalletTx* pcoin, bool fOnlyConfirmed, int& nDepth) -{ - const CBlockIndex* pindexRet = nullptr; - return CheckTXAvailability(pcoin, fOnlyConfirmed, nDepth, pindexRet); -} - bool CWallet::GetMasternodeVinAndKeys(CTxIn& txinRet, CPubKey& pubKeyRet, CKey& keyRet, std::string strTxHash, std::string strOutputIndex, std::string& strError) { // wait for reindex and/or import to finish @@ -2547,13 +2541,13 @@ bool CWallet::StakeableCoins(std::vector* pCoins) // Check if the tx is selectable int nDepth; - const CBlockIndex* pindex = nullptr; - if (!CheckTXAvailability(pcoin, true, nDepth, pindex)) + if (!CheckTXAvailability(pcoin, true, nDepth)) continue; // Check min depth requirement for stake inputs if (nDepth < Params().GetConsensus().nStakeMinDepth) continue; + const CBlockIndex* pindex = nullptr; for (unsigned int index = 0; index < pcoin->tx->vout.size(); index++) { auto res = CheckOutputAvailability( @@ -2571,6 +2565,7 @@ bool CWallet::StakeableCoins(std::vector* pCoins) // found valid coin if (!pCoins) return true; + if (!pindex) pindex = mapBlockIndex.at(pcoin->m_confirm.hashBlock); pCoins->emplace_back(CStakeableOutput(pcoin, (int) index, nDepth, res.spendable, res.solvable, pindex)); } } @@ -4301,12 +4296,6 @@ void CWalletTx::SetConf(Status status, const uint256& blockHash, int posInBlock) } int CWalletTx::GetDepthInMainChain() const -{ - const CBlockIndex* pindexRet = nullptr; - return GetDepthInMainChain(pindexRet); -} - -int CWalletTx::GetDepthInMainChain(const CBlockIndex*& pindexRet) const { if (isUnconfirmed() || isAbandoned()) return 0; AssertLockHeld(cs_main); @@ -4321,7 +4310,6 @@ int CWalletTx::GetDepthInMainChain(const CBlockIndex*& pindexRet) const if (!pindex || !chainActive.Contains(pindex)) { nResult = 0; } else { - pindexRet = pindex; nResult = (isConflicted() ? (-1) : 1) * (chainActive.Height() - pindex->nHeight + 1); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ce79dfa3ebe8..afe607588a54 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -520,7 +520,6 @@ class CWalletTx * 0 : in memory pool, waiting to be included in a block * >=1 : this many blocks deep in the main chain */ - int GetDepthInMainChain(const CBlockIndex*& pindexRet) const; int GetDepthInMainChain() const; bool IsInMainChainImmature() const; int GetBlocksToMaturity() const; From 495705147d37a01b0038296a13052e3fee737c7d Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Feb 2021 16:57:52 -0300 Subject: [PATCH 10/63] wallet: cache block hash, height and time inside the wallet. --- src/validation.cpp | 2 +- src/validationinterface.cpp | 10 +++++----- src/validationinterface.h | 4 ++-- src/wallet/wallet.cpp | 30 +++++++++++++++++++++------- src/wallet/wallet.h | 14 +++++++++++-- src/zmq/zmqnotificationinterface.cpp | 2 +- src/zmq/zmqnotificationinterface.h | 2 +- 7 files changed, 45 insertions(+), 19 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index a2375f782c4b..d8477fd5cd8a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1913,7 +1913,7 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara UpdateTip(pindexDelete->pprev); // Let wallets know transactions went from 1-confirmed to // 0-confirmed or conflicted: - GetMainSignals().BlockDisconnected(pblock, pindexDelete->nHeight); + GetMainSignals().BlockDisconnected(pblock, pindexDelete->GetBlockHash(), pindexDelete->nHeight, pindexDelete->GetBlockTime()); return true; } diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index ad4af45dd7b1..ab2b92527642 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -37,7 +37,7 @@ struct MainSignalsInstance { */ boost::signals2::signal &, const CBlockIndex *pindex, const std::vector &)> BlockConnected; /** Notifies listeners of a block being disconnected */ - boost::signals2::signal &, int nBlockHeight)> BlockDisconnected; + boost::signals2::signal &, const uint256& blockHash, int nBlockHeight, int64_t blockTime)> BlockDisconnected; /** Notifies listeners of a transaction removal from the mempool */ boost::signals2::signal TransactionRemovedFromMempool; /** Notifies listeners of a new active block chain. */ @@ -98,7 +98,7 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) conns.UpdatedBlockTip = g_signals.m_internals->UpdatedBlockTip.connect(std::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3)); conns.TransactionAddedToMempool = g_signals.m_internals->TransactionAddedToMempool.connect(std::bind(&CValidationInterface::TransactionAddedToMempool, pwalletIn, std::placeholders::_1)); conns.BlockConnected = g_signals.m_internals->BlockConnected.connect(std::bind(&CValidationInterface::BlockConnected, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3)); - conns.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, std::placeholders::_1, std::placeholders::_2)); + conns.BlockDisconnected = g_signals.m_internals->BlockDisconnected.connect(std::bind(&CValidationInterface::BlockDisconnected, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4)); conns.TransactionRemovedFromMempool = g_signals.m_internals->TransactionRemovedFromMempool.connect(std::bind(&CValidationInterface::TransactionRemovedFromMempool, pwalletIn, std::placeholders::_1)); conns.SetBestChain = g_signals.m_internals->SetBestChain.connect(std::bind(&CValidationInterface::SetBestChain, pwalletIn, std::placeholders::_1)); conns.Broadcast = g_signals.m_internals->Broadcast.connect(std::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, std::placeholders::_1)); @@ -163,9 +163,9 @@ void CMainSignals::BlockConnected(const std::shared_ptr &pblock, c }); } -void CMainSignals::BlockDisconnected(const std::shared_ptr &pblock, int nBlockHeight) { - m_internals->m_schedulerClient.AddToProcessQueue([pblock, nBlockHeight, this] { - m_internals->BlockDisconnected(pblock, nBlockHeight); +void CMainSignals::BlockDisconnected(const std::shared_ptr &pblock, const uint256& blockHash, int nBlockHeight, int64_t blockTime) { + m_internals->m_schedulerClient.AddToProcessQueue([pblock, blockHash, nBlockHeight, blockTime, this] { + m_internals->BlockDisconnected(pblock, blockHash, nBlockHeight, blockTime); }); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 7377e13ae286..27bfd9d4ce17 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -93,7 +93,7 @@ class CValidationInterface { * * Called on a background thread. */ - virtual void BlockDisconnected(const std::shared_ptr &block, int nBlockHeight) {} + virtual void BlockDisconnected(const std::shared_ptr &block, const uint256& blockHash, int nBlockHeight, int64_t blockTime) {} /** * Notifies listeners of the new active block chain on-disk. * @@ -138,7 +138,7 @@ class CMainSignals { void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); void TransactionAddedToMempool(const CTransactionRef &ptxn); void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::shared_ptr> &); - void BlockDisconnected(const std::shared_ptr &block, int nBlockHeight); + void BlockDisconnected(const std::shared_ptr &block, const uint256& blockHash, int nBlockHeight, int64_t blockTime); void SetBestChain(const CBlockLocator &); void Broadcast(CConnman* connman); void BlockChecked(const CBlock&, const CValidationState&); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3bdd67027b34..f9460f2e3df1 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1278,6 +1278,9 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const { LOCK2(cs_main, cs_wallet); + m_last_block_processed = pindex->GetBlockHash(); + m_last_block_processed_time = pindex->GetBlockTime(); + m_last_block_processed_height = pindex->nHeight; for (size_t i = 0; i < pblock->vtx.size(); i++) { SyncTransaction(pblock->vtx[i], CWalletTx::Status::CONFIRMED, pindex, i); TransactionRemovedFromMempool(pblock->vtx[i]); @@ -1301,11 +1304,9 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const // Sapling: Update cached incremental witnesses ChainTipAdded(pindex, pblock.get(), oldSaplingTree); - - m_last_block_processed = pindex; } -void CWallet::BlockDisconnected(const std::shared_ptr& pblock, int nBlockHeight) +void CWallet::BlockDisconnected(const std::shared_ptr& pblock, const uint256& blockHash, int nBlockHeight, int64_t blockTime) { LOCK2(cs_main, cs_wallet); @@ -1313,6 +1314,9 @@ void CWallet::BlockDisconnected(const std::shared_ptr& pblock, int // be unconfirmed, whether or not the transaction is added back to the mempool. // User may have to call abandontransaction again. It may be addressed in the // future with a stickier abandoned state or even removing abandontransaction call. + m_last_block_processed_height = nBlockHeight - 1; + m_last_block_processed_time = blockTime; + m_last_block_processed = blockHash; for (const CTransactionRef& ptx : pblock->vtx) { SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, nullptr, 0); } @@ -1328,17 +1332,21 @@ void CWallet::BlockUntilSyncedToCurrentChain() { AssertLockNotHeld(cs_main); AssertLockNotHeld(cs_wallet); - if (m_last_block_processed) { + { // Skip the queue-draining stuff if we know we're caught up with // chainActive.Tip()... // We could also take cs_wallet here, and call m_last_block_processed // protected by cs_wallet instead of cs_main, but as long as we need // cs_main here anyway, its easier to just call it cs_main-protected. + uint256 last_block_hash = WITH_LOCK(cs_wallet, return m_last_block_processed); LOCK(cs_main); const CBlockIndex* initialChainTip = chainActive.Tip(); - if (m_last_block_processed->GetAncestor(initialChainTip->nHeight) == initialChainTip) { - return; + if (!last_block_hash.IsNull()) { + auto it = mapBlockIndex.find(last_block_hash); + if (it == mapBlockIndex.end() || it->second->GetAncestor(initialChainTip->nHeight) == initialChainTip) { + return; + } } } @@ -4191,7 +4199,15 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile) pindexRescan = FindForkInGlobalIndex(chainActive, locator); } - walletInstance->m_last_block_processed = chainActive.Tip(); + { + LOCK(walletInstance->cs_wallet); + const CBlockIndex* tip = chainActive.Tip(); + if (tip) { + walletInstance->m_last_block_processed = tip->GetBlockHash(); + walletInstance->m_last_block_processed_height = tip->nHeight; + walletInstance->m_last_block_processed_time = tip->GetBlockTime(); + } + } RegisterValidationInterface(walletInstance); if (chainActive.Tip() && chainActive.Tip() != pindexRescan) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index afe607588a54..607914e2717f 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -579,7 +579,15 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface * * Protected by cs_main (see BlockUntilSyncedToCurrentChain) */ - const CBlockIndex* m_last_block_processed{nullptr}; + uint256 m_last_block_processed GUARDED_BY(cs_wallet) = UINT256_ZERO; + + /* Height of last block processed is used by wallet to know depth of transactions + * without relying on Chain interface beyond asynchronous updates. For safety, we + * initialize it to -1. Height is a pointer on node's tip and doesn't imply + * that the wallet has scanned sequentially all blocks up to this one. + */ + int m_last_block_processed_height GUARDED_BY(cs_wallet) = -1; + int64_t m_last_block_processed_time GUARDED_BY(cs_wallet) = 0; int64_t nNextResend; int64_t nLastResend; @@ -636,6 +644,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface bool IsHDEnabled() const; //! Whether the wallet supports Sapling or not // bool IsSaplingUpgradeEnabled() const; + //! Return the height of the last processed block + int GetLastBlockHeight() const { return m_last_block_processed_height; } /* SPKM Helpers */ const CKeyingMaterial& GetEncryptionKey() const; @@ -903,7 +913,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface bool LoadToWallet(CWalletTx& wtxIn); void TransactionAddedToMempool(const CTransactionRef& tx) override; void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) override; - void BlockDisconnected(const std::shared_ptr& pblock, int nBlockHeight) override; + void BlockDisconnected(const std::shared_ptr& pblock, const uint256& blockHash, int nBlockHeight, int64_t blockTime) override; bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, CWalletTx::Status status, const uint256& blockHash, int posInBlock, bool fUpdate); void EraseFromWallet(const uint256& hash); diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index 19ad4a0eadd7..30d522c3d92e 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -172,7 +172,7 @@ void CZMQNotificationInterface::BlockConnected(const std::shared_ptr& pblock, int nBlockHeight) +void CZMQNotificationInterface::BlockDisconnected(const std::shared_ptr& pblock, const uint256& blockHash, int nBlockHeight, int64_t blockTime) { for (const CTransactionRef& ptx : pblock->vtx) { // Do a normal notify for each transaction removed in block disconnection diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index b4df973d9781..5bb17cd6c504 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -27,7 +27,7 @@ class CZMQNotificationInterface : public CValidationInterface // CValidationInterface void TransactionAddedToMempool(const CTransactionRef& tx) override; void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted) override; - void BlockDisconnected(const std::shared_ptr& pblock, int nBlockHeight) override; + void BlockDisconnected(const std::shared_ptr& pblock, const uint256& blockHash, int nBlockHeight, int64_t blockTime) override; void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override; private: From 6320efbbef2316716763683773a79515dea89581 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Feb 2021 17:10:46 -0300 Subject: [PATCH 11/63] Update wallet last processed index in every unit test that needs it --- .../test/wallet_shielded_balances_tests.cpp | 5 +++-- src/wallet/test/wallet_tests.cpp | 5 +++-- src/wallet/wallet.h | 18 ++++++++++++++++-- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/wallet/test/wallet_shielded_balances_tests.cpp b/src/wallet/test/wallet_shielded_balances_tests.cpp index 32a18649551e..628753a54d2b 100644 --- a/src/wallet/test/wallet_shielded_balances_tests.cpp +++ b/src/wallet/test/wallet_shielded_balances_tests.cpp @@ -291,7 +291,7 @@ struct FakeBlock CBlockIndex* pindex; }; -FakeBlock SimpleFakeMine(CWalletTx& wtx, SaplingMerkleTree& currentTree) +FakeBlock SimpleFakeMine(CWalletTx& wtx, SaplingMerkleTree& currentTree, CWallet& wallet) { FakeBlock fakeBlock; fakeBlock.block.nVersion = 8; @@ -306,6 +306,7 @@ FakeBlock SimpleFakeMine(CWalletTx& wtx, SaplingMerkleTree& currentTree) fakeBlock.pindex->phashBlock = &mapBlockIndex.find(fakeBlock.block.GetHash())->first; chainActive.SetTip(fakeBlock.pindex); BOOST_CHECK(chainActive.Contains(fakeBlock.pindex)); + WITH_LOCK(wallet.cs_wallet, wallet.SetLastBlockProcessed(fakeBlock.pindex)); wtx.SetConf(CWalletTx::Status::CONFIRMED, fakeBlock.pindex->GetBlockHash(), 0); return fakeBlock; } @@ -347,7 +348,7 @@ BOOST_AUTO_TEST_CASE(GetShieldedAvailableCredit) // 2) Confirm the tx SaplingMerkleTree tree; - FakeBlock fakeBlock = SimpleFakeMine(wtxUpdated, tree); + FakeBlock fakeBlock = SimpleFakeMine(wtxUpdated, tree, wallet); // Simulate receiving a new block and updating the witnesses/nullifiers wallet.IncrementNoteWitnesses(fakeBlock.pindex, &fakeBlock.block, tree); wallet.GetSaplingScriptPubKeyMan()->UpdateSaplingNullifierNoteMapForBlock(&fakeBlock.block); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index fc5595be3586..44d75e16dd80 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -320,7 +320,7 @@ void removeTxFromMempool(CWalletTx& wtx) /** * Mimic block creation. */ -CBlockIndex* SimpleFakeMine(CWalletTx& wtx, CBlockIndex* pprev = nullptr) +CBlockIndex* SimpleFakeMine(CWalletTx& wtx, CWallet &wallet, CBlockIndex* pprev = nullptr) { CBlock block; block.vtx.emplace_back(wtx.tx); @@ -332,6 +332,7 @@ CBlockIndex* SimpleFakeMine(CWalletTx& wtx, CBlockIndex* pprev = nullptr) fakeIndex->phashBlock = &mapBlockIndex.find(block.GetHash())->first; chainActive.SetTip(fakeIndex); BOOST_CHECK(chainActive.Contains(fakeIndex)); + WITH_LOCK(wallet.cs_wallet, wallet.SetLastBlockProcessed(fakeIndex)); wtx.SetConf(CWalletTx::Status::CONFIRMED, fakeIndex->GetBlockHash(), 0); removeTxFromMempool(wtx); wtx.fInMempool = false; @@ -442,7 +443,7 @@ BOOST_AUTO_TEST_CASE(cached_balances_tests) BOOST_CHECK_EQUAL(wallet.GetUnconfirmedBalance(), nCredit); // 2) Confirm tx and verify - SimpleFakeMine(wtxCredit); + SimpleFakeMine(wtxCredit, wallet); BOOST_CHECK_EQUAL(wallet.GetUnconfirmedBalance(), 0); BOOST_CHECK_EQUAL(wtxCredit.GetAvailableCredit(), nCredit); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 607914e2717f..09e03758c8f5 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -644,8 +644,22 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface bool IsHDEnabled() const; //! Whether the wallet supports Sapling or not // bool IsSaplingUpgradeEnabled() const; - //! Return the height of the last processed block - int GetLastBlockHeight() const { return m_last_block_processed_height; } + + /** Get last block processed height */ + int GetLastBlockHeight() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) + { + AssertLockHeld(cs_wallet); + assert(m_last_block_processed_height >= 0); + return m_last_block_processed_height; + }; + /** Set last block processed height, currently only use in unit test */ + void SetLastBlockProcessed(const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) + { + AssertLockHeld(cs_wallet); + m_last_block_processed_height = pindex->nHeight; + m_last_block_processed = pindex->GetBlockHash(); + m_last_block_processed_time = pindex->GetBlockTime(); + }; /* SPKM Helpers */ const CKeyingMaterial& GetEncryptionKey() const; From 4405ac076d34d9949df5df6d38b1986ec3e63305 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Feb 2021 19:24:58 -0300 Subject: [PATCH 12/63] Replace CWalletTx::SetConf by Confirmation initialization list Adaptation of btc@9700fcb47feca9d78e005b8d18b41148c8f6b25f --- src/test/librust/sapling_rpc_wallet_tests.cpp | 2 +- src/test/librust/sapling_wallet_tests.cpp | 14 +++---- .../test/wallet_shielded_balances_tests.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/wallet.cpp | 42 +++++++------------ src/wallet/wallet.h | 13 +++--- 6 files changed, 31 insertions(+), 44 deletions(-) diff --git a/src/test/librust/sapling_rpc_wallet_tests.cpp b/src/test/librust/sapling_rpc_wallet_tests.cpp index 8ddc53dca318..7e0a49f7617d 100644 --- a/src/test/librust/sapling_rpc_wallet_tests.cpp +++ b/src/test/librust/sapling_rpc_wallet_tests.cpp @@ -433,7 +433,7 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_taddr_to_sapling) chainActive.SetTip(&fakeIndex); BOOST_CHECK(chainActive.Contains(&fakeIndex)); BOOST_CHECK_EQUAL(1, chainActive.Height()); - wtx.SetConf(CWalletTx::Status::CONFIRMED, blockHash, 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, blockHash, 0); pwalletMain->LoadToWallet(wtx); BOOST_CHECK_MESSAGE(pwalletMain->GetAvailableBalance() > 0, "tx not confirmed"); diff --git a/src/test/librust/sapling_wallet_tests.cpp b/src/test/librust/sapling_wallet_tests.cpp index 667372b5aaee..4621e487159b 100644 --- a/src/test/librust/sapling_wallet_tests.cpp +++ b/src/test/librust/sapling_wallet_tests.cpp @@ -232,7 +232,7 @@ BOOST_AUTO_TEST_CASE(GetConflictedSaplingNotes) { auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); - wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); BOOST_CHECK(wallet.LoadToWallet(wtx)); // Simulate receiving new block and ChainTip signal @@ -351,7 +351,7 @@ BOOST_AUTO_TEST_CASE(SaplingNullifierIsSpent) { BOOST_CHECK(chainActive.Contains(&fakeIndex)); BOOST_CHECK_EQUAL(0, chainActive.Height()); - wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); wallet.LoadToWallet(wtx); // Verify note has been spent @@ -414,7 +414,7 @@ BOOST_AUTO_TEST_CASE(NavigateFromSaplingNullifierToNote) { BOOST_CHECK_EQUAL(0, chainActive.Height()); // Simulate SyncTransaction which calls AddToWalletIfInvolvingMe - wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); @@ -512,7 +512,7 @@ BOOST_AUTO_TEST_CASE(SpentSaplingNoteIsFromMe) { auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); - wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); wallet.LoadToWallet(wtx); // Simulate receiving new block and ChainTip signal. @@ -588,7 +588,7 @@ BOOST_AUTO_TEST_CASE(SpentSaplingNoteIsFromMe) { auto saplingNoteData2 = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx2.tx).first; BOOST_CHECK(saplingNoteData2.size() > 0); wtx2.SetSaplingNoteData(saplingNoteData2); - wtx2.SetConf(CWalletTx::Status::CONFIRMED, block2.GetHash(), 0); + wtx2.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block2.GetHash(), 0); wallet.LoadToWallet(wtx2); // Verify note B is spent. AddToWallet invokes AddToSpends which updates mapTxSaplingNullifiers @@ -967,7 +967,7 @@ BOOST_AUTO_TEST_CASE(UpdatedSaplingNoteData) { auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() == 1); // wallet only has key for change output wtx.SetSaplingNoteData(saplingNoteData); - wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); wallet.LoadToWallet(wtx); // Simulate receiving new block and ChainTip signal @@ -1083,7 +1083,7 @@ BOOST_AUTO_TEST_CASE(MarkAffectedSaplingTransactionsDirty) { auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); - wtx.SetConf(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); wallet.LoadToWallet(wtx); // Simulate receiving new block and ChainTip signal diff --git a/src/wallet/test/wallet_shielded_balances_tests.cpp b/src/wallet/test/wallet_shielded_balances_tests.cpp index 628753a54d2b..5cd4b96374b7 100644 --- a/src/wallet/test/wallet_shielded_balances_tests.cpp +++ b/src/wallet/test/wallet_shielded_balances_tests.cpp @@ -307,7 +307,7 @@ FakeBlock SimpleFakeMine(CWalletTx& wtx, SaplingMerkleTree& currentTree, CWallet chainActive.SetTip(fakeBlock.pindex); BOOST_CHECK(chainActive.Contains(fakeBlock.pindex)); WITH_LOCK(wallet.cs_wallet, wallet.SetLastBlockProcessed(fakeBlock.pindex)); - wtx.SetConf(CWalletTx::Status::CONFIRMED, fakeBlock.pindex->GetBlockHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeBlock.pindex->GetBlockHash(), 0); return fakeBlock; } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 44d75e16dd80..27578bd0119b 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -333,7 +333,7 @@ CBlockIndex* SimpleFakeMine(CWalletTx& wtx, CWallet &wallet, CBlockIndex* pprev chainActive.SetTip(fakeIndex); BOOST_CHECK(chainActive.Contains(fakeIndex)); WITH_LOCK(wallet.cs_wallet, wallet.SetLastBlockProcessed(fakeIndex)); - wtx.SetConf(CWalletTx::Status::CONFIRMED, fakeIndex->GetBlockHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex->GetBlockHash(), 0); removeTxFromMempool(wtx); wtx.fInMempool = false; return fakeIndex; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index f9460f2e3df1..f308c5c6ddd3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1055,19 +1055,19 @@ void CWallet::AddExternalNotesDataToTx(CWalletTx& wtx) const * Abandoned state should probably be more carefully tracked via different * posInBlock signals or by checking mempool presence when necessary. */ -bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Status status, const uint256& blockHash, int posInBlock, bool fUpdate) +bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CWalletTx::Confirmation& confirm, bool fUpdate) { const CTransaction& tx = *ptx; { AssertLockHeld(cs_wallet); - if (!blockHash.IsNull() && !tx.HasZerocoinSpendInputs() && !tx.IsCoinBase()) { + if (!confirm.hashBlock.IsNull() && !tx.HasZerocoinSpendInputs() && !tx.IsCoinBase()) { for (const CTxIn& txin : tx.vin) { std::pair range = mapTxSpends.equal_range(txin.prevout); while (range.first != range.second) { if (range.first->second != tx.GetHash()) { - LogPrintf("Transaction %s (in block %s) conflicts with wallet transaction %s (both spend %s:%i)\n", tx.GetHash().ToString(), blockHash.ToString(), range.first->second.ToString(), range.first->first.hash.ToString(), range.first->first.n); - MarkConflicted(blockHash, range.first->second); + LogPrintf("Transaction %s (in block %s) conflicts with wallet transaction %s (both spend %s:%i)\n", tx.GetHash().ToString(), confirm.hashBlock.ToString(), range.first->second.ToString(), range.first->first.hash.ToString(), range.first->first.n); + MarkConflicted(confirm.hashBlock, range.first->second); } range.first++; } @@ -1111,7 +1111,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::St // Block disconnection override an abandoned tx as unconfirmed // which means user may have to call abandontransaction again - wtx.SetConf(status, blockHash, posInBlock); + wtx.m_confirm = confirm; return AddToWallet(wtx, false); } @@ -1242,13 +1242,9 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) } } -void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Status status, const CBlockIndex *pindexBlockConnected, int posInBlock) +void CWallet::SyncTransaction(const CTransactionRef& ptx, const CWalletTx::Confirmation& confirm) { - if (!AddToWalletIfInvolvingMe(ptx, - status, - (pindexBlockConnected) ? pindexBlockConnected->GetBlockHash() : uint256(), - posInBlock, - true)) { + if (!AddToWalletIfInvolvingMe(ptx, confirm, true)) { return; // Not one of ours } @@ -1258,7 +1254,8 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Status stat void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) { LOCK2(cs_main, cs_wallet); - SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, nullptr, 0); + CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, {}, 0); + SyncTransaction(ptx, confirm); auto it = mapWallet.find(ptx->GetHash()); if (it != mapWallet.end()) { @@ -1282,7 +1279,8 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const m_last_block_processed_time = pindex->GetBlockTime(); m_last_block_processed_height = pindex->nHeight; for (size_t i = 0; i < pblock->vtx.size(); i++) { - SyncTransaction(pblock->vtx[i], CWalletTx::Status::CONFIRMED, pindex, i); + CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, m_last_block_processed->GetBlockHash(), i); + SyncTransaction(pblock->vtx[i], confirm); TransactionRemovedFromMempool(pblock->vtx[i]); } for (const CTransactionRef& ptx : vtxConflicted) { @@ -1318,7 +1316,8 @@ void CWallet::BlockDisconnected(const std::shared_ptr& pblock, con m_last_block_processed_time = blockTime; m_last_block_processed = blockHash; for (const CTransactionRef& ptx : pblock->vtx) { - SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, nullptr, 0); + CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, {}, 0); + SyncTransaction(ptx, confirm); } if (Params().GetConsensus().NetworkUpgradeActive(nBlockHeight, Consensus::UPGRADE_V5_0)) { @@ -1849,7 +1848,8 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate, b } for (int posInBlock = 0; posInBlock < (int) block.vtx.size(); posInBlock++) { const auto& tx = block.vtx[posInBlock]; - if (AddToWalletIfInvolvingMe(tx, CWalletTx::Status::CONFIRMED, pindex->GetBlockHash(), posInBlock, fUpdate)) { + CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, pindex->GetBlockHash(), posInBlock); + if (AddToWalletIfInvolvingMe(tx, confirm, fUpdate)) { myTxHashes.push_back(tx->GetHash()); ret++; } @@ -4299,18 +4299,6 @@ CWalletKey::CWalletKey(int64_t nExpires) nTimeExpires = nExpires; } -void CWalletTx::SetConf(Status status, const uint256& blockHash, int posInBlock) -{ - // Update tx status - m_confirm.status = status; - - // Update the tx's hashBlock - m_confirm.hashBlock = blockHash; - - // set the position of the transaction in the block - m_confirm.nIndex = posInBlock; -} - int CWalletTx::GetDepthInMainChain() const { if (isUnconfirmed() || isAbandoned()) return 0; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 09e03758c8f5..5c5a297d23e0 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -369,9 +369,10 @@ class CWalletTx * where they instead point to block hash and index of the deepest conflicting tx. */ struct Confirmation { - Status status = UNCONFIRMED; - uint256 hashBlock = uint256(); - int nIndex = 0; + Status status; + uint256 hashBlock; + int nIndex; + Confirmation(Status s = UNCONFIRMED, uint256 h = uint256(), int i = 0) : status(s), hashBlock(h), nIndex(i) {} }; Confirmation m_confirm; @@ -512,8 +513,6 @@ class CWalletTx void RelayWalletTransaction(CConnman* connman); std::set GetConflicts() const; - void SetConf(Status status, const uint256& blockHash, int posInBlock); - /** * Return depth of transaction in blockchain: * <0 : conflicts with a transaction this deep in the blockchain @@ -610,7 +609,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface void ChainTipAdded(const CBlockIndex *pindex, const CBlock *pblock, SaplingMerkleTree saplingTree); /* Used by TransactionAddedToMemorypool/BlockConnected/Disconnected */ - void SyncTransaction(const CTransactionRef& tx, CWalletTx::Status status, const CBlockIndex *pindexBlockConnected, int posInBlock); + void SyncTransaction(const CTransactionRef& tx, const CWalletTx::Confirmation& confirm); bool IsKeyUsed(const CPubKey& vchPubKey); @@ -928,7 +927,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface void TransactionAddedToMempool(const CTransactionRef& tx) override; void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) override; void BlockDisconnected(const std::shared_ptr& pblock, const uint256& blockHash, int nBlockHeight, int64_t blockTime) override; - bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, CWalletTx::Status status, const uint256& blockHash, int posInBlock, bool fUpdate); + bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, const CWalletTx::Confirmation& confirm, bool fUpdate); void EraseFromWallet(const uint256& hash); /** From 8aa2d31d9832e069971fd2fb90bd8ad2aef8bd1c Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Feb 2021 20:13:04 -0300 Subject: [PATCH 13/63] Add block_height field in struct Confirmation At wallet loading, we rely on chain state querying to retrieve height of txn, to do so we ensure that lock order is respected between cs_main and cs_wallet. If wallet loaded is the wallet-tool one, all wallet txn will show up with a height of zero. It doesn't matter as confirmation height is not used by wallet-tool. Reorder arguments and document Confirmation calls to avoid ambiguity. Adaptation of btc@5971d3848e09abf571e5308185275296127efca4 --- src/test/librust/sapling_rpc_wallet_tests.cpp | 2 +- src/test/librust/sapling_wallet_tests.cpp | 14 ++--- .../test/wallet_shielded_balances_tests.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/wallet.cpp | 51 ++++++++++++++----- src/wallet/wallet.h | 15 +++--- 6 files changed, 56 insertions(+), 30 deletions(-) diff --git a/src/test/librust/sapling_rpc_wallet_tests.cpp b/src/test/librust/sapling_rpc_wallet_tests.cpp index 7e0a49f7617d..c3901aaaf39f 100644 --- a/src/test/librust/sapling_rpc_wallet_tests.cpp +++ b/src/test/librust/sapling_rpc_wallet_tests.cpp @@ -433,7 +433,7 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_taddr_to_sapling) chainActive.SetTip(&fakeIndex); BOOST_CHECK(chainActive.Contains(&fakeIndex)); BOOST_CHECK_EQUAL(1, chainActive.Height()); - wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, blockHash, 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, blockHash, 0); pwalletMain->LoadToWallet(wtx); BOOST_CHECK_MESSAGE(pwalletMain->GetAvailableBalance() > 0, "tx not confirmed"); diff --git a/src/test/librust/sapling_wallet_tests.cpp b/src/test/librust/sapling_wallet_tests.cpp index 4621e487159b..c35dd0a9fd78 100644 --- a/src/test/librust/sapling_wallet_tests.cpp +++ b/src/test/librust/sapling_wallet_tests.cpp @@ -232,7 +232,7 @@ BOOST_AUTO_TEST_CASE(GetConflictedSaplingNotes) { auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); - wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, block.GetHash(), 0); BOOST_CHECK(wallet.LoadToWallet(wtx)); // Simulate receiving new block and ChainTip signal @@ -351,7 +351,7 @@ BOOST_AUTO_TEST_CASE(SaplingNullifierIsSpent) { BOOST_CHECK(chainActive.Contains(&fakeIndex)); BOOST_CHECK_EQUAL(0, chainActive.Height()); - wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, 0, block.GetHash(), 0); wallet.LoadToWallet(wtx); // Verify note has been spent @@ -414,7 +414,7 @@ BOOST_AUTO_TEST_CASE(NavigateFromSaplingNullifierToNote) { BOOST_CHECK_EQUAL(0, chainActive.Height()); // Simulate SyncTransaction which calls AddToWalletIfInvolvingMe - wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, block.GetHash(), 0); auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); @@ -512,7 +512,7 @@ BOOST_AUTO_TEST_CASE(SpentSaplingNoteIsFromMe) { auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); - wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, block.GetHash(), 0); wallet.LoadToWallet(wtx); // Simulate receiving new block and ChainTip signal. @@ -588,7 +588,7 @@ BOOST_AUTO_TEST_CASE(SpentSaplingNoteIsFromMe) { auto saplingNoteData2 = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx2.tx).first; BOOST_CHECK(saplingNoteData2.size() > 0); wtx2.SetSaplingNoteData(saplingNoteData2); - wtx2.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block2.GetHash(), 0); + wtx2.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex2.nHeight, block2.GetHash(), 0); wallet.LoadToWallet(wtx2); // Verify note B is spent. AddToWallet invokes AddToSpends which updates mapTxSaplingNullifiers @@ -967,7 +967,7 @@ BOOST_AUTO_TEST_CASE(UpdatedSaplingNoteData) { auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() == 1); // wallet only has key for change output wtx.SetSaplingNoteData(saplingNoteData); - wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, block.GetHash(), 0); wallet.LoadToWallet(wtx); // Simulate receiving new block and ChainTip signal @@ -1083,7 +1083,7 @@ BOOST_AUTO_TEST_CASE(MarkAffectedSaplingTransactionsDirty) { auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; BOOST_CHECK(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); - wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, block.GetHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, block.GetHash(), 0); wallet.LoadToWallet(wtx); // Simulate receiving new block and ChainTip signal diff --git a/src/wallet/test/wallet_shielded_balances_tests.cpp b/src/wallet/test/wallet_shielded_balances_tests.cpp index 5cd4b96374b7..caaa1c61b0ef 100644 --- a/src/wallet/test/wallet_shielded_balances_tests.cpp +++ b/src/wallet/test/wallet_shielded_balances_tests.cpp @@ -307,7 +307,7 @@ FakeBlock SimpleFakeMine(CWalletTx& wtx, SaplingMerkleTree& currentTree, CWallet chainActive.SetTip(fakeBlock.pindex); BOOST_CHECK(chainActive.Contains(fakeBlock.pindex)); WITH_LOCK(wallet.cs_wallet, wallet.SetLastBlockProcessed(fakeBlock.pindex)); - wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeBlock.pindex->GetBlockHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeBlock.pindex->nHeight, fakeBlock.pindex->GetBlockHash(), 0); return fakeBlock; } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 27578bd0119b..2564791b76d7 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -333,7 +333,7 @@ CBlockIndex* SimpleFakeMine(CWalletTx& wtx, CWallet &wallet, CBlockIndex* pprev chainActive.SetTip(fakeIndex); BOOST_CHECK(chainActive.Contains(fakeIndex)); WITH_LOCK(wallet.cs_wallet, wallet.SetLastBlockProcessed(fakeIndex)); - wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex->GetBlockHash(), 0); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex->nHeight, fakeIndex->GetBlockHash(), 0); removeTxFromMempool(wtx); wtx.fInMempool = false; return fakeIndex; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index f308c5c6ddd3..30b48434a7f3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -930,11 +930,13 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) wtx.m_confirm.status = wtxIn.m_confirm.status; wtx.m_confirm.nIndex = wtxIn.m_confirm.nIndex; wtx.m_confirm.hashBlock = wtxIn.m_confirm.hashBlock; + wtx.m_confirm.block_height = wtxIn.m_confirm.block_height; wtx.UpdateTimeSmart(); fUpdated = true; } else { assert(wtx.m_confirm.nIndex == wtxIn.m_confirm.nIndex); assert(wtx.m_confirm.hashBlock == wtxIn.m_confirm.hashBlock); + assert(wtx.m_confirm.block_height == wtxIn.m_confirm.block_height); } if (HasSaplingSPKM() && m_sspk_man->UpdatedNoteData(wtxIn, wtx)) { fUpdated = true; @@ -971,17 +973,38 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) return true; } +// Internal function for now, this will be part of a chain interface class in the future. +Optional getTipBlockHeight(const uint256& hash) +{ + auto it = mapBlockIndex.find(hash); + CBlockIndex* pindex = it == mapBlockIndex.end() ? nullptr : it->second; + if (pindex && chainActive.Contains(pindex)) { + return Optional(pindex->nHeight); + } + return nullopt; +} + bool CWallet::LoadToWallet(CWalletTx& wtxIn) { LOCK2(cs_main, cs_wallet); // If tx hasn't been reorged out of chain while wallet being shutdown // change tx status to UNCONFIRMED and reset hashBlock/nIndex. if (!wtxIn.m_confirm.hashBlock.IsNull()) { - auto it = mapBlockIndex.find(wtxIn.m_confirm.hashBlock); - CBlockIndex* pindex = it == mapBlockIndex.end() ? nullptr : it->second; - if (!pindex || !chainActive.Contains(pindex)) { + Optional block_height = getTipBlockHeight(wtxIn.m_confirm.hashBlock); + if (block_height) { + // Update cached block height variable since it not stored in the + // serialized transaction. + wtxIn.m_confirm.block_height = *block_height; + } else if (wtxIn.isConflicted() || wtxIn.isConfirmed()) { + // If tx block (or conflicting block) was reorged out of chain + // while the wallet was shutdown, change tx status to UNCONFIRMED + // and reset block height, hash, and index. ABANDONED tx don't have + // associated blocks and don't need to be updated. The case where a + // transaction was reorged out while online and then reconfirmed + // while offline is covered by the rescan logic. wtxIn.setUnconfirmed(); wtxIn.m_confirm.hashBlock = UINT256_ZERO; + wtxIn.m_confirm.block_height = 0; wtxIn.m_confirm.nIndex = 0; } } @@ -997,7 +1020,7 @@ bool CWallet::LoadToWallet(CWalletTx& wtxIn) if (it != mapWallet.end()) { CWalletTx& prevtx = it->second; if (prevtx.isConflicted()) { - MarkConflicted(prevtx.m_confirm.hashBlock, wtx.GetHash()); + MarkConflicted(prevtx.m_confirm.hashBlock, prevtx.m_confirm.block_height, wtx.GetHash()); } } } @@ -1067,7 +1090,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CWallet while (range.first != range.second) { if (range.first->second != tx.GetHash()) { LogPrintf("Transaction %s (in block %s) conflicts with wallet transaction %s (both spend %s:%i)\n", tx.GetHash().ToString(), confirm.hashBlock.ToString(), range.first->second.ToString(), range.first->first.hash.ToString(), range.first->first.n); - MarkConflicted(confirm.hashBlock, range.first->second); + MarkConflicted(confirm.hashBlock, confirm.block_height, range.first->second); } range.first++; } @@ -1152,7 +1175,6 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) if (currentconfirm == 0 && !wtx.isAbandoned()) { // If the orig tx was not in block/mempool, none of its spends can be in mempool assert(!wtx.InMempool()); - wtx.m_confirm.nIndex = 0; wtx.setAbandoned(); wtx.MarkDirty(); walletdb.WriteTx(wtx); @@ -1179,7 +1201,7 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) return true; } -void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) +void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, const uint256& hashTx) { LOCK2(cs_main, cs_wallet); @@ -1219,6 +1241,7 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) // Mark transaction as conflicted with this block. wtx.m_confirm.nIndex = 0; wtx.m_confirm.hashBlock = hashBlock; + wtx.m_confirm.block_height = conflicting_height; wtx.setConflicted(); wtx.MarkDirty(); walletdb.WriteTx(wtx); @@ -1254,7 +1277,7 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, const CWalletTx::Confi void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) { LOCK2(cs_main, cs_wallet); - CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, {}, 0); + CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, {}, /* nIndex */ 0); SyncTransaction(ptx, confirm); auto it = mapWallet.find(ptx->GetHash()); @@ -1278,10 +1301,10 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const m_last_block_processed = pindex->GetBlockHash(); m_last_block_processed_time = pindex->GetBlockTime(); m_last_block_processed_height = pindex->nHeight; - for (size_t i = 0; i < pblock->vtx.size(); i++) { - CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, m_last_block_processed->GetBlockHash(), i); - SyncTransaction(pblock->vtx[i], confirm); - TransactionRemovedFromMempool(pblock->vtx[i]); + for (size_t index = 0; index < pblock->vtx.size(); index++) { + CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, m_last_block_processed_height, m_last_block_processed->GetBlockHash(), index); + SyncTransaction(pblock->vtx[index], confirm); + TransactionRemovedFromMempool(pblock->vtx[index]); } for (const CTransactionRef& ptx : vtxConflicted) { TransactionRemovedFromMempool(ptx); @@ -1316,7 +1339,7 @@ void CWallet::BlockDisconnected(const std::shared_ptr& pblock, con m_last_block_processed_time = blockTime; m_last_block_processed = blockHash; for (const CTransactionRef& ptx : pblock->vtx) { - CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, {}, 0); + CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, {}, /* nIndex */ 0); SyncTransaction(ptx, confirm); } @@ -1848,7 +1871,7 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate, b } for (int posInBlock = 0; posInBlock < (int) block.vtx.size(); posInBlock++) { const auto& tx = block.vtx[posInBlock]; - CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, pindex->GetBlockHash(), posInBlock); + CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, pindex->nHeight, pindex->GetBlockHash(), posInBlock); if (AddToWalletIfInvolvingMe(tx, confirm, fUpdate)) { myTxHashes.push_back(tx->GetHash()); ret++; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 5c5a297d23e0..efd1ba1af0d0 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -364,15 +364,17 @@ class CWalletTx ABANDONED }; - /* Confirmation includes tx status and a pair of {block hash/tx index in block} at which tx has been confirmed. - * This pair is both 0 if tx hasn't confirmed yet. Meaning of these fields changes with CONFLICTED state - * where they instead point to block hash and index of the deepest conflicting tx. + /* Confirmation includes tx status and a triplet of {block height/block hash/tx index in block} + * at which tx has been confirmed. All three are set to 0 if tx is unconfirmed or abandoned. + * Meaning of these fields changes with CONFLICTED state where they instead point to block hash + * and block height of the deepest conflicting tx. */ struct Confirmation { Status status; + int block_height; uint256 hashBlock; int nIndex; - Confirmation(Status s = UNCONFIRMED, uint256 h = uint256(), int i = 0) : status(s), hashBlock(h), nIndex(i) {} + Confirmation(Status s = UNCONFIRMED, int b = 0, const uint256& h = UINT256_ZERO, int i = 0) : status(s), block_height(b), hashBlock(h), nIndex(i) {} }; Confirmation m_confirm; @@ -423,7 +425,6 @@ class CWalletTx * compatibility (pre-commit 9ac63d6). */ if (serializedIndex == -1 && m_confirm.hashBlock == ABANDON_HASH) { - m_confirm.hashBlock = uint256(); setAbandoned(); } else if (serializedIndex == -1) { setConflicted(); @@ -528,12 +529,14 @@ class CWalletTx { m_confirm.status = CWalletTx::ABANDONED; m_confirm.hashBlock = UINT256_ZERO; + m_confirm.block_height = 0; m_confirm.nIndex = 0; } bool isConflicted() const { return m_confirm.status == CWalletTx::CONFLICTED; } void setConflicted() { m_confirm.status = CWalletTx::CONFLICTED; } bool isUnconfirmed() const { return m_confirm.status == CWalletTx::UNCONFIRMED; } void setUnconfirmed() { m_confirm.status = CWalletTx::UNCONFIRMED; } + bool isConfirmed() const { return m_confirm.status == CWalletTx::CONFIRMED; } void setConfirmed() { m_confirm.status = CWalletTx::CONFIRMED; } const uint256& GetHash() const { return tx->GetHash(); } @@ -602,7 +605,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface void AddToSpends(const uint256& wtxid); /* Mark a transaction (and its in-wallet descendants) as conflicting with a particular block. */ - void MarkConflicted(const uint256& hashBlock, const uint256& hashTx); + void MarkConflicted(const uint256& hashBlock, int conflicting_height, const uint256& hashTx); template void SyncMetaData(std::pair::iterator, typename TxSpendMap::iterator> range); From 9adeb6148624b834b5bd859824dbe8cb573729f1 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Feb 2021 20:21:26 -0300 Subject: [PATCH 14/63] Only return early from BlockUntilSyncedToCurrentChain if current tip is exact match In the next commit, we start using BlockConnected/BlockDisconnected callbacks to establish tx depth, rather than querying the chain directly. Currently, BlockUntilSyncedToCurrentChain will return early if the best block processed by the wallet is a descendant of the node'tip. That means that in the case of a re-org, it won't wait for the BlockDisconnected callbacks that have been enqueued during the re-org but have not yet been triggered in the wallet. Change BlockUntilSyncedToCurrentChain to only return early if the wallet's m_last_block_processed matches the tip exactly. This ensures that there are no BlockDisconnected or BlockConnected callbacks in-flight. Adaptation of btc@f77b1de16feee097a88e99d2ecdd4d84beb4f915 --- src/wallet/wallet.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 30b48434a7f3..298e0743cef3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1363,12 +1363,9 @@ void CWallet::BlockUntilSyncedToCurrentChain() { uint256 last_block_hash = WITH_LOCK(cs_wallet, return m_last_block_processed); LOCK(cs_main); const CBlockIndex* initialChainTip = chainActive.Tip(); - - if (!last_block_hash.IsNull()) { - auto it = mapBlockIndex.find(last_block_hash); - if (it == mapBlockIndex.end() || it->second->GetAncestor(initialChainTip->nHeight) == initialChainTip) { + if (!last_block_hash.IsNull() && initialChainTip && + last_block_hash == initialChainTip->GetBlockHash()) { return; - } } } From 1386ab74933071600b53b6b37501239ce9acff71 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Feb 2021 20:23:32 -0300 Subject: [PATCH 15/63] Use CWallet::m_last_block_processed_height in GetDepthInMainChain Avoid to lock chain to query state thanks to tracking last block height in CWallet. Adaptation of btc@0ff03871add000f8b4d8f82aeb168eed2fc9dc5f --- src/wallet/wallet.cpp | 19 +++---------------- src/wallet/wallet.h | 8 +++++++- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 298e0743cef3..73bde9833260 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4321,24 +4321,11 @@ CWalletKey::CWalletKey(int64_t nExpires) int CWalletTx::GetDepthInMainChain() const { + assert(pwallet != nullptr); + AssertLockHeld(pwallet->cs_wallet); if (isUnconfirmed() || isAbandoned()) return 0; - AssertLockHeld(cs_main); - int nResult; - - // Find the block it claims to be in - BlockMap::iterator mi = mapBlockIndex.find(m_confirm.hashBlock); - if (mi == mapBlockIndex.end()) { - nResult = 0; - } else { - CBlockIndex* pindex = (*mi).second; - if (!pindex || !chainActive.Contains(pindex)) { - nResult = 0; - } else { - nResult = (isConflicted() ? (-1) : 1) * (chainActive.Height() - pindex->nHeight + 1); - } - } - return nResult; + return (pwallet->GetLastBlockHeight() - m_confirm.block_height + 1) * (isConflicted() ? -1 : 1); } int CWalletTx::GetBlocksToMaturity() const diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index efd1ba1af0d0..65e9167c89c6 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -520,7 +520,13 @@ class CWalletTx * 0 : in memory pool, waiting to be included in a block * >=1 : this many blocks deep in the main chain */ - int GetDepthInMainChain() const; + // TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct + // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The annotation + // "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid having to + // resolve the issue of member access into incomplete type CWallet. Note + // that we still have the runtime check "AssertLockHeld(pwallet->cs_wallet)" + // in place. + int GetDepthInMainChain() const NO_THREAD_SAFETY_ANALYSIS; bool IsInMainChainImmature() const; int GetBlocksToMaturity() const; From 239d6a2017f8148d6a61612e0ef9ec05ea0798d3 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Feb 2021 20:51:35 -0300 Subject: [PATCH 16/63] wallet: remove the now unneeded cs_main locks --- src/wallet/wallet.cpp | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 73bde9833260..8ea54379b70b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1144,7 +1144,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CWallet bool CWallet::AbandonTransaction(const uint256& hashTx) { - LOCK2(cs_main, cs_wallet); + LOCK(cs_wallet); CWalletDB walletdb(*dbw, "r+"); @@ -1590,7 +1590,6 @@ CAmount CWalletTx::GetCredit(const isminefilter& filter, bool recalculate) const CAmount CWalletTx::GetImmatureCredit(bool fUseCache, const isminefilter& filter) const { - LOCK(cs_main); if (IsInMainChainImmature()) { return GetCachableAmount(IMMATURE_CREDIT, filter, !fUseCache); } @@ -1692,7 +1691,6 @@ CAmount CWalletTx::GetLockedCredit() const CAmount CWalletTx::GetImmatureWatchOnlyCredit(const bool& fUseCache) const { - LOCK(cs_main); if (IsInMainChainImmature()) { return GetCachableAmount(IMMATURE_CREDIT, ISMINE_WATCH_ONLY, !fUseCache); } @@ -2059,7 +2057,7 @@ CAmount CWallet::loopTxsBalance(std::function& vCoins) const { vCoins.clear(); { - LOCK2(cs_main, cs_wallet); + LOCK(cs_wallet); for (const auto& it : mapWallet) { const uint256& wtxid = it.first; const CWalletTx* pcoin = &it.second; @@ -2277,7 +2275,6 @@ void CWallet::GetAvailableP2CSCoins(std::vector& vCoins) const { */ bool CheckTXAvailability(const CWalletTx* pcoin, bool fOnlyConfirmed, int& nDepth) { - AssertLockHeld(cs_main); if (!CheckFinalTx(pcoin->tx)) return false; if (fOnlyConfirmed && !pcoin->IsTrusted()) return false; if (pcoin->GetBlocksToMaturity() > 0) return false; @@ -2333,13 +2330,11 @@ bool CWallet::GetMasternodeVinAndKeys(CTxIn& txinRet, CPubKey& pubKeyRet, CKey& // Check availability int nDepth = 0; - { - LOCK(cs_main); - if (!CheckTXAvailability(wtx, true, nDepth)) { - strError = "Not available collateral transaction"; - return error("%s: tx %s not available", __func__, strTxHash); - } + if (!CheckTXAvailability(wtx, true, nDepth)) { + strError = "Not available collateral transaction"; + return error("%s: tx %s not available", __func__, strTxHash); } + // Skip spent coins if (IsSpent(txHash, nOutputIndex)) { strError = "Error: collateral already spent"; @@ -2435,7 +2430,7 @@ bool CWallet::AvailableCoins(std::vector* pCoins, // --> populates coinsFilter.fIncludeDelegated = true; { - LOCK2(cs_main, cs_wallet); + LOCK(cs_wallet); for (std::map::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) { const uint256& wtxid = it->first; const CWalletTx* pcoin = &(*it).second; @@ -4330,7 +4325,6 @@ int CWalletTx::GetDepthInMainChain() const int CWalletTx::GetBlocksToMaturity() const { - LOCK(cs_main); if (!(IsCoinBase() || IsCoinStake())) return 0; return std::max(0, (Params().GetConsensus().nCoinbaseMaturity + 1) - GetDepthInMainChain()); From 1bd97ca5cdae0ba40471b1f82a86bd51ea3395c1 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Feb 2021 20:53:40 -0300 Subject: [PATCH 17/63] wallet::MarkConflicted remove blockIndex and there by cs_main lock dependency. --- src/wallet/wallet.cpp | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8ea54379b70b..09d64ac23779 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1203,16 +1203,9 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, const uint256& hashTx) { - LOCK2(cs_main, cs_wallet); - - int conflictconfirms = 0; - if (mapBlockIndex.count(hashBlock)) { - CBlockIndex* pindex = mapBlockIndex[hashBlock]; - if (chainActive.Contains(pindex)) { - conflictconfirms = -(chainActive.Height() - pindex->nHeight + 1); - } - } + LOCK(cs_wallet); + int conflictconfirms = (m_last_block_processed_height - conflicting_height + 1) * -1; // If number of conflict confirms cannot be determined, this means // that the block is still unknown or not yet part of the main chain, // for example when loading the wallet during a reindex. Do nothing in that From 5e06330cd7f6bf399311a85b2d699d7abd9e3030 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 12 Feb 2021 18:05:05 -0300 Subject: [PATCH 18/63] Removed IsFinalTx() cs_main lock requirement. --- src/consensus/tx_verify.cpp | 3 --- src/consensus/tx_verify.h | 2 +- src/wallet/rpcwallet.cpp | 18 ++++++++++-------- src/wallet/wallet.cpp | 13 ++++++++----- 4 files changed, 19 insertions(+), 17 deletions(-) diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index 272499d1c3b6..b569974da1ff 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -12,12 +12,9 @@ bool IsFinalTx(const CTransactionRef& tx, int nBlockHeight, int64_t nBlockTime) { - AssertLockHeld(cs_main); // Time based nLockTime implemented in 0.1.6 if (tx->nLockTime == 0) return true; - if (nBlockHeight == 0) - nBlockHeight = chainActive.Height(); if (nBlockTime == 0) nBlockTime = GetAdjustedTime(); if ((int64_t)tx->nLockTime < ((int64_t)tx->nLockTime < LOCKTIME_THRESHOLD ? (int64_t)nBlockHeight : nBlockTime)) diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h index 876f8d83fc95..bc54e1c9021b 100644 --- a/src/consensus/tx_verify.h +++ b/src/consensus/tx_verify.h @@ -39,6 +39,6 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& ma * Check if transaction is final and can be included in a block with the * specified height and time. Consensus critical. */ -bool IsFinalTx(const CTransactionRef& tx, int nBlockHeight = 0, int64_t nBlockTime = 0); +bool IsFinalTx(const CTransactionRef& tx, int nBlockHeight, int64_t nBlockTime = 0); #endif // BITCOIN_CONSENSUS_TX_VERIFY_H diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 2d013f27f65b..c2b20478a8c3 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1895,6 +1895,7 @@ UniValue getreceivedbyaddress(const JSONRPCRequest& request) pwalletMain->BlockUntilSyncedToCurrentChain(); LOCK2(cs_main, pwalletMain->cs_wallet); + int nBlockHeight = chainActive.Height(); // pivx address CTxDestination address = DecodeDestination(request.params[0].get_str()); @@ -1913,7 +1914,7 @@ UniValue getreceivedbyaddress(const JSONRPCRequest& request) CAmount nAmount = 0; for (std::map::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it) { const CWalletTx& wtx = (*it).second; - if (wtx.IsCoinBase() || !IsFinalTx(wtx.tx)) + if (wtx.IsCoinBase() || !IsFinalTx(wtx.tx, nBlockHeight)) continue; for (const CTxOut& txout : wtx.tx->vout) @@ -1955,6 +1956,7 @@ UniValue getreceivedbylabel(const JSONRPCRequest& request) pwalletMain->BlockUntilSyncedToCurrentChain(); LOCK2(cs_main, pwalletMain->cs_wallet); + int nBlockHeight = chainActive.Height(); // Minimum confirmations int nMinDepth = 1; @@ -1969,7 +1971,7 @@ UniValue getreceivedbylabel(const JSONRPCRequest& request) CAmount nAmount = 0; for (std::map::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it) { const CWalletTx& wtx = (*it).second; - if (wtx.IsCoinBase() || !IsFinalTx(wtx.tx)) + if (wtx.IsCoinBase() || !IsFinalTx(wtx.tx, nBlockHeight)) continue; for (const CTxOut& txout : wtx.tx->vout) { @@ -2302,7 +2304,7 @@ struct tallyitem { } }; -UniValue ListReceived(const UniValue& params, bool by_label) +UniValue ListReceived(const UniValue& params, bool by_label, int nBlockHeight) { // Minimum confirmations int nMinDepth = 1; @@ -2335,7 +2337,7 @@ UniValue ListReceived(const UniValue& params, bool by_label) for (std::map::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it) { const CWalletTx& wtx = (*it).second; - if (wtx.IsCoinBase() || !IsFinalTx(wtx.tx)) + if (wtx.IsCoinBase() || !IsFinalTx(wtx.tx, nBlockHeight)) continue; int nDepth = wtx.GetDepthInMainChain(); @@ -2477,8 +2479,8 @@ UniValue listreceivedbyaddress(const JSONRPCRequest& request) pwalletMain->BlockUntilSyncedToCurrentChain(); LOCK2(cs_main, pwalletMain->cs_wallet); - - return ListReceived(request.params, false); + int nBlockHeight = chainActive.Height(); + return ListReceived(request.params, false, nBlockHeight); } UniValue listreceivedbyshieldaddress(const JSONRPCRequest& request) @@ -2613,8 +2615,8 @@ UniValue listreceivedbylabel(const JSONRPCRequest& request) pwalletMain->BlockUntilSyncedToCurrentChain(); LOCK2(cs_main, pwalletMain->cs_wallet); - - return ListReceived(request.params, true); + int nBlockHeight = chainActive.Height(); + return ListReceived(request.params, true, nBlockHeight); } UniValue listcoldutxos(const JSONRPCRequest& request) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 09d64ac23779..6181a3392f60 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2191,7 +2191,7 @@ CAmount CWallet::GetLegacyBalance(const isminefilter& filter, int minDepth) cons const CWalletTx& wtx = entry.second; bool fConflicted; const int depth = wtx.GetDepthAndMempool(fConflicted); - if (!IsFinalTx(wtx.tx) || wtx.GetBlocksToMaturity() > 0 || depth < 0 || fConflicted) { + if (!IsFinalTx(wtx.tx, m_last_block_processed_height) || wtx.GetBlocksToMaturity() > 0 || depth < 0 || fConflicted) { continue; } @@ -3480,7 +3480,7 @@ std::map CWallet::GetAddressBalances() for (std::pair walletEntry : mapWallet) { CWalletTx* pcoin = &walletEntry.second; - if (!IsFinalTx(pcoin->tx) || !pcoin->IsTrusted()) + if (!IsFinalTx(pcoin->tx, m_last_block_processed_height) || !pcoin->IsTrusted()) continue; if (pcoin->IsCoinBase() && pcoin->GetBlocksToMaturity() > 0) @@ -4626,9 +4626,12 @@ bool CWalletTx::IsTrusted() const bool CWalletTx::IsTrusted(int& nDepth, bool& fConflicted) const { - // Quick answer in most cases - if (!IsFinalTx(tx)) - return false; + { + LOCK(pwallet->cs_wallet); // future: receive block height instead of locking here. + // Quick answer in most cases + if (!IsFinalTx(tx, pwallet->GetLastBlockHeight())) + return false; + } nDepth = GetDepthAndMempool(fConflicted); From 3dede64a4824d05634332506a6abd8fb7d8eecc0 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 12 Feb 2021 18:26:50 -0300 Subject: [PATCH 19/63] GUI: Remove cs_main lock from balance polling timer And avoid recomputing wallet balances and txes status unless a tx changed or a block tip notification was received. --- src/qt/clientmodel.cpp | 15 +++++++++++++++ src/qt/clientmodel.h | 3 +++ src/qt/pivx.cpp | 1 + src/qt/walletmodel.cpp | 39 ++++++++++++++++++++++++++++++--------- src/qt/walletmodel.h | 9 +++++++++ 5 files changed, 58 insertions(+), 9 deletions(-) diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp index 18559865bac1..00a4fcfb6f28 100644 --- a/src/qt/clientmodel.cpp +++ b/src/qt/clientmodel.cpp @@ -135,6 +135,21 @@ QString ClientModel::getLastBlockHash() const return QString::fromStdString(nHash.GetHex()); } +uint256 ClientModel::getLastBlockProcessed() const +{ + return cacheTip == nullptr ? Params().GenesisBlock().GetHash() : cacheTip->GetBlockHash(); +} + +int ClientModel::getLastBlockProcessedHeight() const +{ + return cacheTip == nullptr ? 0 : cacheTip->nHeight; +} + +int64_t ClientModel::getLastBlockProcessedTime() const +{ + return cacheTip == nullptr ? Params().GenesisBlock().GetBlockTime() : cacheTip->GetBlockTime(); +} + double ClientModel::getVerificationProgress() const { return Checkpoints::GuessVerificationProgress(cacheTip); diff --git a/src/qt/clientmodel.h b/src/qt/clientmodel.h index e2284aaf04bd..b7e7d2c0f402 100644 --- a/src/qt/clientmodel.h +++ b/src/qt/clientmodel.h @@ -64,6 +64,9 @@ class ClientModel : public QObject int getNumBlocks(); QDateTime getLastBlockDate() const; QString getLastBlockHash() const; + uint256 getLastBlockProcessed() const; + int getLastBlockProcessedHeight() const; + int64_t getLastBlockProcessedTime() const; double getVerificationProgress() const; quint64 getTotalBytesRecv() const; diff --git a/src/qt/pivx.cpp b/src/qt/pivx.cpp index c2b5d50732a0..6cc23c2b123c 100644 --- a/src/qt/pivx.cpp +++ b/src/qt/pivx.cpp @@ -486,6 +486,7 @@ void BitcoinApplication::initializeResult(int retval) #ifdef ENABLE_WALLET if (pwalletMain) { walletModel = new WalletModel(pwalletMain, optionsModel); + walletModel->setClientModel(clientModel); window->addWallet(PIVXGUI::DEFAULT_WALLET, walletModel); window->setCurrentWallet(PIVXGUI::DEFAULT_WALLET); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index bfb3d7673550..7467be53b138 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -7,6 +7,7 @@ #include "walletmodel.h" #include "addresstablemodel.h" +#include "qt/clientmodel.h" #include "guiconstants.h" #include "optionsmodel.h" #include "recentrequeststablemodel.h" @@ -208,29 +209,33 @@ void WalletModel::pollBalanceChanged() waitLonger = 0; } + // Avoid recomputing wallet balances unless a tx changed or + // BlockTip notification was received. + if (!fForceCheckBalanceChanged && m_cached_best_block_hash == getLastBlockProcessed()) return; + // Get required locks upfront. This avoids the GUI from getting stuck on // periodical polls if the core is holding the locks for a longer time - // for example, during a wallet rescan. - TRY_LOCK(cs_main, lockMain); - if (!lockMain) - return; - TRY_LOCK(wallet->cs_wallet, lockWallet); - if (!lockWallet) - return; + int chainHeight = m_client_model->getLastBlockProcessedHeight(); + const uint256& blockHash = m_client_model->getLastBlockProcessed(); + int64_t blockTime = m_client_model->getLastBlockProcessedTime(); // Don't continue processing if the chain tip time is less than the first // key creation time as there is no need to iterate over the transaction // table model in this case. - auto tip = chainActive.Tip(); - if (tip->GetBlockTime() < getCreationTime()) + if (blockTime < getCreationTime()) + return; + + TRY_LOCK(wallet->cs_wallet, lockWallet); + if (!lockWallet) return; - int chainHeight = tip->nHeight; if (fForceCheckBalanceChanged || chainHeight != cachedNumBlocks) { fForceCheckBalanceChanged = false; // Balance and number of transactions might have changed cachedNumBlocks = chainHeight; + m_cached_best_block_hash = blockHash; checkBalanceChanged(walletWrapper.getBalances()); if (transactionTableModel) { @@ -1093,3 +1098,19 @@ Optional WalletModel::getShieldedAddressFromSpendDesc(const uint256& tx Optional opAddr = wallet->GetSaplingScriptPubKeyMan()->GetAddressFromInputIfPossible(txHash, index); return opAddr ? Optional(QString::fromStdString(KeyIO::EncodePaymentAddress(*opAddr))) : nullopt; } + +void WalletModel::setClientModel(ClientModel* client_model) +{ + m_client_model = client_model; +} + +uint256 WalletModel::getLastBlockProcessed() const +{ + return m_client_model ? m_client_model->getLastBlockProcessed() : UINT256_ZERO; +} + +int WalletModel::getLastBlockProcessedNum() const +{ + return m_client_model ? m_client_model->getLastBlockProcessedHeight() : 0; +} + diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 3ad51df80780..928b19c72d9a 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -24,6 +24,7 @@ #include class AddressTableModel; +class ClientModel; class OptionsModel; class RecentRequestsTableModel; class TransactionTableModel; @@ -327,6 +328,12 @@ class WalletModel : public QObject void loadReceiveRequests(std::vector& vReceiveRequests); bool saveReceiveRequest(const std::string& sAddress, const int64_t nId, const std::string& sRequest); + ClientModel& clientModel() const { return *m_client_model; } + void setClientModel(ClientModel* client_model); + + uint256 getLastBlockProcessed() const; + int getLastBlockProcessedNum() const; + private: CWallet* wallet; // Simple Wallet interface. @@ -341,6 +348,7 @@ class WalletModel : public QObject std::unique_ptr m_handler_show_progress; std::unique_ptr m_handler_notify_watch_only_changed; std::unique_ptr m_handler_notify_walletbacked; + ClientModel* m_client_model; bool fHaveWatchOnly; bool fForceCheckBalanceChanged; @@ -358,6 +366,7 @@ class WalletModel : public QObject EncryptionStatus cachedEncryptionStatus; int cachedNumBlocks; + uint256 m_cached_best_block_hash; QTimer* pollTimer; From 33588fec63c884088d0dc08f19bf326d0a1b2188 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 12 Feb 2021 18:28:34 -0300 Subject: [PATCH 20/63] GUI: Remove cs_main lock and chain dependency from transaction update status --- src/qt/transactionrecord.cpp | 19 +++++-------------- src/qt/transactionrecord.h | 6 ++++-- src/qt/transactiontablemodel.cpp | 25 ++++++++++++++----------- 3 files changed, 23 insertions(+), 27 deletions(-) diff --git a/src/qt/transactionrecord.cpp b/src/qt/transactionrecord.cpp index c616cfb8b4e7..4a5cb3703baf 100644 --- a/src/qt/transactionrecord.cpp +++ b/src/qt/transactionrecord.cpp @@ -562,17 +562,8 @@ void TransactionRecord::loadHotOrColdStakeOrContract( ExtractAddress(p2csUtxo.scriptPubKey, false, record.address); } -void TransactionRecord::updateStatus(const CWalletTx& wtx) +void TransactionRecord::updateStatus(const CWalletTx& wtx, int chainHeight) { - AssertLockHeld(cs_main); - int chainHeight = chainActive.Height(); - - CBlockIndex *pindex = nullptr; - // Find the block the tx is in - BlockMap::iterator mi = mapBlockIndex.find(wtx.m_confirm.hashBlock); - if (mi != mapBlockIndex.end()) - pindex = (*mi).second; - // Determine transaction status // Update time if needed @@ -581,7 +572,7 @@ void TransactionRecord::updateStatus(const CWalletTx& wtx) // Sort order, unrecorded transactions sort to the top status.sortKey = strprintf("%010d-%01d-%010u-%03d", - (pindex ? pindex->nHeight : std::numeric_limits::max()), + wtx.m_confirm.block_height, (wtx.IsCoinBase() ? 1 : 0), time, idx); @@ -634,12 +625,12 @@ void TransactionRecord::updateStatus(const CWalletTx& wtx) status.status = TransactionStatus::Confirmed; } } + status.needsUpdate = false; } -bool TransactionRecord::statusUpdateNeeded() +bool TransactionRecord::statusUpdateNeeded(int blockHeight) const { - AssertLockHeld(cs_main); - return status.cur_num_blocks != chainActive.Height(); + return status.cur_num_blocks != blockHeight || status.needsUpdate; } int TransactionRecord::getOutputIndex() const diff --git a/src/qt/transactionrecord.h b/src/qt/transactionrecord.h index fdbfc235167d..6bf51cd2cc12 100644 --- a/src/qt/transactionrecord.h +++ b/src/qt/transactionrecord.h @@ -62,6 +62,8 @@ class TransactionStatus /** Current number of blocks (to know whether cached status is still valid) */ int cur_num_blocks; + + bool needsUpdate; }; /** UI model for a transaction. A core transaction can be represented by multiple UI transactions if it has @@ -181,11 +183,11 @@ class TransactionRecord /** Update status from core wallet tx. */ - void updateStatus(const CWalletTx& wtx); + void updateStatus(const CWalletTx& wtx, int chainHeight); /** Return whether a status update is needed. */ - bool statusUpdateNeeded(); + bool statusUpdateNeeded(int blockHeight) const; /** Return transaction status */ diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp index f9260e850885..4f1c8bdb2080 100644 --- a/src/qt/transactiontablemodel.cpp +++ b/src/qt/transactiontablemodel.cpp @@ -283,6 +283,10 @@ class TransactionTablePriv case CT_UPDATED: // Miscellaneous updates -- nothing to do, status update will take care of this, and is only computed for // visible transactions. + for (int i = lowerIndex; i < upperIndex; i++) { + TransactionRecord *rec = &cachedWallet[i]; + rec->status.needsUpdate = true; + } break; } } @@ -297,7 +301,7 @@ class TransactionTablePriv return hasZcTxes; } - TransactionRecord* index(int idx) + TransactionRecord* index(int cur_block_num, const uint256& cur_block_hash, int idx) { if (idx >= 0 && idx < cachedWallet.size()) { TransactionRecord* rec = &cachedWallet[idx]; @@ -309,15 +313,12 @@ class TransactionTablePriv // If a status update is needed (blocks came in since last check), // update the status of this transaction from the wallet. Otherwise, // simply re-use the cached status. - TRY_LOCK(cs_main, lockMain); - if (lockMain) { - TRY_LOCK(wallet->cs_wallet, lockWallet); - if (lockWallet && rec->statusUpdateNeeded()) { - std::map::iterator mi = wallet->mapWallet.find(rec->hash); - - if (mi != wallet->mapWallet.end()) { - rec->updateStatus(mi->second); - } + TRY_LOCK(wallet->cs_wallet, lockWallet); + if (lockWallet && rec->statusUpdateNeeded(cur_block_num)) { + std::map::iterator mi = wallet->mapWallet.find(rec->hash); + + if (mi != wallet->mapWallet.end()) { + rec->updateStatus(mi->second, cur_block_num); } } return rec; @@ -809,7 +810,9 @@ QVariant TransactionTableModel::headerData(int section, Qt::Orientation orientat QModelIndex TransactionTableModel::index(int row, int column, const QModelIndex& parent) const { Q_UNUSED(parent); - TransactionRecord* data = priv->index(row); + TransactionRecord* data = priv->index(walletModel->getLastBlockProcessedNum(), + walletModel->getLastBlockProcessed(), + row); if (data) { return createIndex(row, column, data); } From 0b618577eadd1f56700ed668fb3f3eb9efe67485 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 12 Feb 2021 18:29:26 -0300 Subject: [PATCH 21/63] GUI: fix coinstake tx ordering. --- src/qt/transactionrecord.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qt/transactionrecord.cpp b/src/qt/transactionrecord.cpp index 4a5cb3703baf..6ef501c243fc 100644 --- a/src/qt/transactionrecord.cpp +++ b/src/qt/transactionrecord.cpp @@ -573,7 +573,7 @@ void TransactionRecord::updateStatus(const CWalletTx& wtx, int chainHeight) // Sort order, unrecorded transactions sort to the top status.sortKey = strprintf("%010d-%01d-%010u-%03d", wtx.m_confirm.block_height, - (wtx.IsCoinBase() ? 1 : 0), + ((wtx.IsCoinBase() || wtx.IsCoinStake()) ? 1 : 0), time, idx); From 65fbad1ea93c88436066059891d7468596be0f62 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 12 Feb 2021 18:37:22 -0300 Subject: [PATCH 22/63] GUI: remove cs_main lock dependency from transaction model update. --- src/qt/transactiontablemodel.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp index 4f1c8bdb2080..fb4f9a79ddd5 100644 --- a/src/qt/transactiontablemodel.cpp +++ b/src/qt/transactiontablemodel.cpp @@ -239,24 +239,22 @@ class TransactionTablePriv break; } if (showTransaction) { - LOCK2(cs_main, wallet->cs_wallet); // Find transaction in wallet - auto mi = wallet->mapWallet.find(hash); - if (mi == wallet->mapWallet.end()) { + const CWalletTx* wtx = wallet->GetWalletTx(hash); + if (!wtx) { qWarning() << "TransactionTablePriv::updateWallet : Warning: Got CT_NEW, but transaction is not in wallet"; break; } - const CWalletTx& wtx = mi->second; // As old transactions are still getting updated (+20k range), // do not add them if we deliberately didn't load them at startup. - if (cachedWallet.size() >= MAX_AMOUNT_LOADED_RECORDS && wtx.GetTxTime() < nFirstLoadedTxTime) { + if (cachedWallet.size() >= MAX_AMOUNT_LOADED_RECORDS && wtx->GetTxTime() < nFirstLoadedTxTime) { return; } // Added -- insert at the right position QList toInsert = - TransactionRecord::decomposeTransaction(wallet, wtx); + TransactionRecord::decomposeTransaction(wallet, *wtx); if (!toInsert.isEmpty()) { /* only if something to insert */ parent->beginInsertRows(QModelIndex(), lowerIndex, lowerIndex + toInsert.size() - 1); int insert_idx = lowerIndex; From fcb20c286143fb64ee0272531e50a5cd9114dd49 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 12 Feb 2021 18:40:02 -0300 Subject: [PATCH 23/63] GUI: remove cs_main lock dependency from CWalletModel::isSpent, CWalletModel::isLockedCoin, CWalletModel::lockCoin, CWalletModel::unlockCoin --- src/qt/walletmodel.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 7467be53b138..998d8b1e7132 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -949,7 +949,7 @@ bool WalletModel::getMNCollateralCandidate(COutPoint& outPoint) bool WalletModel::isSpent(const COutPoint& outpoint) const { - LOCK2(cs_main, wallet->cs_wallet); + LOCK(wallet->cs_wallet); return wallet->IsSpent(outpoint.hash, outpoint.n); } @@ -1026,19 +1026,19 @@ void WalletModel::listCoins(std::map>& bool WalletModel::isLockedCoin(uint256 hash, unsigned int n) const { - LOCK2(cs_main, wallet->cs_wallet); + LOCK(wallet->cs_wallet); return wallet->IsLockedCoin(hash, n); } void WalletModel::lockCoin(COutPoint& output) { - LOCK2(cs_main, wallet->cs_wallet); + LOCK(wallet->cs_wallet); wallet->LockCoin(output); } void WalletModel::unlockCoin(COutPoint& output) { - LOCK2(cs_main, wallet->cs_wallet); + LOCK(wallet->cs_wallet); wallet->UnlockCoin(output); } From fcc4c83f1fd768728c69d36ad80b2bd4a2a2e0ff Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 13 Feb 2021 12:20:44 -0300 Subject: [PATCH 24/63] Move AutoCombineDust functionality to the wallet background thread --- src/validation.cpp | 12 ------------ src/wallet/wallet.cpp | 6 ++++++ 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index d8477fd5cd8a..e4af3d8a27ee 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3297,18 +3297,6 @@ bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const std::shared_pt } } - if (pwalletMain) { - /* disable multisend - // If turned on MultiSend will send a transaction (or more) on the after maturity of a stake - if (pwalletMain->isMultiSendEnabled()) - pwalletMain->MultiSend(); - */ - - // If turned on Auto Combine will scan wallet for dust to combine - if (pwalletMain->fCombineDust) - pwalletMain->AutoCombineDust(g_connman.get()); - } - LogPrintf("%s : ACCEPTED Block %ld in %ld milliseconds with size=%d\n", __func__, newHeight, GetTimeMillis() - nStartTime, GetSerializeSize(*pblock, SER_DISK, CLIENT_VERSION)); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6181a3392f60..b54695249185 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1318,6 +1318,12 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const // Sapling: Update cached incremental witnesses ChainTipAdded(pindex, pblock.get(), oldSaplingTree); + + // Auto-combine functionality + // If turned on Auto Combine will scan wallet for dust to combine + if (fCombineDust) { + AutoCombineDust(g_connman.get()); + } } void CWallet::BlockDisconnected(const std::shared_ptr& pblock, const uint256& blockHash, int nBlockHeight, int64_t blockTime) From 59ed47d873916c716c5d5846e4a66bdaab0de94f Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 13 Feb 2021 12:36:19 -0300 Subject: [PATCH 25/63] Wallet: remove cs_main lock from AutoCombineDust plus a redundant maturity check. --- src/wallet/wallet.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b54695249185..496a5b9db6c1 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3822,9 +3822,10 @@ bool CWallet::LoadDestData(const CTxDestination& dest, const std::string& key, c void CWallet::AutoCombineDust(CConnman* connman) { - LOCK2(cs_main, cs_wallet); - const CBlockIndex* tip = chainActive.Tip(); - if (tip->nTime < (GetAdjustedTime() - 300) || IsLocked()) { + LOCK(cs_wallet); + if (m_last_block_processed.IsNull() || + m_last_block_processed_time < (GetAdjustedTime() - 300) || + IsLocked()) { return; } @@ -3846,9 +3847,6 @@ void CWallet::AutoCombineDust(CConnman* connman) for (const COutput& out : vCoins) { if (!out.fSpendable) continue; - //no coins should get this far if they dont have proper maturity, this is double checking - if (out.tx->IsCoinStake() && out.tx->GetDepthInMainChain() < Params().GetConsensus().nCoinbaseMaturity + 1) - continue; // no p2cs accepted, those coins are "locked" if (out.tx->tx->vout[out.i].scriptPubKey.IsPayToColdStaking()) From b9220f44b26e757274188b9be3fbc5057c648a3e Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 13 Feb 2021 12:39:14 -0300 Subject: [PATCH 26/63] Wallet: Do not add P2CS utxo to autocombine flow and discard them later. --- src/wallet/wallet.cpp | 10 ++++------ src/wallet/wallet.h | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 496a5b9db6c1..fd16e2e0858e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2480,11 +2480,12 @@ bool CWallet::AvailableCoins(std::vector* pCoins, // --> populates } } -std::map > CWallet::AvailableCoinsByAddress(bool fConfirmed, CAmount maxCoinValue) +std::map > CWallet::AvailableCoinsByAddress(bool fConfirmed, CAmount maxCoinValue, bool fIncludeColdStaking) { CWallet::AvailableCoinsFilter coinFilter; coinFilter.fIncludeColdStaking = true; coinFilter.fOnlyConfirmed = fConfirmed; + coinFilter.fIncludeColdStaking = fIncludeColdStaking; std::vector vCoins; // include cold AvailableCoins(&vCoins, nullptr, coinFilter); @@ -3829,7 +3830,8 @@ void CWallet::AutoCombineDust(CConnman* connman) return; } - std::map > mapCoinsByAddress = AvailableCoinsByAddress(true, nAutoCombineThreshold * COIN); + std::map > mapCoinsByAddress = + AvailableCoinsByAddress(true, nAutoCombineThreshold * COIN, false); //coins are sectioned by address. This combination code only wants to combine inputs that belong to the same address for (std::map >::iterator it = mapCoinsByAddress.begin(); it != mapCoinsByAddress.end(); it++) { @@ -3848,10 +3850,6 @@ void CWallet::AutoCombineDust(CConnman* connman) if (!out.fSpendable) continue; - // no p2cs accepted, those coins are "locked" - if (out.tx->tx->vout[out.i].scriptPubKey.IsPayToColdStaking()) - continue; - COutPoint outpt(out.tx->GetHash(), out.i); coinControl->Select(outpt); vRewardCoins.push_back(out); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 65e9167c89c6..e6f16a0b812f 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -812,7 +812,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface //! >> Available coins (P2CS) void GetAvailableP2CSCoins(std::vector& vCoins) const; - std::map > AvailableCoinsByAddress(bool fConfirmed = true, CAmount maxCoinValue = 0); + std::map > AvailableCoinsByAddress(bool fConfirmed, CAmount maxCoinValue, bool fIncludeColdStaking); /// Get 10000 PIV output and keys which can be used for the Masternode bool GetMasternodeVinAndKeys(CTxIn& txinRet, CPubKey& pubKeyRet, From 2e7cdb2d4bc93cf85a9c7910dcad346c9f3a43a1 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 13 Feb 2021 13:00:49 -0300 Subject: [PATCH 27/63] Wallet: added max value out filter for AvailableCoins. Plus cleaned AvailableCoinsByAddress to make use of it. --- src/wallet/wallet.cpp | 24 +++++++++++++++--------- src/wallet/wallet.h | 8 ++++++-- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index fd16e2e0858e..73eddb1a3db3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2448,6 +2448,11 @@ bool CWallet::AvailableCoins(std::vector* pCoins, // --> populates for (unsigned int i = 0; i < pcoin->tx->vout.size(); i++) { const auto& output = pcoin->tx->vout[i]; + // Filter by value if needed + if (coinsFilter.nMaxOutValue > 0 && output.nValue > coinsFilter.nMaxOutValue) { + continue; + } + // Filter by specific destinations if needed if (coinsFilter.onlyFilteredDest && !coinsFilter.onlyFilteredDest->empty()) { CTxDestination address; @@ -2486,26 +2491,27 @@ std::map > CWallet::AvailableCoinsByAddres coinFilter.fIncludeColdStaking = true; coinFilter.fOnlyConfirmed = fConfirmed; coinFilter.fIncludeColdStaking = fIncludeColdStaking; + coinFilter.nMaxOutValue = maxCoinValue; std::vector vCoins; - // include cold AvailableCoins(&vCoins, nullptr, coinFilter); std::map > mapCoins; - for (COutput& out : vCoins) { - if (maxCoinValue > 0 && out.tx->tx->vout[out.i].nValue > maxCoinValue) - continue; - + for (const COutput& out : vCoins) { CTxDestination address; bool fColdStakeAddr = false; if (!ExtractDestination(out.tx->tx->vout[out.i].scriptPubKey, address, fColdStakeAddr)) { + bool isP2CS = out.tx->tx->vout[out.i].scriptPubKey.IsPayToColdStaking(); + if (isP2CS && !fIncludeColdStaking) { + // It must never happen as the coin filtering process shouldn't had added the P2CS in the first place + assert(false); + } // if this is a P2CS we don't have the owner key - check if we have the staking key fColdStakeAddr = true; - if ( !out.tx->tx->vout[out.i].scriptPubKey.IsPayToColdStaking() || - !ExtractDestination(out.tx->tx->vout[out.i].scriptPubKey, address, fColdStakeAddr) ) + if (!isP2CS || !ExtractDestination(out.tx->tx->vout[out.i].scriptPubKey, address, fColdStakeAddr) ) continue; } - mapCoins[address].push_back(out); + mapCoins[address].emplace_back(out); } return mapCoins; @@ -3207,7 +3213,7 @@ std::string CWallet::CommitResult::ToString() const CWallet::CommitResult CWallet::CommitTransaction(CTransactionRef tx, CReserveKey& opReservekey, CConnman* connman) { - return CommitTransaction(tx, &opReservekey, connman); + return CommitTransaction(std::move(tx), &opReservekey, connman); } /** diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index e6f16a0b812f..4729edda2dc6 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -779,7 +779,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface bool _fOnlySpendable, std::set* _onlyFilteredDest, int _minDepth, - bool _fIncludeLocked = false) : + bool _fIncludeLocked = false, + CAmount _nMaxOutValue = 0) : fIncludeDelegated(_fIncludeDelegated), fIncludeColdStaking(_fIncludeColdStaking), nCoinType(_nCoinType), @@ -787,7 +788,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface fOnlySpendable(_fOnlySpendable), onlyFilteredDest(_onlyFilteredDest), minDepth(_minDepth), - fIncludeLocked(_fIncludeLocked) {} + fIncludeLocked(_fIncludeLocked), + nMaxOutValue(_nMaxOutValue) {} bool fIncludeDelegated{true}; bool fIncludeColdStaking{false}; @@ -797,6 +799,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface std::set* onlyFilteredDest{nullptr}; int minDepth{0}; bool fIncludeLocked{false}; + // Select outputs with value <= nMaxOutValue + CAmount nMaxOutValue{0}; }; //! >> Available coins (generic) From 1e7ffc27b4e10e08c0928e8aca4849f724dc6bf5 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 13 Feb 2021 13:03:28 -0300 Subject: [PATCH 28/63] Wallet: remove cs_main requirement from CreateCoinStake --- src/wallet/wallet.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 73eddb1a3db3..35ab754fef5c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3085,8 +3085,8 @@ bool CWallet::CreateCoinStake( outPoint, it->pindex); - //new block came in, move on - if (WITH_LOCK(cs_main, return chainActive.Height()) != pindexPrev->nHeight) return false; + // New block came in, move on + if (WITH_LOCK(cs_wallet, return m_last_block_processed_height) != pindexPrev->nHeight) return false; // Make sure the wallet is unlocked and shutdown hasn't been requested if (IsLocked() || ShutdownRequested()) return false; From a7f6ab1b63f9abf75ccea3e142cd202f79dd066c Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 13 Feb 2021 13:14:17 -0300 Subject: [PATCH 29/63] Wallet: remove cs_main requirement from RelayWalletTransaction and FundTransaction. --- src/wallet/wallet.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 35ab754fef5c..0cbc1a1cdc82 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1950,7 +1950,6 @@ void CWalletTx::RelayWalletTransaction(CConnman* connman) // Nothing to do. Return early return; } - LOCK(cs_main); if (GetDepthInMainChain() == 0 && !isAbandoned()) { const uint256& hash = GetHash(); LogPrintf("Relaying wtx %s\n", hash.ToString()); @@ -2801,7 +2800,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool ov tx.vin.push_back(txin); if (lockUnspents) { - LOCK2(cs_main, cs_wallet); + LOCK(cs_wallet); LockCoin(txin.prevout); } } From 5109c8d82d39f4216cb5d66a5b94408d0c1e5e74 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 14 Feb 2021 15:17:39 -0300 Subject: [PATCH 30/63] Wallet: remove cs_main from IsSaplingSpent() and GetFilteredNotes() --- src/sapling/saplingscriptpubkeyman.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/sapling/saplingscriptpubkeyman.cpp b/src/sapling/saplingscriptpubkeyman.cpp index decc1753e4d5..4794149395f3 100644 --- a/src/sapling/saplingscriptpubkeyman.cpp +++ b/src/sapling/saplingscriptpubkeyman.cpp @@ -9,6 +9,7 @@ void SaplingScriptPubKeyMan::AddToSaplingSpends(const uint256& nullifier, const uint256& wtxid) { + AssertLockHeld(wallet->cs_wallet); mapTxSaplingNullifiers.emplace(nullifier, wtxid); std::pair range; @@ -17,7 +18,7 @@ void SaplingScriptPubKeyMan::AddToSaplingSpends(const uint256& nullifier, const } bool SaplingScriptPubKeyMan::IsSaplingSpent(const uint256& nullifier) const { - LOCK(cs_main); + LOCK(wallet->cs_wallet); // future: move to AssertLockHeld() std::pair range; range = mapTxSaplingNullifiers.equal_range(nullifier); @@ -470,7 +471,7 @@ void SaplingScriptPubKeyMan::GetFilteredNotes( bool requireSpendingKey, bool ignoreLocked) const { - LOCK2(cs_main, wallet->cs_wallet); + LOCK(wallet->cs_wallet); for (auto& p : wallet->mapWallet) { const CWalletTx& wtx = p.second; From 45c9471a7ea140f2ad16eefacd78fcb8161a7d5c Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 15 Feb 2021 15:35:08 -0300 Subject: [PATCH 31/63] wallet: split CheckTXAvailability in two. Use the cached chain height when is possible instead of locking cs_main to the tip height. --- src/sapling/saplingscriptpubkeyman.cpp | 2 +- src/wallet/wallet.cpp | 45 +++++++++++++++++++------- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/src/sapling/saplingscriptpubkeyman.cpp b/src/sapling/saplingscriptpubkeyman.cpp index 4794149395f3..fed52cf7713d 100644 --- a/src/sapling/saplingscriptpubkeyman.cpp +++ b/src/sapling/saplingscriptpubkeyman.cpp @@ -482,7 +482,7 @@ void SaplingScriptPubKeyMan::GetFilteredNotes( } // Filter the transactions before checking for notes - if (!CheckFinalTx(wtx.tx) || + if (!IsFinalTx(wtx.tx, wallet->GetLastBlockHeight() + 1, GetAdjustedTime()) || wtx.GetDepthInMainChain() < minDepth || wtx.GetDepthInMainChain() > maxDepth) { continue; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0cbc1a1cdc82..f28e816fc075 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2271,9 +2271,8 @@ void CWallet::GetAvailableP2CSCoins(std::vector& vCoins) const { /** * Test if the transaction is spendable. */ -bool CheckTXAvailability(const CWalletTx* pcoin, bool fOnlyConfirmed, int& nDepth) +static bool CheckTXAvailabilityInternal(const CWalletTx* pcoin, bool fOnlyConfirmed, int& nDepth) { - if (!CheckFinalTx(pcoin->tx)) return false; if (fOnlyConfirmed && !pcoin->IsTrusted()) return false; if (pcoin->GetBlocksToMaturity() > 0) return false; @@ -2286,6 +2285,25 @@ bool CheckTXAvailability(const CWalletTx* pcoin, bool fOnlyConfirmed, int& nDept return true; } +// cs_main lock required +static bool CheckTXAvailability(const CWalletTx* pcoin, bool fOnlyConfirmed, int& nDepth) +{ + AssertLockHeld(cs_main); + if (!CheckFinalTx(pcoin->tx)) return false; + return CheckTXAvailabilityInternal(pcoin, fOnlyConfirmed, nDepth); +} + +// cs_main lock NOT required +static bool CheckTXAvailability(const CWalletTx* pcoin, + bool fOnlyConfirmed, + int& nDepth, + int nBlockHeight) +{ + // Mimic CheckFinalTx without cs_main lock + if (!IsFinalTx(pcoin->tx, nBlockHeight + 1, GetAdjustedTime())) return false; + return CheckTXAvailabilityInternal(pcoin, fOnlyConfirmed, nDepth); +} + bool CWallet::GetMasternodeVinAndKeys(CTxIn& txinRet, CPubKey& pubKeyRet, CKey& keyRet, std::string strTxHash, std::string strOutputIndex, std::string& strError) { // wait for reindex and/or import to finish @@ -2326,17 +2344,20 @@ bool CWallet::GetMasternodeVinAndKeys(CTxIn& txinRet, CPubKey& pubKeyRet, CKey& return error("%s: tx %s, index %d not a masternode collateral", __func__, strTxHash, nOutputIndex); } - // Check availability int nDepth = 0; - if (!CheckTXAvailability(wtx, true, nDepth)) { - strError = "Not available collateral transaction"; - return error("%s: tx %s not available", __func__, strTxHash); - } + { + LOCK(cs_wallet); + // Check availability + if (!CheckTXAvailability(wtx, true, nDepth, m_last_block_processed_height)) { + strError = "Not available collateral transaction"; + return error("%s: tx %s not available", __func__, strTxHash); + } - // Skip spent coins - if (IsSpent(txHash, nOutputIndex)) { - strError = "Error: collateral already spent"; - return error("%s: tx %s already spent", __func__, strTxHash); + // Skip spent coins + if (IsSpent(txHash, nOutputIndex)) { + strError = "Error: collateral already spent"; + return error("%s: tx %s already spent", __func__, strTxHash); + } } // Depth must be at least MASTERNODE_MIN_CONFIRMATIONS @@ -2435,7 +2456,7 @@ bool CWallet::AvailableCoins(std::vector* pCoins, // --> populates // Check if the tx is selectable int nDepth; - if (!CheckTXAvailability(pcoin, coinsFilter.fOnlyConfirmed, nDepth)) + if (!CheckTXAvailability(pcoin, coinsFilter.fOnlyConfirmed, nDepth, m_last_block_processed_height)) continue; // Check min depth requirement for stake inputs From f889dcb55b69bcccf52824dfd1f1a78923ed5c03 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 23 Feb 2021 16:23:35 -0300 Subject: [PATCH 32/63] test : updating wallet's last block processed manually. --- src/test/librust/sapling_rpc_wallet_tests.cpp | 7 ++++--- src/test/librust/sapling_wallet_tests.cpp | 7 +++++-- src/wallet/test/wallet_tests.cpp | 1 + 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/test/librust/sapling_rpc_wallet_tests.cpp b/src/test/librust/sapling_rpc_wallet_tests.cpp index c3901aaaf39f..c3a86997c2e2 100644 --- a/src/test/librust/sapling_rpc_wallet_tests.cpp +++ b/src/test/librust/sapling_rpc_wallet_tests.cpp @@ -429,12 +429,13 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_taddr_to_sapling) auto blockHash = block.GetHash(); CBlockIndex fakeIndex {block}; fakeIndex.nHeight = 1; - mapBlockIndex.insert(std::make_pair(blockHash, &fakeIndex)); + BlockMap::iterator mi = mapBlockIndex.emplace(blockHash, &fakeIndex).first; + fakeIndex.phashBlock = &((*mi).first); chainActive.SetTip(&fakeIndex); BOOST_CHECK(chainActive.Contains(&fakeIndex)); BOOST_CHECK_EQUAL(1, chainActive.Height()); - wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, blockHash, 0); - pwalletMain->LoadToWallet(wtx); + std::vector vtxConflicted; + pwalletMain->BlockConnected(std::make_shared(block), mi->second, vtxConflicted); BOOST_CHECK_MESSAGE(pwalletMain->GetAvailableBalance() > 0, "tx not confirmed"); // Context that shieldsendmany requires diff --git a/src/test/librust/sapling_wallet_tests.cpp b/src/test/librust/sapling_wallet_tests.cpp index c35dd0a9fd78..ac346c002365 100644 --- a/src/test/librust/sapling_wallet_tests.cpp +++ b/src/test/librust/sapling_wallet_tests.cpp @@ -351,8 +351,8 @@ BOOST_AUTO_TEST_CASE(SaplingNullifierIsSpent) { BOOST_CHECK(chainActive.Contains(&fakeIndex)); BOOST_CHECK_EQUAL(0, chainActive.Height()); - wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, 0, block.GetHash(), 0); - wallet.LoadToWallet(wtx); + std::vector vtxConflicted; + wallet.BlockConnected(std::make_shared(block), mi->second, vtxConflicted); // Verify note has been spent BOOST_CHECK(wallet.GetSaplingScriptPubKeyMan()->IsSaplingSpent(nullifier)); @@ -421,6 +421,7 @@ BOOST_AUTO_TEST_CASE(NavigateFromSaplingNullifierToNote) { wallet.LoadToWallet(wtx); // Verify dummy note is now spent, as AddToWallet invokes AddToSpends() + wallet.SetLastBlockProcessed(mi->second); BOOST_CHECK(wallet.GetSaplingScriptPubKeyMan()->IsSaplingSpent(nullifier)); // Test invariant: no witnesses means no nullifier. @@ -520,6 +521,7 @@ BOOST_AUTO_TEST_CASE(SpentSaplingNoteIsFromMe) { // in the output descriptions of wtx. wallet.IncrementNoteWitnesses(&fakeIndex, &block, saplingTree); wallet.GetSaplingScriptPubKeyMan()->UpdateSaplingNullifierNoteMapForBlock(&block); + wallet.SetLastBlockProcessed(mi->second); // Retrieve the updated wtx from wallet wtx = wallet.mapWallet.at(wtx.GetHash()); @@ -590,6 +592,7 @@ BOOST_AUTO_TEST_CASE(SpentSaplingNoteIsFromMe) { wtx2.SetSaplingNoteData(saplingNoteData2); wtx2.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex2.nHeight, block2.GetHash(), 0); wallet.LoadToWallet(wtx2); + wallet.SetLastBlockProcessed(mi2->second); // Verify note B is spent. AddToWallet invokes AddToSpends which updates mapTxSaplingNullifiers BOOST_CHECK(wallet.GetSaplingScriptPubKeyMan()->IsSaplingSpent(nullifier2)); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 2564791b76d7..276cef991098 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -420,6 +420,7 @@ BOOST_AUTO_TEST_CASE(cached_balances_tests) LOCK2(cs_main, wallet.cs_wallet); wallet.SetMinVersion(FEATURE_PRE_SPLIT_KEYPOOL); wallet.SetupSPKM(false); + wallet.SetLastBlockProcessed(chainActive.Tip()); // Receive balance from an external source CTxDestination receivingAddr; From c4952a27cd4698b27d8109d3478caef13c54712c Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 23 Feb 2021 20:52:55 -0300 Subject: [PATCH 33/63] Wallet::CreateWalletFromFile guard direct chainActive access --- src/wallet/wallet.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index f28e816fc075..a76f031813e4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4222,11 +4222,12 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile) return nullptr; } - walletInstance->SetBestChain(chainActive.GetLocator()); + walletInstance->SetBestChain(WITH_LOCK(cs_main, return chainActive.GetLocator())); } LogPrintf("Wallet completed loading in %15dms\n", GetTimeMillis() - nStart); + LOCK(cs_main); CBlockIndex* pindexRescan = chainActive.Genesis(); if (!gArgs.GetBoolArg("-rescan", false)) { CWalletDB walletdb(*walletInstance->dbw); From c69e7e89955abb0c8a6f9352bf4842c5fef77b31 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 23 Feb 2021 21:19:07 -0300 Subject: [PATCH 34/63] Adapt sapling_wallet_listreceived.py to the new wtx confirmation structure. The block index initial value is 0, not -1 now. --- test/functional/sapling_wallet_listreceived.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/sapling_wallet_listreceived.py b/test/functional/sapling_wallet_listreceived.py index 5fbfd4c6963b..e05e803509c5 100755 --- a/test/functional/sapling_wallet_listreceived.py +++ b/test/functional/sapling_wallet_listreceived.py @@ -95,7 +95,7 @@ def run_test(self): assert_false(r[0]['change'], "Note should not be change") assert_equal(my_memo_hex, r[0]['memo']) assert_equal(0, r[0]['confirmations']) - assert_equal(-1, r[0]['blockindex']) + assert_equal(0, r[0]['blockindex']) assert_equal(0, r[0]['blockheight']) c = self.nodes[1].getsaplingnotescount(0) From e0a0d2d57482cd6dc6ff44279d5e51e7c9becaf6 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 23 Feb 2021 21:48:30 -0300 Subject: [PATCH 35/63] sapling_wallet_tests: locking order refactor, solving inconsistent orders for cs_main and cs_wallet. --- src/test/librust/sapling_wallet_tests.cpp | 56 +++++++++++++---------- src/test/librust/utiltest.cpp | 11 +++-- src/test/librust/utiltest.h | 5 +- 3 files changed, 42 insertions(+), 30 deletions(-) diff --git a/src/test/librust/sapling_wallet_tests.cpp b/src/test/librust/sapling_wallet_tests.cpp index ac346c002365..af0cc84688ae 100644 --- a/src/test/librust/sapling_wallet_tests.cpp +++ b/src/test/librust/sapling_wallet_tests.cpp @@ -614,8 +614,10 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesEmptyChain) { auto consensusParams = RegtestActivateSapling(); CWallet& wallet = *pwalletMain; - LOCK(wallet.cs_wallet); - setupWallet(wallet); + { + LOCK(wallet.cs_wallet); + setupWallet(wallet); + } auto sk = GetTestMasterSaplingSpendingKey(); CWalletTx wtx = GetValidSaplingReceive(Params().GetConsensus(), wallet, sk, 10, true); @@ -653,18 +655,19 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesEmptyChain) { BOOST_AUTO_TEST_CASE(CachedWitnessesChainTip) { auto consensusParams = RegtestActivateSapling(); + libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey(); CWallet& wallet = *pwalletMain; - LOCK(wallet.cs_wallet); - setupWallet(wallet); + { + LOCK(wallet.cs_wallet); + setupWallet(wallet); + BOOST_CHECK(wallet.AddSaplingZKey(sk)); + BOOST_CHECK(wallet.HaveSaplingSpendingKey(sk.ToXFVK())); + } uint256 anchors1; CBlock block1; SaplingMerkleTree saplingTree; - libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey(); - BOOST_CHECK(wallet.AddSaplingZKey(sk)); - BOOST_CHECK(wallet.HaveSaplingSpendingKey(sk.ToXFVK())); - { // First block (case tested in _empty_chain) CBlockIndex index1(block1); @@ -731,15 +734,15 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesChainTip) { BOOST_AUTO_TEST_CASE(CachedWitnessesDecrementFirst) { auto consensusParams = RegtestActivateSapling(); + libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey(); CWallet& wallet = *pwalletMain; - LOCK(wallet.cs_wallet); - setupWallet(wallet); + { + LOCK(wallet.cs_wallet); + setupWallet(wallet); + BOOST_CHECK(wallet.AddSaplingZKey(sk)); + } SaplingMerkleTree saplingTree; - - libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey(); - BOOST_CHECK(wallet.AddSaplingZKey(sk)); - { // First block (case tested in _empty_chain) CBlock block1; @@ -799,9 +802,13 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesDecrementFirst) { BOOST_AUTO_TEST_CASE(CachedWitnessesCleanIndex) { auto consensusParams = RegtestActivateSapling(); + libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey(); CWallet& wallet = *pwalletMain; - LOCK(wallet.cs_wallet); - setupWallet(wallet); + { + LOCK(wallet.cs_wallet); + setupWallet(wallet); + BOOST_CHECK(wallet.AddSaplingZKey(sk)); + } std::vector blocks; std::vector indices; @@ -811,9 +818,6 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesCleanIndex) { SaplingMerkleTree saplingRiTree = saplingTree; std::vector> saplingWitnesses; - libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey(); - BOOST_CHECK(wallet.AddSaplingZKey(sk)); - // Generate a chain size_t numBlocks = WITNESS_CACHE_SIZE + 10; blocks.resize(numBlocks); @@ -874,12 +878,13 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesCleanIndex) { BOOST_AUTO_TEST_CASE(ClearNoteWitnessCache) { auto consensusParams = RegtestActivateSapling(); - CWallet& wallet = *pwalletMain; - LOCK(wallet.cs_wallet); - setupWallet(wallet); - libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey(); - BOOST_CHECK(wallet.AddSaplingZKey(sk)); + CWallet& wallet = *pwalletMain; + { + LOCK(wallet.cs_wallet); + setupWallet(wallet); + BOOST_CHECK(wallet.AddSaplingZKey(sk)); + } CWalletTx wtx = GetValidSaplingReceive(Params().GetConsensus(), wallet, sk, 10, true); @@ -922,7 +927,8 @@ BOOST_AUTO_TEST_CASE(UpdatedSaplingNoteData) { auto consensusParams = RegtestActivateSapling(); CWallet& wallet = *pwalletMain; - LOCK(wallet.cs_wallet); + // Need to lock cs_main for now due the lock ordering. future: revamp all of this function to only lock where is needed. + LOCK2(cs_main, wallet.cs_wallet); setupWallet(wallet); auto m = GetTestMasterSaplingSpendingKey(); diff --git a/src/test/librust/utiltest.cpp b/src/test/librust/utiltest.cpp index be1c63e33ae3..caf2511daf1b 100644 --- a/src/test/librust/utiltest.cpp +++ b/src/test/librust/utiltest.cpp @@ -43,6 +43,11 @@ CKey AddTestCKeyToKeyStore(CBasicKeyStore& keyStore, bool genNewKey) { return tsk; } +CKey AddTestCKeyToWallet(CWallet& wallet, bool genNewKey) { + LOCK(wallet.cs_wallet); + return AddTestCKeyToKeyStore(wallet, genNewKey); +} + TestSaplingNote GetTestSaplingNote(const libzcash::SaplingPaymentAddress& pa, CAmount value) { // Generate dummy Sapling note libzcash::SaplingNote note(pa, value); @@ -80,13 +85,13 @@ CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams, // Two dummy input (to trick coinbase check), one or many shielded outputs CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams, - CBasicKeyStore& keyStoreFrom, + CWallet& keyStoreFrom, CAmount inputAmount, std::vector vDest, bool genNewKey, const CWallet* pwalletIn) { // From taddr - CKey tsk = AddTestCKeyToKeyStore(keyStoreFrom, genNewKey); + CKey tsk = AddTestCKeyToWallet(keyStoreFrom, genNewKey); auto scriptPubKey = GetScriptForDestination(tsk.GetPubKey().GetID()); // Two equal dummy inputs to by-pass the coinbase check. @@ -97,7 +102,7 @@ CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams, // Single input, single shielded output CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams, - CBasicKeyStore& keyStore, + CWallet& keyStore, const libzcash::SaplingExtendedSpendingKey &sk, CAmount value, bool genNewKey, diff --git a/src/test/librust/utiltest.h b/src/test/librust/utiltest.h index df2e69d17e5d..4f6e8412015f 100644 --- a/src/test/librust/utiltest.h +++ b/src/test/librust/utiltest.h @@ -35,6 +35,7 @@ void RegtestDeactivateSapling(); libzcash::SaplingExtendedSpendingKey GetTestMasterSaplingSpendingKey(); CKey AddTestCKeyToKeyStore(CBasicKeyStore& keyStore, bool genNewKey = false); +CKey AddTestCKeyToWallet(CWallet& wallet, bool genNewKey = false); /** * Generates a dummy destination script @@ -60,7 +61,7 @@ CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams, * Single dummy input, one or many shielded outputs. */ CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams, - CBasicKeyStore& keyStoreFrom, + CWallet& keyStoreFrom, CAmount inputAmount, std::vector vDest, bool genNewKey = false, @@ -70,7 +71,7 @@ CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams, * Single dummy input, single shielded output to sk default address. */ CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams, - CBasicKeyStore& keyStore, + CWallet& keyStore, const libzcash::SaplingExtendedSpendingKey &sk, CAmount value, bool genNewKey = false, From 4ccec74e80c924cac944279a6066aa7848ecbae9 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 24 Feb 2021 10:06:29 -0300 Subject: [PATCH 36/63] wallet: Add IsSpent() cs_wallet lock assertion. The method is no longer needing cs_main lock, only cs_wallet. --- src/wallet/wallet.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a76f031813e4..be2351a4033a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -703,6 +703,7 @@ bool CWallet::HasSaplingSPKM() const */ bool CWallet::IsSpent(const COutPoint& outpoint) const { + AssertLockHeld(cs_wallet); std::pair range; range = mapTxSpends.equal_range(outpoint); for (TxSpends::const_iterator it = range.first; it != range.second; ++it) { @@ -3112,9 +3113,7 @@ bool CWallet::CreateCoinStake( if (IsLocked() || ShutdownRequested()) return false; // Make sure the stake input hasn't been spent since last check - // for now, IsSpent() requires cs_main lock due its internal call to GetDepthInMainChain. - // This dependency will be completely removed moving forward, in #2209. - if (WITH_LOCK(cs_main, return IsSpent(outPoint))) { + if (WITH_LOCK(cs_wallet, return IsSpent(outPoint))) { // remove it from the available coins it = availableCoins->erase(it); continue; From 9720579b7212582f74bccb9d0225e8f79137a84e Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 24 Feb 2021 10:18:32 -0300 Subject: [PATCH 37/63] SSPKM: remove redundant ReadBlockFromDisk from IncrementNoteWitnesses. On the latest BlockConnected/BlockDisconnected flow, the block cannot be null. --- src/sapling/saplingscriptpubkeyman.cpp | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/sapling/saplingscriptpubkeyman.cpp b/src/sapling/saplingscriptpubkeyman.cpp index fed52cf7713d..3366baee001b 100644 --- a/src/sapling/saplingscriptpubkeyman.cpp +++ b/src/sapling/saplingscriptpubkeyman.cpp @@ -203,7 +203,7 @@ void UpdateWitnessHeights(NoteDataMap& noteDataMap, int indexHeight, int64_t nWi } void SaplingScriptPubKeyMan::IncrementNoteWitnesses(const CBlockIndex* pindex, - const CBlock* pblockIn, + const CBlock* pblock, SaplingMerkleTree& saplingTree) { LOCK(wallet->cs_wallet); @@ -217,17 +217,6 @@ void SaplingScriptPubKeyMan::IncrementNoteWitnesses(const CBlockIndex* pindex, nWitnessCacheNeedsUpdate = true; } - const CBlock* pblock {pblockIn}; - CBlock block; - if (!pblock) { - if (!ReadBlockFromDisk(block, pindex)) { - std::string read_failed_str = strprintf("Unable to read block %d (%s) from disk.", - pindex->nHeight, pindex->GetBlockHash().ToString()); - throw std::runtime_error(read_failed_str); - } - pblock = █ - } - for (const auto& tx : pblock->vtx) { if (!tx->IsShieldedTx()) continue; From 208a2921cca702c6e2aae193ce7cfbb491849aae Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 24 Feb 2021 10:23:33 -0300 Subject: [PATCH 38/63] wallet: remove last cs_main locks from every signal handler function :) . TransactionAddedToMempool, BlockConnected, BlockDisconnected don't longer need cs_main lock. --- src/wallet/wallet.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index be2351a4033a..e665bcc64c77 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1270,7 +1270,7 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, const CWalletTx::Confi void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) { - LOCK2(cs_main, cs_wallet); + LOCK(cs_wallet); CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, {}, /* nIndex */ 0); SyncTransaction(ptx, confirm); @@ -1290,7 +1290,7 @@ void CWallet::TransactionRemovedFromMempool(const CTransactionRef &ptx) { void CWallet::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) { - LOCK2(cs_main, cs_wallet); + LOCK(cs_wallet); m_last_block_processed = pindex->GetBlockHash(); m_last_block_processed_time = pindex->GetBlockTime(); @@ -1329,7 +1329,7 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const void CWallet::BlockDisconnected(const std::shared_ptr& pblock, const uint256& blockHash, int nBlockHeight, int64_t blockTime) { - LOCK2(cs_main, cs_wallet); + LOCK(cs_wallet); // At block disconnection, this will change an abandoned transaction to // be unconfirmed, whether or not the transaction is added back to the mempool. @@ -3848,7 +3848,7 @@ bool CWallet::LoadDestData(const CTxDestination& dest, const std::string& key, c void CWallet::AutoCombineDust(CConnman* connman) { - LOCK(cs_wallet); + AssertLockHeld(cs_wallet); if (m_last_block_processed.IsNull() || m_last_block_processed_time < (GetAdjustedTime() - 300) || IsLocked()) { From b73500add12115347c5b86d7d769827b16df294d Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 24 Feb 2021 10:35:34 -0300 Subject: [PATCH 39/63] wallet:BlockConnected do not lock cs_wallet for its entire process. AutoCombineDust() requires cs_main lock because is calling internally to CreateTransaction/CommitTransaction. --- src/wallet/wallet.cpp | 82 +++++++++++++++++++++++++------------------ 1 file changed, 47 insertions(+), 35 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e665bcc64c77..c5041b124ae2 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1290,38 +1290,43 @@ void CWallet::TransactionRemovedFromMempool(const CTransactionRef &ptx) { void CWallet::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) { - LOCK(cs_wallet); + { + LOCK(cs_wallet); - m_last_block_processed = pindex->GetBlockHash(); - m_last_block_processed_time = pindex->GetBlockTime(); - m_last_block_processed_height = pindex->nHeight; - for (size_t index = 0; index < pblock->vtx.size(); index++) { - CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, m_last_block_processed_height, m_last_block_processed->GetBlockHash(), index); - SyncTransaction(pblock->vtx[index], confirm); - TransactionRemovedFromMempool(pblock->vtx[index]); - } - for (const CTransactionRef& ptx : vtxConflicted) { - TransactionRemovedFromMempool(ptx); - } - - // Sapling: notify about the connected block - // Get prev block tree anchor - CBlockIndex* pprev = pindex->pprev; - SaplingMerkleTree oldSaplingTree; - bool isSaplingActive = (pprev) != nullptr && - Params().GetConsensus().NetworkUpgradeActive(pprev->nHeight, - Consensus::UPGRADE_V5_0); - if (isSaplingActive) { - assert(pcoinsTip->GetSaplingAnchorAt(pprev->hashFinalSaplingRoot, oldSaplingTree)); - } else { - assert(pcoinsTip->GetSaplingAnchorAt(SaplingMerkleTree::empty_root(), oldSaplingTree)); - } + m_last_block_processed = pindex->GetBlockHash(); + m_last_block_processed_time = pindex->GetBlockTime(); + m_last_block_processed_height = pindex->nHeight; + for (size_t index = 0; index < pblock->vtx.size(); index++) { + CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, m_last_block_processed_height, + m_last_block_processed, index); + SyncTransaction(pblock->vtx[index], confirm); + TransactionRemovedFromMempool(pblock->vtx[index]); + } + for (const CTransactionRef& ptx : vtxConflicted) { + TransactionRemovedFromMempool(ptx); + } + + // Sapling: notify about the connected block + // Get prev block tree anchor + CBlockIndex* pprev = pindex->pprev; + SaplingMerkleTree oldSaplingTree; + bool isSaplingActive = (pprev) != nullptr && + Params().GetConsensus().NetworkUpgradeActive(pprev->nHeight, + Consensus::UPGRADE_V5_0); + if (isSaplingActive) { + assert(pcoinsTip->GetSaplingAnchorAt(pprev->hashFinalSaplingRoot, oldSaplingTree)); + } else { + assert(pcoinsTip->GetSaplingAnchorAt(SaplingMerkleTree::empty_root(), oldSaplingTree)); + } - // Sapling: Update cached incremental witnesses - ChainTipAdded(pindex, pblock.get(), oldSaplingTree); + // Sapling: Update cached incremental witnesses + ChainTipAdded(pindex, pblock.get(), oldSaplingTree); + } // cs_wallet lock end // Auto-combine functionality // If turned on Auto Combine will scan wallet for dust to combine + // Outside of the cs_wallet lock because requires cs_main for now + // due CreateTransaction/CommitTransaction dependency. if (fCombineDust) { AutoCombineDust(g_connman.get()); } @@ -3848,11 +3853,13 @@ bool CWallet::LoadDestData(const CTxDestination& dest, const std::string& key, c void CWallet::AutoCombineDust(CConnman* connman) { - AssertLockHeld(cs_wallet); - if (m_last_block_processed.IsNull() || - m_last_block_processed_time < (GetAdjustedTime() - 300) || - IsLocked()) { - return; + { + LOCK(cs_wallet); + if (m_last_block_processed.IsNull() || + m_last_block_processed_time < (GetAdjustedTime() - 300) || + IsLocked()) { + return; + } } std::map > mapCoinsByAddress = @@ -3922,9 +3929,14 @@ void CWallet::AutoCombineDust(CConnman* connman) // 10% safety margin to avoid "Insufficient funds" errors vecSend[0].nAmount = nTotalRewardsValue - (nTotalRewardsValue / 10); - if (!CreateTransaction(vecSend, wtx, keyChange, nFeeRet, nChangePosInOut, strErr, coinControl, ALL_COINS, true, false, CAmount(0))) { - LogPrintf("AutoCombineDust createtransaction failed, reason: %s\n", strErr); - continue; + { + // For now, CreateTransaction requires cs_main lock. + LOCK2(cs_main, cs_wallet); + if (!CreateTransaction(vecSend, wtx, keyChange, nFeeRet, nChangePosInOut, strErr, coinControl, ALL_COINS, + true, false, CAmount(0))) { + LogPrintf("AutoCombineDust createtransaction failed, reason: %s\n", strErr); + continue; + } } //we don't combine below the threshold unless the fees are 0 to avoid paying fees over fees over fees From 76bc08bb754c379bd8da68e412aef1819f5387b4 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 25 Feb 2021 20:42:06 -0300 Subject: [PATCH 40/63] GUI: balance polling update moved to a background worker thread. --- src/qt/clientmodel.h | 2 +- src/qt/pivx.cpp | 7 +-- src/qt/walletmodel.cpp | 103 ++++++++++++++++++++++++++++------------- src/qt/walletmodel.h | 19 +++++++- 4 files changed, 94 insertions(+), 37 deletions(-) diff --git a/src/qt/clientmodel.h b/src/qt/clientmodel.h index b7e7d2c0f402..8ce41d0f5283 100644 --- a/src/qt/clientmodel.h +++ b/src/qt/clientmodel.h @@ -117,7 +117,7 @@ class ClientModel : public QObject QString cachedMasternodeCountString; bool cachedReindexing; bool cachedImporting; - bool cachedInitialSync; + std::atomic cachedInitialSync{false}; int numBlocksAtStartup; diff --git a/src/qt/pivx.cpp b/src/qt/pivx.cpp index 6cc23c2b123c..6f644f6340c3 100644 --- a/src/qt/pivx.cpp +++ b/src/qt/pivx.cpp @@ -449,16 +449,17 @@ void BitcoinApplication::requestShutdown() qDebug() << __func__ << ": Requesting shutdown"; startThread(); window->hide(); - window->setClientModel(0); + if (walletModel) walletModel->stop(); + window->setClientModel(nullptr); pollShutdownTimer->stop(); #ifdef ENABLE_WALLET window->removeAllWallets(); delete walletModel; - walletModel = 0; + walletModel = nullptr; #endif delete clientModel; - clientModel = 0; + clientModel = nullptr; // Show a simple window indicating shutdown status ShutdownWindow::showShutdownWindow(window); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 998d8b1e7132..7fe21f59ed4d 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -29,7 +29,7 @@ #include #include -#include +#include #include #include @@ -193,57 +193,78 @@ bool WalletModel::isWalletLocked(bool fFullUnlocked) const return (status == Locked || (!fFullUnlocked && status == UnlockedForStaking)); } -bool IsImportingOrReindexing() +static bool IsImportingOrReindexing() { return fImporting || fReindex; } +std::atomic processingBalance{false}; + +static bool processBalanceChangeInternal(WalletModel* walletModel) +{ + int chainHeight = walletModel->getLastBlockProcessedNum(); + const uint256& blockHash = walletModel->getLastBlockProcessed(); + + if (walletModel->hasForceCheckBalance() || chainHeight != walletModel->getCacheNumBLocks()) { + // Try to get lock only if needed + TRY_LOCK(pwalletMain->cs_wallet, lockWallet); + if (!lockWallet) + return false; + + walletModel->setfForceCheckBalanceChanged(false); + + // Balance and number of transactions might have changed + walletModel->setCacheNumBlocks(chainHeight); + walletModel->setCacheBlockHash(blockHash); + walletModel->checkBalanceChanged(walletModel->getBalances()); + QMetaObject::invokeMethod(walletModel, "updateTxModelData", Qt::QueuedConnection); + QMetaObject::invokeMethod(walletModel, "pollFinished", Qt::QueuedConnection); + + // Address in receive tab may have been used + Q_EMIT walletModel->notifyReceiveAddressChanged(); + } + return true; +} + +static void processBalanceChange(WalletModel* walletModel) +{ + if (!processBalanceChangeInternal(walletModel)) { + processingBalance = false; + } +} + void WalletModel::pollBalanceChanged() { + if (processingBalance || !m_client_model) return; + // Wait a little bit more when the wallet is reindexing and/or importing, no need to lock cs_main so often. - if (IsImportingOrReindexing()) { + if (IsImportingOrReindexing() || m_client_model->inInitialBlockDownload()) { static uint8_t waitLonger = 0; waitLonger++; - if (waitLonger < 10) // 10 seconds + if (waitLonger < 30) // 30 seconds return; waitLonger = 0; } - // Avoid recomputing wallet balances unless a tx changed or - // BlockTip notification was received. - if (!fForceCheckBalanceChanged && m_cached_best_block_hash == getLastBlockProcessed()) return; - - // Get required locks upfront. This avoids the GUI from getting stuck on - // periodical polls if the core is holding the locks for a longer time - - // for example, during a wallet rescan. - int chainHeight = m_client_model->getLastBlockProcessedHeight(); - const uint256& blockHash = m_client_model->getLastBlockProcessed(); - int64_t blockTime = m_client_model->getLastBlockProcessedTime(); - // Don't continue processing if the chain tip time is less than the first // key creation time as there is no need to iterate over the transaction // table model in this case. + int64_t blockTime = clientModel().getLastBlockProcessedTime(); if (blockTime < getCreationTime()) return; - TRY_LOCK(wallet->cs_wallet, lockWallet); - if (!lockWallet) - return; - - if (fForceCheckBalanceChanged || chainHeight != cachedNumBlocks) { - fForceCheckBalanceChanged = false; - - // Balance and number of transactions might have changed - cachedNumBlocks = chainHeight; - m_cached_best_block_hash = blockHash; + // Avoid recomputing wallet balances unless a tx changed or + // BlockTip notification was received. + if (!fForceCheckBalanceChanged && m_cached_best_block_hash == getLastBlockProcessed()) return; - checkBalanceChanged(walletWrapper.getBalances()); - if (transactionTableModel) { - transactionTableModel->updateConfirmations(); - } + processingBalance = true; + pollFuture = QtConcurrent::run(processBalanceChange, this); +} - // Address in receive tab may have been used - Q_EMIT notifyReceiveAddressChanged(); +void WalletModel::updateTxModelData() +{ + if (transactionTableModel) { + transactionTableModel->updateConfirmations(); } } @@ -257,7 +278,25 @@ void WalletModel::checkBalanceChanged(const interfaces::WalletBalances& newBalan { if (newBalance.balanceChanged(m_cached_balances)) { m_cached_balances = newBalance; - Q_EMIT balanceChanged(m_cached_balances); + QMetaObject::invokeMethod(this, "balanceNotify", Qt::QueuedConnection); + } +} + +void WalletModel::balanceNotify() +{ + Q_EMIT balanceChanged(m_cached_balances); +} + +void WalletModel::pollFinished() +{ + processingBalance = false; +} + +void WalletModel::stop() +{ + if(pollFuture.isRunning()) { + pollFuture.cancel(); + pollFuture.setPaused(true); } } diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 928b19c72d9a..ce0ffb91c61a 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -22,6 +22,7 @@ #include #include +#include class AddressTableModel; class ClientModel; @@ -334,6 +335,16 @@ class WalletModel : public QObject uint256 getLastBlockProcessed() const; int getLastBlockProcessedNum() const; + interfaces::WalletBalances getBalances() { return walletWrapper.getBalances(); }; + bool hasForceCheckBalance() { return fForceCheckBalanceChanged; } + void setCacheNumBlocks(int _cachedNumBlocks) { cachedNumBlocks = _cachedNumBlocks; } + int getCacheNumBLocks() { return cachedNumBlocks; } + void setCacheBlockHash(const uint256& _blockHash) { m_cached_best_block_hash = _blockHash; } + void setfForceCheckBalanceChanged(bool _fForceCheckBalanceChanged) { fForceCheckBalanceChanged = _fForceCheckBalanceChanged; } + Q_INVOKABLE void checkBalanceChanged(const interfaces::WalletBalances& new_balances); + + void stop(); + private: CWallet* wallet; // Simple Wallet interface. @@ -369,10 +380,10 @@ class WalletModel : public QObject uint256 m_cached_best_block_hash; QTimer* pollTimer; + QFuture pollFuture; void subscribeToCoreSignals(); void unsubscribeFromCoreSignals(); - Q_INVOKABLE void checkBalanceChanged(const interfaces::WalletBalances& new_balances); Q_SIGNALS: // Signal that balance in wallet changed @@ -402,6 +413,12 @@ class WalletModel : public QObject void notifyReceiveAddressChanged(); public Q_SLOTS: + /* Wallet balances changes */ + void balanceNotify(); + /* Update transaction model after wallet changes */ + void updateTxModelData(); + /* Balance polling process finished */ + void pollFinished(); /* Wallet status might have changed */ void updateStatus(); /* New transaction, or transaction changed status */ From 379e5f25e760145e6ea5e2f88e4b28ed64d30a7f Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 25 Feb 2021 20:42:44 -0300 Subject: [PATCH 41/63] GUI: send screen, move refresh amounts calculation to a background worker thread. --- src/qt/pivx/send.cpp | 70 ++++++++++++++++++++++++++------------------ src/qt/pivx/send.h | 7 ++++- 2 files changed, 47 insertions(+), 30 deletions(-) diff --git a/src/qt/pivx/send.cpp b/src/qt/pivx/send.cpp index f02c5b8c530d..c5c21e46cb5d 100644 --- a/src/qt/pivx/send.cpp +++ b/src/qt/pivx/send.cpp @@ -20,6 +20,7 @@ #include "openuridialog.h" #define REQUEST_PREPARE_TX 1 +#define REQUEST_REFRESH_BALANCE 2 SendWidget::SendWidget(PIVXGUI* parent) : PWidget(parent), @@ -142,9 +143,9 @@ void SendWidget::refreshAmounts() } nDisplayUnit = walletModel->getOptionsModel()->getDisplayUnit(); - ui->labelAmountSend->setText(GUIUtil::formatBalance(total, nDisplayUnit, false)); CAmount totalAmount = 0; + QString titleTotalRemaining; if (coinControlDialog->coinControl->HasSelected()) { // Set remaining balance to the sum of the coinControl selected inputs std::vector coins; @@ -154,20 +155,27 @@ void SendWidget::refreshAmounts() selectedBalance += coin.value; } totalAmount = selectedBalance - total; - ui->labelTitleTotalRemaining->setText(tr("Total remaining from the selected UTXO")); + titleTotalRemaining = tr("Total remaining from the selected UTXO"); } else { // Wallet's unlocked balance. totalAmount = isTransparent ? (walletModel->getUnlockedBalance(nullptr, fDelegationsChecked, false) - total) : (walletModel->GetWalletBalances().shielded_balance - total); - ui->labelTitleTotalRemaining->setText(tr("Unlocked remaining")); + titleTotalRemaining = tr("Unlocked remaining"); } + QString type = isTransparent ? "transparent" : "shielded"; - ui->labelAmountRemaining->setText( - GUIUtil::formatBalance( - totalAmount, - nDisplayUnit, - false) + " " + type - ); + QString labelAmountRemaining = GUIUtil::formatBalance( totalAmount, nDisplayUnit, false) + " " + type; + QMetaObject::invokeMethod(this, "updateAmounts", Qt::QueuedConnection, + Q_ARG(QString, titleTotalRemaining), + Q_ARG(QString, GUIUtil::formatBalance(total, nDisplayUnit, false)), + Q_ARG(QString, labelAmountRemaining)); +} + +void SendWidget::updateAmounts(const QString& _titleTotalRemaining, const QString& _labelAmountSend, const QString& _labelAmountRemaining) +{ + ui->labelTitleTotalRemaining->setText(_titleTotalRemaining); + ui->labelAmountSend->setText(_labelAmountSend); + ui->labelAmountRemaining->setText(_labelAmountRemaining); // show or hide delegations checkbox if need be showHideCheckBoxDelegations(); } @@ -203,9 +211,6 @@ void SendWidget::loadWalletModel() setCustomFeeSelected(true, nCustomFee); } - // Refresh - refreshAmounts(); - // TODO: This only happen when the coin control features are modified in other screen, check before do this if the wallet has another screen modifying it. // Coin Control //connect(walletModel->getOptionsModel(), &OptionsModel::coinControlFeaturesChanged, [this](){}); @@ -227,7 +232,7 @@ void SendWidget::clearAll(bool fClearSettings) if (fClearSettings) onResetSettings(); hideContactsMenu(); clearEntries(); - refreshAmounts(); + tryRefreshAmounts(); } void SendWidget::onResetSettings() @@ -243,7 +248,7 @@ void SendWidget::onResetCustomOptions(bool fRefreshAmounts) if (ui->checkBoxDelegations->isChecked()) ui->checkBoxDelegations->setChecked(false); resetCoinControl(); if (fRefreshAmounts) { - refreshAmounts(); + tryRefreshAmounts(); } } @@ -326,13 +331,7 @@ void SendWidget::showEvent(QShowEvent *event) { // Set focus on last recipient address when Send-window is displayed setFocusOnLastEntry(); - - // Update cached delegated balance - CAmount cachedDelegatedBalance_new = walletModel->getDelegatedBalance(); - if (cachedDelegatedBalance != cachedDelegatedBalance_new) { - cachedDelegatedBalance = cachedDelegatedBalance_new; - refreshAmounts(); - } + tryRefreshAmounts(); } void SendWidget::setFocusOnLastEntry() @@ -537,8 +536,8 @@ bool SendWidget::sendFinalStep() void SendWidget::run(int type) { - assert(!processingResult); if (type == REQUEST_PREPARE_TX) { + assert(!processingResult); if (!isProcessing) { isProcessing = true; OperationResult result(false); @@ -553,6 +552,12 @@ void SendWidget::run(int type) } isProcessing = false; } + } else if (type == REQUEST_REFRESH_BALANCE) { + if (!isUpdatingBalance) { + isUpdatingBalance = true; + refreshAmounts(); + isUpdatingBalance = false; + } } } @@ -562,9 +567,16 @@ void SendWidget::onError(QString error, int type) processingResultError = error; } -void SendWidget::updateEntryLabels(QList recipients) +void SendWidget::tryRefreshAmounts() +{ + if (!execute(REQUEST_REFRESH_BALANCE)) { + inform(tr("Processing full, refreshing amounts later")); + } +} + +void SendWidget::updateEntryLabels(const QList& recipients) { - for (SendCoinsRecipient rec : recipients) { + for (const SendCoinsRecipient& rec : recipients) { QString label = rec.label; if (!label.isNull()) { QString labelOld = walletModel->getAddressTableModel()->labelForAddress(rec.address); @@ -669,7 +681,7 @@ void SendWidget::onCoinControlClicked() setCoinControlPayAmounts(); coinControlDialog->exec(); ui->btnCoinControl->setActive(coinControlDialog->coinControl->HasSelected()); - refreshAmounts(); + tryRefreshAmounts(); } else { inform(tr("You don't have any %1 to select.").arg(CURRENCY_UNIT.c_str())); } @@ -752,7 +764,7 @@ void SendWidget::setCoinControlPayAmounts() void SendWidget::onValueChanged() { - refreshAmounts(); + tryRefreshAmounts(); } void SendWidget::onCheckBoxChanged() @@ -760,7 +772,7 @@ void SendWidget::onCheckBoxChanged() const bool checked = ui->checkBoxDelegations->isChecked(); if (checked != fDelegationsChecked) { fDelegationsChecked = checked; - refreshAmounts(); + tryRefreshAmounts(); } } @@ -776,7 +788,7 @@ void SendWidget::onPIVSelected(bool _isTransparent) resetChangeAddress(); resetCoinControl(); - refreshAmounts(); + tryRefreshAmounts(); updateStyle(coinIcon); } @@ -946,7 +958,7 @@ void SendWidget::onDeleteClicked() focusedEntry = nullptr; // Update total amounts - refreshAmounts(); + tryRefreshAmounts(); setFocusOnLastEntry(); } } diff --git a/src/qt/pivx/send.h b/src/qt/pivx/send.h index 6feac119ad20..2687b17ad04f 100644 --- a/src/qt/pivx/send.h +++ b/src/qt/pivx/send.h @@ -57,6 +57,7 @@ public Q_SLOTS: void onValueChanged(); void refreshAmounts(); void changeTheme(bool isLightTheme, QString &theme) override; + void updateAmounts(const QString& titleTotalRemaining, const QString& labelAmountSend, const QString& labelAmountRemaining); protected: void resizeEvent(QResizeEvent *event) override; @@ -99,6 +100,9 @@ private Q_SLOTS: Optional processingResultError{nullopt}; std::atomic processingResult{false}; + // Balance update + std::atomic isUpdatingBalance{false}; + ContactsDropdown *menuContacts = nullptr; TooltipMenu *menu = nullptr; // Current focus entry @@ -114,12 +118,13 @@ private Q_SLOTS: bool sendFinalStep(); void setFocusOnLastEntry(); void showHideCheckBoxDelegations(); - void updateEntryLabels(QList recipients); + void updateEntryLabels(const QList& recipients); void setCustomFeeSelected(bool isSelected, const CAmount& customFee = DEFAULT_TRANSACTION_FEE); void setCoinControlPayAmounts(); void resetCoinControl(); void resetChangeAddress(); void hideContactsMenu(); + void tryRefreshAmounts(); }; #endif // SEND_H From 6e49a1177903ada3495a56c0ec93c70b384c8368 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 25 Feb 2021 21:13:34 -0300 Subject: [PATCH 42/63] GUI: send screen, initialize coinControlDialog view only when it's being called. future: move workload to a background worker thread. --- src/qt/coincontroldialog.h | 1 + src/qt/pivx/send.cpp | 13 ++++++++----- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/qt/coincontroldialog.h b/src/qt/coincontroldialog.h index 0d60c1288ae1..79faa027e328 100644 --- a/src/qt/coincontroldialog.h +++ b/src/qt/coincontroldialog.h @@ -65,6 +65,7 @@ class CoinControlDialog : public QDialog void clearPayAmounts(); void addPayAmount(const CAmount& amount, bool isShieldedRecipient); void setSelectionType(bool isTransparent) { fSelectTransparent = isTransparent; } + bool hasModel() { return model; } CCoinControl* coinControl{nullptr}; diff --git a/src/qt/pivx/send.cpp b/src/qt/pivx/send.cpp index c5c21e46cb5d..277b80b8a051 100644 --- a/src/qt/pivx/send.cpp +++ b/src/qt/pivx/send.cpp @@ -192,7 +192,6 @@ void SendWidget::loadClientModel() void SendWidget::loadWalletModel() { if (walletModel) { - coinControlDialog->setModel(walletModel); if (walletModel->getOptionsModel()) { // display unit nDisplayUnit = walletModel->getOptionsModel()->getDisplayUnit(); @@ -254,13 +253,13 @@ void SendWidget::onResetCustomOptions(bool fRefreshAmounts) void SendWidget::resetCoinControl() { - coinControlDialog->coinControl->SetNull(); + if (coinControlDialog) coinControlDialog->coinControl->SetNull(); ui->btnCoinControl->setActive(false); } void SendWidget::resetChangeAddress() { - coinControlDialog->coinControl->destChange = CNoDestination(); + if (coinControlDialog) coinControlDialog->coinControl->destChange = CNoDestination(); ui->btnChangeAddress->setActive(false); ui->btnChangeAddress->setVisible(isTransparent); } @@ -343,7 +342,7 @@ void SendWidget::showHideCheckBoxDelegations() { // Show checkbox only when there is any available owned delegation and // coincontrol is not selected, and we are trying to spend transparent PIVs. - const bool isCControl = coinControlDialog->coinControl->HasSelected(); + const bool isCControl = coinControlDialog ? coinControlDialog->coinControl->HasSelected() : false; const bool hasDel = cachedDelegatedBalance > 0; const bool showCheckBox = isTransparent && !isCControl && hasDel; @@ -471,7 +470,9 @@ OperationResult SendWidget::prepareTransparent(WalletModelTransaction* currentTr if (!walletModel) return errorOut("Error, no wallet model loaded"); // prepare transaction for getting txFee earlier WalletModel::SendCoinsReturn prepareStatus; - prepareStatus = walletModel->prepareTransaction(currentTransaction, coinControlDialog->coinControl, fDelegationsChecked); + prepareStatus = walletModel->prepareTransaction(currentTransaction, + coinControlDialog ? coinControlDialog->coinControl : nullptr, + fDelegationsChecked); // process prepareStatus and on error generate message shown to user CClientUIInterface::MessageBoxFlags informType; @@ -676,6 +677,8 @@ void SendWidget::onChangeCustomFeeClicked() void SendWidget::onCoinControlClicked() { if (walletModel->getBalance() > 0) { + // future: move coin control initialization and refresh to a worker thread. + if (!coinControlDialog->hasModel()) coinControlDialog->setModel(walletModel); coinControlDialog->setSelectionType(isTransparent); coinControlDialog->refreshDialog(); setCoinControlPayAmounts(); From 44d9cbe2f0c1197cbd2a3759048c96a66ff930f8 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 25 Feb 2021 21:33:56 -0300 Subject: [PATCH 43/63] GUI: dashboard screen, remove stakes filter source model if chart is not being presented. Plus set the tx filter source model only when all of the init values were set to not re-sort/order data. --- src/qt/pivx/dashboardwidget.cpp | 36 +++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/src/qt/pivx/dashboardwidget.cpp b/src/qt/pivx/dashboardwidget.cpp index 16adb55ae516..12875862ba2a 100644 --- a/src/qt/pivx/dashboardwidget.cpp +++ b/src/qt/pivx/dashboardwidget.cpp @@ -181,7 +181,6 @@ void DashboardWidget::loadWalletModel() filter->setSortCaseSensitivity(Qt::CaseInsensitive); filter->setFilterCaseSensitivity(Qt::CaseInsensitive); filter->setSortRole(Qt::EditRole); - filter->setSourceModel(txModel); // Read filter settings QSettings settings; @@ -190,10 +189,10 @@ void DashboardWidget::loadWalletModel() filterByType = (filterIndex == -1) ? TransactionFilterProxy::ALL_TYPES : filterByType; filter->setTypeFilter(filterByType); // Set filter ui->comboBoxSortType->setCurrentIndex(filterIndex); // Set item in ComboBox - // Read sort settings changeSort(settings.value("transactionSort", SortTx::DATE_DESC).toInt()); + filter->setSourceModel(txModel); txHolder->setFilter(filter); ui->listTransactions->setModel(filter); ui->listTransactions->setModelColumn(TransactionTableModel::ToAddress); @@ -212,15 +211,9 @@ void DashboardWidget::loadWalletModel() // Notification pop-up for new transaction connect(txModel, &TransactionTableModel::rowsInserted, this, &DashboardWidget::processNewTransaction); #ifdef USE_QTCHARTS - // chart filter - stakesFilter = new TransactionFilterProxy(); - stakesFilter->setDynamicSortFilter(true); - stakesFilter->setOnlyStakes(true); - stakesFilter->setSourceModel(txModel); - hasStakes = stakesFilter->rowCount() > 0; - onHideChartsChanged(walletModel->getOptionsModel()->isHideCharts()); - connect(walletModel->getOptionsModel(), &OptionsModel::hideChartsChanged, this, &DashboardWidget::onHideChartsChanged); + connect(walletModel->getOptionsModel(), &OptionsModel::hideChartsChanged, this, + &DashboardWidget::onHideChartsChanged); #endif } // update the display unit, to not use the default ("PIV") @@ -233,7 +226,7 @@ void DashboardWidget::onTxArrived(const QString& hash, const bool& isCoinStake, #ifdef USE_QTCHARTS if (isCoinStake) { // Update value if this is our first stake - if (!hasStakes) + if (!hasStakes && stakesFilter) hasStakes = stakesFilter->rowCount() > 0; tryChartRefresh(); } @@ -307,7 +300,6 @@ void DashboardWidget::changeSort(int nSortIndex) ui->comboBoxSort->setCurrentIndex(nSortIndex); filter->sort(nColumnIndex, order); - ui->listTransactions->update(); // Store settings QSettings settings; @@ -415,7 +407,7 @@ void DashboardWidget::loadChart() void DashboardWidget::showHideEmptyChart(bool showEmpty, bool loading, bool forceView) { - if (stakesFilter->rowCount() > SHOW_EMPTY_CHART_VIEW_THRESHOLD || forceView) { + if ((stakesFilter && stakesFilter->rowCount() > SHOW_EMPTY_CHART_VIEW_THRESHOLD) || forceView) { ui->layoutChart->setVisible(!showEmpty); ui->emptyContainerChart->setVisible(showEmpty); } @@ -487,6 +479,7 @@ void DashboardWidget::changeChartColors() void DashboardWidget::updateStakeFilter() { + if (!stakesFilter) return; if (chartShow != ALL) { bool filterByMonth = false; if (monthFilter != 0 && chartShow == MONTH) { @@ -874,6 +867,23 @@ void DashboardWidget::windowResizeEvent(QResizeEvent* event) void DashboardWidget::onHideChartsChanged(bool fHide) { fShowCharts = !fHide; + + if (fShowCharts) { + if (!stakesFilter) { + stakesFilter = new TransactionFilterProxy(this); + stakesFilter->setDynamicSortFilter(false); + stakesFilter->setSortCaseSensitivity(Qt::CaseInsensitive); + stakesFilter->setFilterCaseSensitivity(Qt::CaseInsensitive); + stakesFilter->setOnlyStakes(true); + } + stakesFilter->setSourceModel(txModel); + hasStakes = stakesFilter->rowCount() > 0; + } else { + if (stakesFilter) { + stakesFilter->setSourceModel(nullptr); + } + } + // Hide charts if requested ui->right->setVisible(fShowCharts); if (fShowCharts) tryChartRefresh(); From c2d66d024e03012e2be90c3c012c7340e0180eda Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 25 Feb 2021 22:31:21 -0300 Subject: [PATCH 44/63] GUI: cold staking screen, do not initialize coin control dialog at startup. --- src/qt/pivx/coldstakingwidget.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qt/pivx/coldstakingwidget.cpp b/src/qt/pivx/coldstakingwidget.cpp index 10af040dbfb9..71b75b6a1fc9 100644 --- a/src/qt/pivx/coldstakingwidget.cpp +++ b/src/qt/pivx/coldstakingwidget.cpp @@ -204,7 +204,6 @@ ColdStakingWidget::ColdStakingWidget(PIVXGUI* parent) : void ColdStakingWidget::loadWalletModel() { if (walletModel) { - coinControlDialog->setModel(walletModel); sendMultiRow->setWalletModel(walletModel); txModel = walletModel->getTransactionTableModel(); csModel = new ColdStakingModel(walletModel, txModel, walletModel->getAddressTableModel(), this); @@ -213,10 +212,10 @@ void ColdStakingWidget::loadWalletModel() addressTableModel = walletModel->getAddressTableModel(); addressesFilter = new AddressFilterProxyModel(AddressTableModel::ColdStaking, this); - addressesFilter->setSourceModel(addressTableModel); addressesFilter->sort(sortType, sortOrder); - ui->listViewStakingAddress->setModel(addressesFilter); + addressesFilter->setSourceModel(addressTableModel); ui->listViewStakingAddress->setModelColumn(AddressTableModel::Address); + ui->listViewStakingAddress->setModel(addressesFilter); connect(txModel, &TransactionTableModel::txArrived, this, &ColdStakingWidget::onTxArrived); @@ -533,6 +532,7 @@ void ColdStakingWidget::onCoinControlClicked() { if (isInDelegation) { if (walletModel->getBalance() > 0) { + if (!coinControlDialog->hasModel()) coinControlDialog->setModel(walletModel); coinControlDialog->refreshDialog(); setCoinControlPayAmounts(); coinControlDialog->exec(); From 0a61f7f57a326e1a9771325e48b952945dff6b71 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 25 Feb 2021 22:49:03 -0300 Subject: [PATCH 45/63] GUI: cold staking, do not refresh delegations if the screen is not visible. Same for dashboard. --- src/qt/pivx/coldstakingwidget.cpp | 8 ++++++-- src/qt/pivx/coldstakingwidget.h | 2 ++ src/qt/pivx/dashboardwidget.cpp | 2 ++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/qt/pivx/coldstakingwidget.cpp b/src/qt/pivx/coldstakingwidget.cpp index 71b75b6a1fc9..d069b5e4963a 100644 --- a/src/qt/pivx/coldstakingwidget.cpp +++ b/src/qt/pivx/coldstakingwidget.cpp @@ -224,8 +224,6 @@ void ColdStakingWidget::loadWalletModel() ui->containerHistoryLabel->setVisible(false); ui->emptyContainer->setVisible(false); ui->listView->setVisible(false); - - tryRefreshDelegations(); } } @@ -245,8 +243,14 @@ void ColdStakingWidget::walletSynced(bool sync) } } +void ColdStakingWidget::showEvent(QShowEvent *event) +{ + tryRefreshDelegations(); +} + void ColdStakingWidget::tryRefreshDelegations() { + if (!isVisible()) return; // Check for min update time to not reload the UI so often if the node is syncing. int64_t now = GetTime(); if (lastRefreshTime + LOAD_MIN_TIME_INTERVAL < now) { diff --git a/src/qt/pivx/coldstakingwidget.h b/src/qt/pivx/coldstakingwidget.h index 385dacf1f9f8..1fc256756189 100644 --- a/src/qt/pivx/coldstakingwidget.h +++ b/src/qt/pivx/coldstakingwidget.h @@ -48,6 +48,8 @@ class ColdStakingWidget : public PWidget void run(int type) override; void onError(QString error, int type) override; + void showEvent(QShowEvent *event) override; + public Q_SLOTS: void walletSynced(bool sync); diff --git a/src/qt/pivx/dashboardwidget.cpp b/src/qt/pivx/dashboardwidget.cpp index 12875862ba2a..2a377669d6d5 100644 --- a/src/qt/pivx/dashboardwidget.cpp +++ b/src/qt/pivx/dashboardwidget.cpp @@ -223,6 +223,7 @@ void DashboardWidget::loadWalletModel() void DashboardWidget::onTxArrived(const QString& hash, const bool& isCoinStake, const bool& isCSAnyType) { showList(); + if (!isVisible()) return; #ifdef USE_QTCHARTS if (isCoinStake) { // Update value if this is our first stake @@ -333,6 +334,7 @@ void DashboardWidget::walletSynced(bool sync) this->isSync = sync; ui->layoutWarning->setVisible(!this->isSync); #ifdef USE_QTCHARTS + if (!isVisible()) return; tryChartRefresh(); #endif } From dee42248e57521cca20ad69922409e5ac7b9fa1e Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 25 Feb 2021 22:55:56 -0300 Subject: [PATCH 46/63] Wallet:GetLockedCoins() loop only over the locked coins, not over the entire wallet txes map. Plus return early if no coins were locked. --- src/wallet/wallet.cpp | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c5041b124ae2..71cfd594c569 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2129,10 +2129,20 @@ CAmount CWallet::GetLockedCoins() const { if (fLiteMode) return 0; - return loopTxsBalance([](const uint256& id, const CWalletTx& pcoin, CAmount& nTotal) { - if (pcoin.IsTrusted() && pcoin.GetDepthInMainChain() > 0) - nTotal += pcoin.GetLockedCredit(); - }); + LOCK(cs_wallet); + if (setLockedCoins.empty()) return 0; + + CAmount ret = 0; + for (const auto& coin : setLockedCoins) { + auto it = mapWallet.find(coin.hash); + if (it != mapWallet.end()) { + const CWalletTx& pcoin = it->second; + if (pcoin.IsTrusted() && pcoin.GetDepthInMainChain() > 0) { + ret += it->second.tx->vout.at(coin.n).nValue; + } + } + } + return ret; } CAmount CWallet::GetUnconfirmedBalance(isminetype filter) const From 0f197ca154fc97bb9fb143125404beae4946cc32 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 25 Feb 2021 23:24:41 -0300 Subject: [PATCH 47/63] Wallet interface: do not load not used cold staking balances. None of the cold staking balances are being used from this helper class at the moment, the CS screen has its own model and is calculating the values on-demand. Guarding them behind a flag to be activated in the future if they are needed. --- src/interfaces/wallet.cpp | 6 ++++-- src/interfaces/wallet.h | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 9f67ba079ba7..894c6f0c7baf 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -19,8 +19,10 @@ namespace interfaces { result.unconfirmed_watch_only_balance = m_wallet.GetUnconfirmedWatchOnlyBalance(); result.immature_watch_only_balance = m_wallet.GetImmatureWatchOnlyBalance(); } - result.delegate_balance = m_wallet.GetDelegatedBalance(); - result.coldstaked_balance = m_wallet.GetColdStakingBalance(); + if (result.have_coldstaking) { // At the moment, the GUI is not using these two balances. + result.delegate_balance = m_wallet.GetDelegatedBalance(); + result.coldstaked_balance = m_wallet.GetColdStakingBalance(); + } result.shielded_balance = m_wallet.GetAvailableShieldedBalance(); result.unconfirmed_shielded_balance = m_wallet.GetUnconfirmedShieldedBalance(); return result; diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index b1d8aa6f363f..b5a08633ec06 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -21,6 +21,7 @@ struct WalletBalances CAmount watch_only_balance{0}; CAmount unconfirmed_watch_only_balance{0}; CAmount immature_watch_only_balance{0}; + bool have_coldstaking{false}; CAmount delegate_balance{0}; CAmount coldstaked_balance{0}; CAmount shielded_balance{0}; From b4ab28696fb296823079630b9657cbe872e39e57 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 25 Feb 2021 23:34:38 -0300 Subject: [PATCH 48/63] GUI: dashboard, charts update delay time increased for IBD. --- src/qt/pivx/dashboardwidget.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qt/pivx/dashboardwidget.cpp b/src/qt/pivx/dashboardwidget.cpp index 2a377669d6d5..7be485bafea6 100644 --- a/src/qt/pivx/dashboardwidget.cpp +++ b/src/qt/pivx/dashboardwidget.cpp @@ -361,7 +361,9 @@ void DashboardWidget::tryChartRefresh() } else { // Check for min update time to not reload the UI so often if the node is syncing. int64_t now = GetTime(); - if (lastRefreshTime + CHART_LOAD_MIN_TIME_INTERVAL < now) { + int chartLoadIntervalTime = CHART_LOAD_MIN_TIME_INTERVAL; + if (clientModel->inInitialBlockDownload()) chartLoadIntervalTime *= 6; // 90 seconds update + if (lastRefreshTime + chartLoadIntervalTime < now) { lastRefreshTime = now; refreshChart(); } From 69b3330923ab0060cc8bb15b984ba52ce27a409f Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 26 Feb 2021 10:27:04 -0300 Subject: [PATCH 49/63] GUI: txmodel, do not lock cs_wallet if no wtx status update is needed. --- src/qt/transactiontablemodel.cpp | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp index fb4f9a79ddd5..8472467c2d7a 100644 --- a/src/qt/transactiontablemodel.cpp +++ b/src/qt/transactiontablemodel.cpp @@ -303,25 +303,21 @@ class TransactionTablePriv { if (idx >= 0 && idx < cachedWallet.size()) { TransactionRecord* rec = &cachedWallet[idx]; - - // Get required locks upfront. This avoids the GUI from getting - // stuck if the core is holding the locks for a longer time - for - // example, during a wallet rescan. - // - // If a status update is needed (blocks came in since last check), - // update the status of this transaction from the wallet. Otherwise, - // simply re-use the cached status. - TRY_LOCK(wallet->cs_wallet, lockWallet); - if (lockWallet && rec->statusUpdateNeeded(cur_block_num)) { - std::map::iterator mi = wallet->mapWallet.find(rec->hash); - - if (mi != wallet->mapWallet.end()) { - rec->updateStatus(mi->second, cur_block_num); + if (!cur_block_hash.IsNull() && rec->statusUpdateNeeded(cur_block_num)) { + // If a status update is needed (blocks came in since last check), + // update the status of this transaction from the wallet. Otherwise, + // simply re-use the cached status. + TRY_LOCK(wallet->cs_wallet, lockWallet); + if (lockWallet) { + auto mi = wallet->mapWallet.find(rec->hash); + if (mi != wallet->mapWallet.end()) { + rec->updateStatus(mi->second, cur_block_num); + } } } return rec; } - return 0; + return nullptr; } }; From 8762e81dd390863e51e6f9bb1b0633ad53e059e9 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 16 Dec 2018 20:29:56 +0200 Subject: [PATCH 50/63] Refactoring with QString::toNSString The behavior of MacNotificationHandler::showNotification() has not been changed. --- src/qt/macnotificationhandler.mm | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/src/qt/macnotificationhandler.mm b/src/qt/macnotificationhandler.mm index 3a3a6a339c29..080405e9792f 100644 --- a/src/qt/macnotificationhandler.mm +++ b/src/qt/macnotificationhandler.mm @@ -25,25 +25,10 @@ - (NSString *)__bundleIdentifier { // check if users OS has support for NSUserNotification if(this->hasUserNotificationCenterSupport()) { - // okay, seems like 10.8+ - QByteArray utf8 = title.toUtf8(); - char* cString = (char *)utf8.constData(); - NSString *titleMac = [[NSString alloc] initWithUTF8String:cString]; - - utf8 = text.toUtf8(); - cString = (char *)utf8.constData(); - NSString *textMac = [[NSString alloc] initWithUTF8String:cString]; - - // do everything weak linked (because we will keep <10.8 compatibility) - id userNotification = [[NSClassFromString(@"NSUserNotification") alloc] init]; - [userNotification performSelector:@selector(setTitle:) withObject:titleMac]; - [userNotification performSelector:@selector(setInformativeText:) withObject:textMac]; - - id notificationCenterInstance = [NSClassFromString(@"NSUserNotificationCenter") performSelector:@selector(defaultUserNotificationCenter)]; - [notificationCenterInstance performSelector:@selector(deliverNotification:) withObject:userNotification]; - - [titleMac release]; - [textMac release]; + NSUserNotification* userNotification = [[NSUserNotification alloc] init]; + userNotification.title = title.toNSString(); + userNotification.informativeText = text.toNSString(); + [[NSUserNotificationCenter defaultUserNotificationCenter] deliverNotification: userNotification]; [userNotification release]; } } From 7cb827608417fd786390a0d9dab68513efb2062c Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 26 Feb 2021 11:34:05 -0300 Subject: [PATCH 51/63] init: move "Done loading" message and rpc warm up finished after the wallet post initialization process --- src/init.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 10e6fea7187d..b15b926e95dd 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1971,11 +1971,9 @@ bool AppInitMain() // ********************************************************* Step 12: finished - SetRPCWarmupFinished(); - uiInterface.InitMessage(_("Done loading")); - #ifdef ENABLE_WALLET if (pwalletMain) { + uiInterface.InitMessage(_("Reaccepting wallet transactions...")); pwalletMain->postInitProcess(scheduler); // StakeMiner thread disabled by default on regtest @@ -1985,5 +1983,8 @@ bool AppInitMain() } #endif + SetRPCWarmupFinished(); + uiInterface.InitMessage(_("Done loading")); + return true; } From 3fb3f34132e96248384befc4193ca8234d20583f Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 26 Feb 2021 12:07:04 -0300 Subject: [PATCH 52/63] GUI dashboard: do not update the staking filter if it's not needed. --- src/qt/pivx/dashboardwidget.cpp | 9 +++++++-- src/qt/pivx/dashboardwidget.h | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/qt/pivx/dashboardwidget.cpp b/src/qt/pivx/dashboardwidget.cpp index 7be485bafea6..7f29578ccc3c 100644 --- a/src/qt/pivx/dashboardwidget.cpp +++ b/src/qt/pivx/dashboardwidget.cpp @@ -521,7 +521,10 @@ void DashboardWidget::updateStakeFilter() // pair PIV, zPIV const QMap> DashboardWidget::getAmountBy() { - updateStakeFilter(); + if (filterUpdateNeeded) { + filterUpdateNeeded = false; + updateStakeFilter(); + } const int size = stakesFilter->rowCount(); QMap> amountBy; // Get all of the stakes @@ -617,6 +620,7 @@ void DashboardWidget::onChartYearChanged(const QString& yearStr) int newYear = yearStr.toInt(); if (newYear != yearFilter) { yearFilter = newYear; + filterUpdateNeeded = true; refreshChart(); } } @@ -628,6 +632,7 @@ void DashboardWidget::onChartMonthChanged(const QString& monthStr) int newMonth = ui->comboBoxMonths->currentData().toInt(); if (newMonth != monthFilter) { monthFilter = newMonth; + filterUpdateNeeded = true; refreshChart(); #ifndef Q_OS_MAC // quick hack to re paint the chart view. @@ -820,7 +825,7 @@ void DashboardWidget::onChartArrowClicked(bool goLeft) } } } - + filterUpdateNeeded = true; refreshChart(); //Check if data end day is current date and monthfilter is current month bool fEndDayisCurrent = dataenddate == currentDate.day() && monthFilter == currentDate.month(); diff --git a/src/qt/pivx/dashboardwidget.h b/src/qt/pivx/dashboardwidget.h index 40596db98891..f2092c92d6fb 100644 --- a/src/qt/pivx/dashboardwidget.h +++ b/src/qt/pivx/dashboardwidget.h @@ -170,6 +170,7 @@ private Q_SLOTS: ChartData* chartData{nullptr}; bool hasStakes{false}; bool fShowCharts{true}; + std::atomic filterUpdateNeeded{false}; void initChart(); void showHideEmptyChart(bool show, bool loading, bool forceView = false); From bc7618686157d0b0012a678856360b6f0039116f Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 26 Feb 2021 12:16:07 -0300 Subject: [PATCH 53/63] wallet model: update delay increased to 5 seconds. --- src/qt/walletmodel.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 7fe21f59ed4d..b6aca41ea06f 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -49,7 +49,7 @@ WalletModel::WalletModel(CWallet* wallet, OptionsModel* optionsModel, QObject* p // This timer will be fired repeatedly to update the balance pollTimer = new QTimer(this); connect(pollTimer, &QTimer::timeout, this, &WalletModel::pollBalanceChanged); - pollTimer->start(MODEL_UPDATE_DELAY); + pollTimer->start(MODEL_UPDATE_DELAY * 5); subscribeToCoreSignals(); } @@ -241,7 +241,7 @@ void WalletModel::pollBalanceChanged() if (IsImportingOrReindexing() || m_client_model->inInitialBlockDownload()) { static uint8_t waitLonger = 0; waitLonger++; - if (waitLonger < 30) // 30 seconds + if (waitLonger < 6) // 30 seconds return; waitLonger = 0; } From 63f3fa7b8f4af09b4994651b2238cae8a47a8d4e Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 26 Feb 2021 12:22:44 -0300 Subject: [PATCH 54/63] GUI: add missing qt metatype declarations. --- src/qt/pivx.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qt/pivx.cpp b/src/qt/pivx.cpp index 6f644f6340c3..63fe0e7574e9 100644 --- a/src/qt/pivx.cpp +++ b/src/qt/pivx.cpp @@ -25,6 +25,7 @@ #ifdef ENABLE_WALLET #include "paymentserver.h" #include "walletmodel.h" +#include "interfaces/wallet.h" #endif #include "masternodeconfig.h" @@ -69,6 +70,8 @@ Q_IMPORT_PLUGIN(QGifPlugin); // Declare meta types used for QMetaObject::invokeMethod Q_DECLARE_METATYPE(bool*) Q_DECLARE_METATYPE(CAmount) +Q_DECLARE_METATYPE(interfaces::WalletBalances); +Q_DECLARE_METATYPE(uint256) static void InitMessage(const std::string& message) { @@ -571,6 +574,8 @@ int main(int argc, char* argv[]) // Need to pass name here as CAmount is a typedef (see http://qt-project.org/doc/qt-5/qmetatype.html#qRegisterMetaType) // IMPORTANT if it is no longer a typedef use the normal variant above qRegisterMetaType("CAmount"); + qRegisterMetaType("interfaces::WalletBalances"); + qRegisterMetaType("size_t"); /// 3. Application identification // must be set before OptionsModel is initialized or translations are loaded, From 565884483de69db350d781c775ddc57cba807a0b Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 26 Feb 2021 18:32:39 -0300 Subject: [PATCH 55/63] wallet: removing unnecessary `mempool.cs` lock from ReacceptWalletTransactions() --- src/wallet/wallet.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 71cfd594c569..dbaf4897c590 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1930,11 +1930,9 @@ void CWallet::ReacceptWalletTransactions(bool fFirstLoad) } // Try to add wallet transactions to memory pool - for (std::pair& item: mapSorted) - { + for (std::pair& item: mapSorted) { CWalletTx& wtx = *(item.second); - LOCK(mempool.cs); CValidationState state; bool fSuccess = wtx.AcceptToMemoryPool(state, false); if (!fSuccess && fFirstLoad && GetTime() - wtx.GetTxTime() > 12*60*60) { From 62cd35f36f82c776a249906530757366a34d6407 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 28 Feb 2021 10:19:17 -0300 Subject: [PATCH 56/63] GUI: MN model, remove now unneeded cs_main locks. --- src/qt/pivx/mnmodel.cpp | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/src/qt/pivx/mnmodel.cpp b/src/qt/pivx/mnmodel.cpp index 31bb1ba8690a..04ae3c5430cb 100644 --- a/src/qt/pivx/mnmodel.cpp +++ b/src/qt/pivx/mnmodel.cpp @@ -22,7 +22,7 @@ void MNModel::updateMNList() int end = nodes.size(); nodes.clear(); collateralTxAccepted.clear(); - for (CMasternodeConfig::CMasternodeEntry mne : masternodeConfig.getEntries()) { + for (const CMasternodeConfig::CMasternodeEntry& mne : masternodeConfig.getEntries()) { int nIndex; if (!mne.castOutputIndex(nIndex)) continue; @@ -35,14 +35,9 @@ void MNModel::updateMNList() } nodes.insert(QString::fromStdString(mne.getAlias()), std::make_pair(QString::fromStdString(mne.getIp()), pmn)); if (pwalletMain) { - bool txAccepted = false; - { - LOCK2(cs_main, pwalletMain->cs_wallet); - const CWalletTx *walletTx = pwalletMain->GetWalletTx(txHash); - if (walletTx && walletTx->GetDepthInMainChain() >= MasternodeCollateralMinConf()) { - txAccepted = true; - } - } + const CWalletTx *walletTx = pwalletMain->GetWalletTx(txHash); + bool txAccepted = walletTx && + WITH_LOCK(pwalletMain->cs_wallet, return walletTx->GetDepthInMainChain()) >= MasternodeCollateralMinConf(); collateralTxAccepted.insert(mne.getTxHash(), txAccepted); } } @@ -115,13 +110,8 @@ QVariant MNModel::data(const QModelIndex &index, int role) const if (!isAvailable) return false; std::string txHash = rec->vin.prevout.hash.GetHex(); if (!collateralTxAccepted.value(txHash)) { - bool txAccepted = false; - { - LOCK2(cs_main, pwalletMain->cs_wallet); - const CWalletTx *walletTx = pwalletMain->GetWalletTx(rec->vin.prevout.hash); - txAccepted = walletTx && walletTx->GetDepthInMainChain() > 0; - } - return txAccepted; + const CWalletTx *walletTx = pwalletMain->GetWalletTx(rec->vin.prevout.hash); + return walletTx && WITH_LOCK(pwalletMain->cs_wallet, return walletTx->GetDepthInMainChain()) > 0; } return true; } From 32538ae0d47a44df07dabe0a691a93a6e6db16e8 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 28 Feb 2021 20:49:29 -0300 Subject: [PATCH 57/63] wallet: Disallow abandon of conflicted txes Coming from btc@fa795cf9c52b82cc3cccd21483360d6e03f767f0 --- src/wallet/rpcwallet.cpp | 27 +++++++++++++---------- src/wallet/wallet.cpp | 2 +- test/functional/wallet_abandonconflict.py | 6 +++++ 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index c2b20478a8c3..6a150e2c32d9 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3056,20 +3056,23 @@ UniValue abandontransaction(const JSONRPCRequest& request) { if (request.fHelp || request.params.size() != 1) throw std::runtime_error( - "abandontransaction \"txid\"\n" - "\nMark in-wallet transaction \"txid\" as abandoned\n" - "This will mark this transaction and all its in-wallet descendants as abandoned which will allow\n" - "for their inputs to be respent. It can be used to replace \"stuck\" or evicted transactions.\n" - "It only works on transactions which are not included in a block and are not currently in the mempool.\n" - "It has no effect on transactions which are already conflicted or abandoned.\n" - "\nArguments:\n" - "1. \"txid\" (string, required) The transaction id\n" - "\nResult:\n" - "\nExamples:\n" - + HelpExampleCli("abandontransaction", "\"1075db55d416d3ca199f55b6084e2115b9345e16c5cf302fc80e9d5fbf5d48d\"") - + HelpExampleRpc("abandontransaction", "\"1075db55d416d3ca199f55b6084e2115b9345e16c5cf302fc80e9d5fbf5d48d\"") + "abandontransaction \"txid\"\n" + "\nMark in-wallet transaction \"txid\" as abandoned\n" + "This will mark this transaction and all its in-wallet descendants as abandoned which will allow\n" + "for their inputs to be respent. It can be used to replace \"stuck\" or evicted transactions.\n" + "It only works on transactions which are not included in a block and are not currently in the mempool.\n" + "It has no effect on transactions which are already abandoned.\n" + "\nArguments:\n" + "1. \"txid\" (string, required) The transaction id\n" + "\nResult:\n" + "\nExamples:\n" + + HelpExampleCli("abandontransaction", + "\"1075db55d416d3ca199f55b6084e2115b9345e16c5cf302fc80e9d5fbf5d48d\"") + + HelpExampleRpc("abandontransaction", + "\"1075db55d416d3ca199f55b6084e2115b9345e16c5cf302fc80e9d5fbf5d48d\"") ); + EnsureWallet(); EnsureWalletIsUnlocked(); // Make sure the results are valid at least up to the most recent block diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index dbaf4897c590..9186e5fcd4f3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1156,7 +1156,7 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) auto it = mapWallet.find(hashTx); assert(it != mapWallet.end()); CWalletTx& origtx = it->second; - if (origtx.GetDepthInMainChain() > 0 || origtx.InMempool()) { + if (origtx.GetDepthInMainChain() != 0 || origtx.InMempool()) { return false; } diff --git a/test/functional/wallet_abandonconflict.py b/test/functional/wallet_abandonconflict.py index f9da3764f554..81b4474a21be 100755 --- a/test/functional/wallet_abandonconflict.py +++ b/test/functional/wallet_abandonconflict.py @@ -7,6 +7,7 @@ from test_framework.test_framework import PivxTestFramework from test_framework.util import ( assert_equal, + assert_raises_rpc_error, connect_nodes, Decimal, disconnect_nodes, @@ -32,6 +33,11 @@ def run_test(self): sync_mempools(self.nodes) self.nodes[1].generate(1) + # Can not abandon non-wallet transaction + assert_raises_rpc_error(-5, 'Invalid or non-wallet transaction id', lambda: self.nodes[0].abandontransaction('ff' * 32)) + # Can not abandon confirmed transaction + assert_raises_rpc_error(-5, 'Transaction not eligible for abandonment', lambda: self.nodes[0].abandontransaction(txA)) + sync_blocks(self.nodes) newbalance = self.nodes[0].getbalance() assert(balance - newbalance < Decimal("0.001")) #no more than fees lost From e9c160a0bf5947b4731c2ba43e4b6a91f4635efd Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 1 Mar 2021 01:48:28 -0300 Subject: [PATCH 58/63] wallet: single loop to calculate the currently required balances. future: add watch-only and CS balances distinction when/if they are needed (at the moment, they aren't). Inspired in btc@fa57411fcba00556ba25d45bca53cc04623da051 . --- src/interfaces/wallet.cpp | 11 ++++++----- src/wallet/wallet.cpp | 26 ++++++++++++++++++++++++++ src/wallet/wallet.h | 9 +++++++++ 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 894c6f0c7baf..cc817af79ffa 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -10,9 +10,10 @@ namespace interfaces { WalletBalances Wallet::getBalances() { WalletBalances result; - result.balance = m_wallet.GetAvailableBalance(); - result.unconfirmed_balance = m_wallet.GetUnconfirmedBalance(ISMINE_SPENDABLE_TRANSPARENT); - result.immature_balance = m_wallet.GetImmatureBalance(); + CWallet::Balance balance = m_wallet.GetBalance(); + result.balance = balance.m_mine_trusted + balance.m_mine_trusted_shield; + result.unconfirmed_balance = balance.m_mine_untrusted_pending; + result.immature_balance = balance.m_mine_immature; result.have_watch_only = m_wallet.HaveWatchOnly(); if (result.have_watch_only) { result.watch_only_balance = m_wallet.GetWatchOnlyBalance(); @@ -23,8 +24,8 @@ namespace interfaces { result.delegate_balance = m_wallet.GetDelegatedBalance(); result.coldstaked_balance = m_wallet.GetColdStakingBalance(); } - result.shielded_balance = m_wallet.GetAvailableShieldedBalance(); - result.unconfirmed_shielded_balance = m_wallet.GetUnconfirmedShieldedBalance(); + result.shielded_balance = balance.m_mine_trusted_shield; + result.unconfirmed_shielded_balance = balance.m_mine_untrusted_shielded_balance; return result; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9186e5fcd4f3..8cea522a99a4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2055,6 +2055,32 @@ void CWallet::ResendWalletTransactions(CConnman* connman) * @{ */ +CWallet::Balance CWallet::GetBalance(const int min_depth) const +{ + Balance ret; + { + LOCK(cs_wallet); + std::set trusted_parents; + for (const auto& entry : mapWallet) { + const CWalletTx& wtx = entry.second; + const bool is_trusted{wtx.IsTrusted()}; + const int tx_depth{wtx.GetDepthInMainChain()}; + const CAmount tx_credit_mine{wtx.GetAvailableCredit(/* fUseCache */ true, ISMINE_SPENDABLE_TRANSPARENT)}; + const CAmount tx_credit_shield_mine{wtx.GetAvailableCredit(/* fUseCache */ true, ISMINE_SPENDABLE_SHIELDED)}; + if (is_trusted && tx_depth >= min_depth) { + ret.m_mine_trusted += tx_credit_mine; + ret.m_mine_trusted_shield += tx_credit_shield_mine; + } + if (!is_trusted && tx_depth == 0 && wtx.InMempool()) { + ret.m_mine_untrusted_pending += tx_credit_mine; + ret.m_mine_untrusted_shielded_balance += tx_credit_shield_mine; + } + ret.m_mine_immature += wtx.GetImmatureCredit(); + } + } + return ret; +} + CAmount CWallet::loopTxsBalance(std::function method) const { CAmount nTotal = 0; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 4729edda2dc6..54ff366f1c5e 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -954,6 +954,15 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface void ReacceptWalletTransactions(bool fFirstLoad = false); void ResendWalletTransactions(CConnman* connman) override; + struct Balance { + CAmount m_mine_trusted{0}; //!< Trusted, at depth=GetBalance.min_depth or more + CAmount m_mine_untrusted_pending{0}; //!< Untrusted, but in mempool (pending) + CAmount m_mine_immature{0}; //!< Immature coinbases/coinstakes in the main chain + CAmount m_mine_trusted_shield{0}; //!< Trusted shield, at depth=GetBalance.min_depth or more + CAmount m_mine_untrusted_shielded_balance{0}; //!< Untrusted shield, but in mempool (pending) + }; + Balance GetBalance(int min_depth = 0) const; + CAmount loopTxsBalance(std::functionmethod) const; CAmount GetAvailableBalance(bool fIncludeDelegated = true, bool fIncludeShielded = true) const; CAmount GetAvailableBalance(isminefilter& filter, bool useCache = false, int minDepth = 1) const; From ef77fdf114c00307fd2b354e9f338abaf53e1b42 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 1 Mar 2021 20:03:01 -0300 Subject: [PATCH 59/63] GUI: topbar, removing not used block source. ClientModel::getBlockSource is locking cs_nodes every second and the result is currently not used. --- src/qt/clientmodel.cpp | 5 +++++ src/qt/clientmodel.h | 1 + src/qt/pivx/topbar.cpp | 23 ++--------------------- 3 files changed, 8 insertions(+), 21 deletions(-) diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp index 00a4fcfb6f28..136faffd829e 100644 --- a/src/qt/clientmodel.cpp +++ b/src/qt/clientmodel.cpp @@ -150,6 +150,11 @@ int64_t ClientModel::getLastBlockProcessedTime() const return cacheTip == nullptr ? Params().GenesisBlock().GetBlockTime() : cacheTip->GetBlockTime(); } +bool ClientModel::isTipCached() const +{ + return cacheTip; +} + double ClientModel::getVerificationProgress() const { return Checkpoints::GuessVerificationProgress(cacheTip); diff --git a/src/qt/clientmodel.h b/src/qt/clientmodel.h index 8ce41d0f5283..f783aaecd931 100644 --- a/src/qt/clientmodel.h +++ b/src/qt/clientmodel.h @@ -68,6 +68,7 @@ class ClientModel : public QObject int getLastBlockProcessedHeight() const; int64_t getLastBlockProcessedTime() const; double getVerificationProgress() const; + bool isTipCached() const; quint64 getTotalBytesRecv() const; quint64 getTotalBytesSent() const; diff --git a/src/qt/pivx/topbar.cpp b/src/qt/pivx/topbar.cpp index 52b3d0e34631..73179e9748f4 100644 --- a/src/qt/pivx/topbar.cpp +++ b/src/qt/pivx/topbar.cpp @@ -474,26 +474,7 @@ void TopBar::setNumBlocks(int count) if (!clientModel) return; - // Acquire current block source - enum BlockSource blockSource = clientModel->getBlockSource(); - std::string text = ""; - switch (blockSource) { - case BLOCK_SOURCE_NETWORK: - text = "Synchronizing.."; - break; - case BLOCK_SOURCE_DISK: - text = "Importing blocks from disk.."; - break; - case BLOCK_SOURCE_REINDEX: - text = "Reindexing blocks on disk.."; - break; - case BLOCK_SOURCE_NONE: - // Case: not Importing, not Reindexing and no network connection - text = "No block source available.."; - ui->pushButtonSync->setChecked(false); - break; - } - + std::string text; bool needState = true; if (masternodeSync.IsBlockchainSynced()) { // chain synced @@ -523,7 +504,7 @@ void TopBar::setNumBlocks(int count) Q_EMIT walletSynced(false); } - if (needState) { + if (needState && clientModel->isTipCached()) { // Represent time from last generated block in human readable text QDateTime lastBlockDate = clientModel->getLastBlockDate(); QDateTime currentDate = QDateTime::currentDateTime(); From cbc50217f2ff405a65b4036d0efb4a8a285c6547 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 1 Mar 2021 20:03:34 -0300 Subject: [PATCH 60/63] Masternode-sync read only function to get the "IsBlockchainSynced" state. --- src/masternode-sync.cpp | 5 +++++ src/masternode-sync.h | 2 ++ src/qt/pivx/topbar.cpp | 4 +--- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/masternode-sync.cpp b/src/masternode-sync.cpp index 243c1e2cc012..a7352ebaf803 100644 --- a/src/masternode-sync.cpp +++ b/src/masternode-sync.cpp @@ -78,6 +78,11 @@ bool CMasternodeSync::IsBlockchainSynced() return true; } +bool CMasternodeSync::IsBlockchainSyncedReadOnly() const +{ + return fBlockchainSynced; +} + void CMasternodeSync::Reset() { fBlockchainSynced = false; diff --git a/src/masternode-sync.h b/src/masternode-sync.h index 69f930f5efc9..41b5dd0c083d 100644 --- a/src/masternode-sync.h +++ b/src/masternode-sync.h @@ -98,6 +98,8 @@ class CMasternodeSync bool IsBlockchainSynced(); void ClearFulfilledRequest(); + bool IsBlockchainSyncedReadOnly() const; + // Sync message dispatcher bool MessageDispatcher(CNode* pfrom, std::string& strCommand, CDataStream& vRecv); diff --git a/src/qt/pivx/topbar.cpp b/src/qt/pivx/topbar.cpp index 73179e9748f4..29b9ac02355d 100644 --- a/src/qt/pivx/topbar.cpp +++ b/src/qt/pivx/topbar.cpp @@ -13,13 +13,11 @@ #include "bitcoinunits.h" #include "qt/pivx/balancebubble.h" #include "clientmodel.h" -#include "qt/guiconstants.h" #include "qt/guiutil.h" #include "optionsmodel.h" #include "qt/platformstyle.h" #include "walletmodel.h" #include "addresstablemodel.h" -#include "guiinterface.h" #include "masternode-sync.h" #include "wallet/wallet.h" @@ -476,7 +474,7 @@ void TopBar::setNumBlocks(int count) std::string text; bool needState = true; - if (masternodeSync.IsBlockchainSynced()) { + if (masternodeSync.IsBlockchainSyncedReadOnly()) { // chain synced Q_EMIT walletSynced(true); if (masternodeSync.IsSynced()) { From 02eb781503413c2776bbee6eb048a8b5ef8f9b1c Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 1 Mar 2021 20:11:06 -0300 Subject: [PATCH 61/63] GUI: settings information, do not update block num and block hash if the screen is not visible. --- src/qt/pivx/settings/settingsinformationwidget.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/qt/pivx/settings/settingsinformationwidget.cpp b/src/qt/pivx/settings/settingsinformationwidget.cpp index af536782402e..2294a3ddbf0d 100644 --- a/src/qt/pivx/settings/settingsinformationwidget.cpp +++ b/src/qt/pivx/settings/settingsinformationwidget.cpp @@ -14,7 +14,7 @@ #include -#define REQUEST_UPDATE_MN_COUNT 0 +#define REQUEST_UPDATE_COUNTS 0 SettingsInformationWidget::SettingsInformationWidget(PIVXGUI* _window,QWidget *parent) : PWidget(_window,parent), @@ -141,6 +141,7 @@ void SettingsInformationWidget::setNumConnections(int count) void SettingsInformationWidget::setNumBlocks(int count) { + if (!isVisible()) return; ui->labelInfoBlockNumber->setText(QString::number(count)); if (clientModel) { ui->labelInfoBlockTime->setText(clientModel->getLastBlockDate().toString()); @@ -168,7 +169,7 @@ void SettingsInformationWidget::showEvent(QShowEvent *event) if (clientModel) { clientModel->startMasternodesTimer(); // Initial masternodes count value, running in a worker thread to not lock mnmanager mutex in the main thread. - execute(REQUEST_UPDATE_MN_COUNT); + execute(REQUEST_UPDATE_COUNTS); } } @@ -181,15 +182,17 @@ void SettingsInformationWidget::hideEvent(QHideEvent *event) { void SettingsInformationWidget::run(int type) { - if (type == REQUEST_UPDATE_MN_COUNT) { + if (type == REQUEST_UPDATE_COUNTS) { QMetaObject::invokeMethod(this, "setMasternodeCount", Qt::QueuedConnection, Q_ARG(QString, clientModel->getMasternodesCount())); + QMetaObject::invokeMethod(this, "setNumBlocks", + Qt::QueuedConnection, Q_ARG(int, clientModel->getLastBlockProcessedHeight())); } } void SettingsInformationWidget::onError(QString error, int type) { - if (type == REQUEST_UPDATE_MN_COUNT) { + if (type == REQUEST_UPDATE_COUNTS) { setMasternodeCount(tr("No available data")); } } From a1390c39270d5f0b46fe688bcf7971a67cb8fcd6 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Mar 2021 17:22:00 -0300 Subject: [PATCH 62/63] wallet balances, cache total delegated balance and calculate it only once. gui send screen: remove on-demand delegated balance calculation --- src/interfaces/wallet.cpp | 4 ++-- src/qt/pivx/send.cpp | 34 ++++++++++++++++++++++------------ src/qt/pivx/send.h | 8 +++++--- src/wallet/wallet.cpp | 3 +++ src/wallet/wallet.h | 1 + 5 files changed, 33 insertions(+), 17 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index cc817af79ffa..17e5365ebb95 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -20,8 +20,8 @@ namespace interfaces { result.unconfirmed_watch_only_balance = m_wallet.GetUnconfirmedWatchOnlyBalance(); result.immature_watch_only_balance = m_wallet.GetImmatureWatchOnlyBalance(); } - if (result.have_coldstaking) { // At the moment, the GUI is not using these two balances. - result.delegate_balance = m_wallet.GetDelegatedBalance(); + result.delegate_balance = balance.m_mine_cs_delegated_trusted; + if (result.have_coldstaking) { // At the moment, the GUI is not using the cold staked balance. result.coldstaked_balance = m_wallet.GetColdStakingBalance(); } result.shielded_balance = balance.m_mine_trusted_shield; diff --git a/src/qt/pivx/send.cpp b/src/qt/pivx/send.cpp index 277b80b8a051..b3e21f817e98 100644 --- a/src/qt/pivx/send.cpp +++ b/src/qt/pivx/send.cpp @@ -145,6 +145,7 @@ void SendWidget::refreshAmounts() nDisplayUnit = walletModel->getOptionsModel()->getDisplayUnit(); CAmount totalAmount = 0; + CAmount delegatedBalance = 0; QString titleTotalRemaining; if (coinControlDialog->coinControl->HasSelected()) { // Set remaining balance to the sum of the coinControl selected inputs @@ -157,9 +158,17 @@ void SendWidget::refreshAmounts() totalAmount = selectedBalance - total; titleTotalRemaining = tr("Total remaining from the selected UTXO"); } else { - // Wallet's unlocked balance. - totalAmount = isTransparent ? (walletModel->getUnlockedBalance(nullptr, fDelegationsChecked, false) - total) - : (walletModel->GetWalletBalances().shielded_balance - total); + interfaces::WalletBalances balances = walletModel->GetWalletBalances(); + if (isTransparent) { + totalAmount = balances.balance - balances.shielded_balance - walletModel->getLockedBalance() - total; + if (!fDelegationsChecked) { + totalAmount -= balances.delegate_balance; + } + // show delegated balance if exist + delegatedBalance = balances.delegate_balance; + } else { + totalAmount = balances.shielded_balance - total; + } titleTotalRemaining = tr("Unlocked remaining"); } @@ -168,16 +177,20 @@ void SendWidget::refreshAmounts() QMetaObject::invokeMethod(this, "updateAmounts", Qt::QueuedConnection, Q_ARG(QString, titleTotalRemaining), Q_ARG(QString, GUIUtil::formatBalance(total, nDisplayUnit, false)), - Q_ARG(QString, labelAmountRemaining)); + Q_ARG(QString, labelAmountRemaining), + Q_ARG(CAmount, delegatedBalance)); } -void SendWidget::updateAmounts(const QString& _titleTotalRemaining, const QString& _labelAmountSend, const QString& _labelAmountRemaining) +void SendWidget::updateAmounts(const QString& _titleTotalRemaining, + const QString& _labelAmountSend, + const QString& _labelAmountRemaining, + CAmount _delegationBalance) { ui->labelTitleTotalRemaining->setText(_titleTotalRemaining); ui->labelAmountSend->setText(_labelAmountSend); ui->labelAmountRemaining->setText(_labelAmountRemaining); // show or hide delegations checkbox if need be - showHideCheckBoxDelegations(); + showHideCheckBoxDelegations(_delegationBalance); } void SendWidget::loadClientModel() @@ -338,19 +351,19 @@ void SendWidget::setFocusOnLastEntry() if (!entries.isEmpty()) entries.last()->setFocus(); } -void SendWidget::showHideCheckBoxDelegations() +void SendWidget::showHideCheckBoxDelegations(CAmount delegationBalance) { // Show checkbox only when there is any available owned delegation and // coincontrol is not selected, and we are trying to spend transparent PIVs. const bool isCControl = coinControlDialog ? coinControlDialog->coinControl->HasSelected() : false; - const bool hasDel = cachedDelegatedBalance > 0; + const bool hasDel = delegationBalance > 0; const bool showCheckBox = isTransparent && !isCControl && hasDel; ui->checkBoxDelegations->setVisible(showCheckBox); if (showCheckBox) ui->checkBoxDelegations->setToolTip( tr("Possibly spend coins delegated for cold-staking (currently available: %1").arg( - GUIUtil::formatBalance(cachedDelegatedBalance, nDisplayUnit, false)) + GUIUtil::formatBalance(delegationBalance, nDisplayUnit, false)) ); } @@ -521,9 +534,6 @@ bool SendWidget::sendFinalStep() ); if (sendStatus.status == WalletModel::OK) { - // if delegations were spent, update cachedBalance - if (fStakeDelegationVoided) - cachedDelegatedBalance = walletModel->getDelegatedBalance(); clearAll(false); inform(tr("Transaction sent")); dialog->deleteLater(); diff --git a/src/qt/pivx/send.h b/src/qt/pivx/send.h index 2687b17ad04f..fcc4f9038408 100644 --- a/src/qt/pivx/send.h +++ b/src/qt/pivx/send.h @@ -57,7 +57,10 @@ public Q_SLOTS: void onValueChanged(); void refreshAmounts(); void changeTheme(bool isLightTheme, QString &theme) override; - void updateAmounts(const QString& titleTotalRemaining, const QString& labelAmountSend, const QString& labelAmountRemaining); + void updateAmounts(const QString& titleTotalRemaining, + const QString& labelAmountSend, + const QString& labelAmountRemaining, + CAmount _delegationBalance); protected: void resizeEvent(QResizeEvent *event) override; @@ -88,7 +91,6 @@ private Q_SLOTS: SendCustomFeeDialog* customFeeDialog = nullptr; bool isCustomFeeSelected = false; bool fDelegationsChecked = false; - CAmount cachedDelegatedBalance{0}; int nDisplayUnit; QList entries; @@ -117,7 +119,7 @@ private Q_SLOTS: OperationResult prepareTransparent(WalletModelTransaction* tx); bool sendFinalStep(); void setFocusOnLastEntry(); - void showHideCheckBoxDelegations(); + void showHideCheckBoxDelegations(CAmount delegationBalance); void updateEntryLabels(const QList& recipients); void setCustomFeeSelected(bool isSelected, const CAmount& customFee = DEFAULT_TRANSACTION_FEE); void setCoinControlPayAmounts(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8cea522a99a4..862a7cd6600e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2070,6 +2070,9 @@ CWallet::Balance CWallet::GetBalance(const int min_depth) const if (is_trusted && tx_depth >= min_depth) { ret.m_mine_trusted += tx_credit_mine; ret.m_mine_trusted_shield += tx_credit_shield_mine; + if (wtx.tx->HasP2CSOutputs()) { + ret.m_mine_cs_delegated_trusted += wtx.GetStakeDelegationCredit(); + } } if (!is_trusted && tx_depth == 0 && wtx.InMempool()) { ret.m_mine_untrusted_pending += tx_credit_mine; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 54ff366f1c5e..100221e9b3d8 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -960,6 +960,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface CAmount m_mine_immature{0}; //!< Immature coinbases/coinstakes in the main chain CAmount m_mine_trusted_shield{0}; //!< Trusted shield, at depth=GetBalance.min_depth or more CAmount m_mine_untrusted_shielded_balance{0}; //!< Untrusted shield, but in mempool (pending) + CAmount m_mine_cs_delegated_trusted{0}; //!< Trusted, at depth=GetBalance.min_depth or more. Part of m_mine_trusted as well }; Balance GetBalance(int min_depth = 0) const; From ca3edc5b01e19408615d413888f93d04ecd92a6c Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Mar 2021 17:37:07 -0300 Subject: [PATCH 63/63] GUI: Update worker type if task already exist. So it doesn't invalidly re-use the cached one. --- src/qt/pivx/loadingdialog.h | 1 + src/qt/pivx/pwidget.cpp | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/qt/pivx/loadingdialog.h b/src/qt/pivx/loadingdialog.h index 82f912729066..707945c9c0ee 100644 --- a/src/qt/pivx/loadingdialog.h +++ b/src/qt/pivx/loadingdialog.h @@ -25,6 +25,7 @@ class Worker : public QObject { runnable = nullptr; } virtual void clean() {}; + void setType(int _type) { type = _type; } public Q_SLOTS: void process(); Q_SIGNALS: diff --git a/src/qt/pivx/pwidget.cpp b/src/qt/pivx/pwidget.cpp index 02c826c6cbd6..d9dd52e01b8c 100644 --- a/src/qt/pivx/pwidget.cpp +++ b/src/qt/pivx/pwidget.cpp @@ -108,6 +108,8 @@ bool PWidget::execute(int type, std::unique_ptr pctx WalletWorker* _worker = static_cast(task->worker.data()); _worker->setContext(std::move(pctx)); } + // Update type + task->worker->setType(type); } QThreadPool::globalInstance()->start(task.data()); return true;