From 1fa0d701f764f351ec51b3b485d63b834f9b857d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 6 Feb 2021 11:48:03 -0300 Subject: [PATCH 01/41] Add a CValidationInterface::TransactionRemovedFromMempool This is currently unused, but will by used by wallet to cache when transactions are in the mempool, obviating the need for calls to mempool from CWalletTx::InMempool() --- src/init.cpp | 2 ++ src/txmempool.h | 3 +++ src/validationinterface.cpp | 19 +++++++++++++++++++ src/validationinterface.h | 18 ++++++++++++++++++ 4 files changed, 42 insertions(+) diff --git a/src/init.cpp b/src/init.cpp index db5a288358d8..3d922e203698 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -318,6 +318,7 @@ void PrepareShutdown() // Disconnect all slots UnregisterAllValidationInterfaces(); GetMainSignals().UnregisterBackgroundSignalScheduler(); + GetMainSignals().UnregisterWithMempoolSignals(mempool); #ifndef WIN32 try { @@ -1264,6 +1265,7 @@ bool AppInitMain() threadGroup.create_thread(std::bind(&TraceThread, "scheduler", serviceLoop)); GetMainSignals().RegisterBackgroundSignalScheduler(scheduler); + GetMainSignals().RegisterWithMempoolSignals(mempool); // Initialize Sapling circuit parameters LoadSaplingParams(); diff --git a/src/txmempool.h b/src/txmempool.h index 6e29cd796507..635606d2aa97 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -535,6 +535,9 @@ class CTxMemPool // to track size/count of descendant transactions. First version of // addUnchecked can be used to have it call CalculateMemPoolAncestors(), and // then invoke the second version. + // Note that addUnchecked is ONLY called from ATMP outside of tests + // and any other callers may break wallet's in-mempool tracking (due to + // lack of CValidationInterface::TransactionAddedToMempool callbacks). bool addUnchecked(const uint256& hash, const CTxMemPoolEntry& entry, bool fCurrentEstimate = true); bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool fCurrentEstimate = true); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index d38e04bf1f42..b82cfe5b556b 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -6,6 +6,7 @@ #include "validationinterface.h" #include "scheduler.h" +#include "txmempool.h" #include #include @@ -16,6 +17,7 @@ struct ValidationInterfaceConnections { boost::signals2::scoped_connection TransactionAddedToMempool; boost::signals2::scoped_connection BlockConnected; boost::signals2::scoped_connection BlockDisconnected; + boost::signals2::scoped_connection TransactionRemovedFromMempool; boost::signals2::scoped_connection SetBestChain; boost::signals2::scoped_connection Broadcast; boost::signals2::scoped_connection BlockChecked; @@ -34,6 +36,8 @@ 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; + /** Notifies listeners of a transaction removal from the mempool */ + boost::signals2::signal TransactionRemovedFromMempool; /** Notifies listeners of a new active block chain. */ boost::signals2::signal SetBestChain; /** Tells listeners to broadcast their data. */ @@ -68,6 +72,14 @@ void CMainSignals::FlushBackgroundCallbacks() { } } +void CMainSignals::RegisterWithMempoolSignals(CTxMemPool& pool) { + pool.NotifyEntryRemoved.connect(std::bind(&CMainSignals::MempoolEntryRemoved, this, std::placeholders::_1, std::placeholders::_2)); +} + +void CMainSignals::UnregisterWithMempoolSignals(CTxMemPool& pool) { + pool.NotifyEntryRemoved.disconnect_all_slots(); +} + CMainSignals& GetMainSignals() { return g_signals; @@ -80,6 +92,7 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) 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.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)); conns.BlockChecked = g_signals.m_internals->BlockChecked.connect(std::bind(&CValidationInterface::BlockChecked, pwalletIn, std::placeholders::_1, std::placeholders::_2)); @@ -100,6 +113,12 @@ void UnregisterAllValidationInterfaces() g_signals.m_internals->m_connMainSignals.clear(); } +void CMainSignals::MempoolEntryRemoved(CTransactionRef ptx, MemPoolRemovalReason reason) { + if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) { + m_internals->TransactionRemovedFromMempool(ptx); + } +} + void CMainSignals::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload) { m_internals->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 0a4343070426..09f9f127d8f2 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -19,6 +19,8 @@ class CValidationInterface; class CValidationState; class uint256; class CScheduler; +class CTxMemPool; +enum class MemPoolRemovalReason; // These functions dispatch to one or all registered wallets @@ -36,6 +38,15 @@ class CValidationInterface { /** Notifies listeners of updated block chain tip */ virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {} virtual void TransactionAddedToMempool(const CTransactionRef &ptxn) {} + /** + * Notifies listeners of a transaction leaving mempool. + * + * This only fires for transactions which leave mempool because of expiry, + * size limiting, reorg (changes in lock times/coinbase/coinstake maturity), or + * replacement. This does not include any transactions which are included + * in BlockConnectedDisconnected either in block->vtx or in txnConflicted. + */ + virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx) {} virtual void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::vector &txnConflicted) {} virtual void BlockDisconnected(const std::shared_ptr &block, int nBlockHeight) {} /** Notifies listeners of the new active block chain on-disk. */ @@ -53,6 +64,8 @@ class CMainSignals { private: std::unique_ptr m_internals; + void MempoolEntryRemoved(CTransactionRef tx, MemPoolRemovalReason reason); + friend void ::RegisterValidationInterface(CValidationInterface*); friend void ::UnregisterValidationInterface(CValidationInterface*); friend void ::UnregisterAllValidationInterfaces(); @@ -65,6 +78,11 @@ class CMainSignals { /** Call any remaining callbacks on the calling thread */ void FlushBackgroundCallbacks(); + /** Register with mempool to call TransactionRemovedFromMempool callbacks */ + void RegisterWithMempoolSignals(CTxMemPool& pool); + /** Unregister with mempool */ + void UnregisterWithMempoolSignals(CTxMemPool& pool); + 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::vector &txnConflicted); From 268be9c4de074d56037015c996b49b0831011a02 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 8 Jun 2017 11:05:18 -0400 Subject: [PATCH 02/41] Call TransactionRemovedFromMempool in the CScheduler thread This is both good practice (we want to move all such callbacks into a background thread eventually) and prevents a lock inversion when we go to use this in wallet (mempool.cs->cs_wallet and cs_wallet->mempool.cs would otherwise both be used). --- src/validationinterface.cpp | 4 +++- src/validationinterface.h | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index b82cfe5b556b..52e4c5fa7379 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -115,7 +115,9 @@ void UnregisterAllValidationInterfaces() void CMainSignals::MempoolEntryRemoved(CTransactionRef ptx, MemPoolRemovalReason reason) { if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) { - m_internals->TransactionRemovedFromMempool(ptx); + m_internals->m_schedulerClient.AddToProcessQueue([ptx, this] { + m_internals->TransactionRemovedFromMempool(ptx); + }); } } diff --git a/src/validationinterface.h b/src/validationinterface.h index 09f9f127d8f2..2836103940bf 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -45,6 +45,8 @@ class CValidationInterface { * size limiting, reorg (changes in lock times/coinbase/coinstake maturity), or * replacement. This does not include any transactions which are included * in BlockConnectedDisconnected either in block->vtx or in txnConflicted. + * + * Called on a background thread. */ virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx) {} virtual void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::vector &txnConflicted) {} From 40ed4c4ad5e187c0fbfc83a16ddc661057c0028d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 8 Jun 2017 11:13:46 -0400 Subject: [PATCH 03/41] Add CallFunctionInQueue to wait on validation interface queue drain --- src/validationinterface.cpp | 4 ++++ src/validationinterface.h | 14 ++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 52e4c5fa7379..fe8854f62d0a 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -113,6 +113,10 @@ void UnregisterAllValidationInterfaces() g_signals.m_internals->m_connMainSignals.clear(); } +void CallFunctionInValidationInterfaceQueue(std::function func) { + g_signals.m_internals->m_schedulerClient.AddToProcessQueue(std::move(func)); +} + void CMainSignals::MempoolEntryRemoved(CTransactionRef ptx, MemPoolRemovalReason reason) { if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) { m_internals->m_schedulerClient.AddToProcessQueue([ptx, this] { diff --git a/src/validationinterface.h b/src/validationinterface.h index 2836103940bf..9a6ce3453d30 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -11,6 +11,9 @@ #include "sapling/incrementalmerkletree.h" #include "primitives/transaction.h" +#include +#include + class CBlock; struct CBlockLocator; class CBlockIndex; @@ -30,6 +33,16 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn); void UnregisterValidationInterface(CValidationInterface* pwalletIn); /** Unregister all wallets from core */ void UnregisterAllValidationInterfaces(); +/** + * Pushes a function to callback onto the notification queue, guaranteeing any + * callbacks generated prior to now are finished when the function is called. + * + * Be very careful blocking on func to be called if any locks are held - + * validation interface clients may not be able to make progress as they often + * wait for things like cs_main, so blocking until func is called with cs_main + * will result in a deadlock (that DEBUG_LOCKORDER will miss). + */ +void CallFunctionInValidationInterfaceQueue(std::function func); class CValidationInterface { public: @@ -71,6 +84,7 @@ class CMainSignals { friend void ::RegisterValidationInterface(CValidationInterface*); friend void ::UnregisterValidationInterface(CValidationInterface*); friend void ::UnregisterAllValidationInterfaces(); + friend void ::CallFunctionInValidationInterfaceQueue(std::function func); public: /** Register a CScheduler to give callbacks which should run in the background (may only be called once) */ From 24a3ce45eb4c588befddea6c0324d9abb67dd7f4 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 6 Feb 2021 11:55:26 -0300 Subject: [PATCH 04/41] Add CWallet::BlockUntilSyncedToCurrentChain() This blocks until the wallet has synced up to the current height. --- src/wallet/wallet.cpp | 38 ++++++++++++++++++++++++++++++++++++-- src/wallet/wallet.h | 20 ++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c4f4377a3fa0..70838680f8fa 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -21,6 +21,7 @@ #include "utilmoneystr.h" #include "zpivchain.h" +#include #include CWallet* pwalletMain = nullptr; @@ -1285,6 +1286,8 @@ 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) @@ -1301,6 +1304,35 @@ void CWallet::BlockDisconnected(const std::shared_ptr& pblock, int } } +void CWallet::BlockUntilSyncedToCurrentChain() { + AssertLockNotHeld(cs_main); + AssertLockNotHeld(cs_wallet); + + { + // 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. + LOCK(cs_main); + const CBlockIndex* initialChainTip = chainActive.Tip(); + + if (m_last_block_processed->GetAncestor(initialChainTip->nHeight) == initialChainTip) { + return; + } + } + + // ...otherwise put a callback in the validation interface queue and wait + // for the queue to drain enough to execute it (indicating we are caught up + // at least with the time we entered this function). + + std::promise promise; + CallFunctionInValidationInterfaceQueue([&promise] { + promise.set_value(); + }); + promise.get_future().wait(); +} + void CWallet::MarkAffectedTransactionsDirty(const CTransaction& tx) { // If a transaction changes 'conflicted' state, that changes the balance @@ -4112,8 +4144,6 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile) LogPrintf("Wallet completed loading in %15dms\n", GetTimeMillis() - nStart); - RegisterValidationInterface(walletInstance); - CBlockIndex* pindexRescan = chainActive.Tip(); if (gArgs.GetBoolArg("-rescan", false)) pindexRescan = chainActive.Genesis(); @@ -4125,6 +4155,10 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile) else pindexRescan = chainActive.Genesis(); } + + walletInstance->m_last_block_processed = chainActive.Tip(); + RegisterValidationInterface(walletInstance); + if (chainActive.Tip() && chainActive.Tip() != pindexRescan) { uiInterface.InitMessage(_("Rescanning...")); LogPrintf("Rescanning last %i blocks (from block %i)...\n", chainActive.Height() - pindexRescan->nHeight, pindexRescan->nHeight); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 220a887baec0..5cd1ce610cc5 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -518,6 +518,18 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface std::unique_ptr dbw; + /** + * The following is used to keep track of how far behind the wallet is + * from the chain sync, and to allow clients to block on us being caught up. + * + * Note that this is *not* how far we've processed, we may need some rescan + * to have seen all transactions in the chain, but is only used to track + * live BlockConnected callbacks. + * + * Protected by cs_main (see BlockUntilSyncedToCurrentChain) + */ + const CBlockIndex* m_last_block_processed; + int64_t nNextResend; int64_t nLastResend; @@ -1023,6 +1035,14 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface */ void postInitProcess(CScheduler& scheduler); + /** + * Blocks until the wallet state is up-to-date to /at least/ the current + * chain at the time this function is entered + * Obviously holding cs_main/cs_wallet when going into this call may cause + * deadlock + */ + void BlockUntilSyncedToCurrentChain(); + /** * Address book entry changed. * @note called with lock cs_wallet held. From f6df6e413a504fbd2fa4b28469a3007d18dd8978 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 6 Feb 2021 12:56:32 -0300 Subject: [PATCH 05/41] Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs Adaptation from btc@5d67a7868db188f7e43ce9028f215034d057790d This prevents the wallet-RPCs-return-stale-info issue from being re-introduced when new-block callbacks no longer happen in the block-connection cs_main lock --- src/rpc/rawtransaction.cpp | 4 ++ src/wallet/rpcwallet.cpp | 117 +++++++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 1afdabe60772..c6268745caf1 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -518,6 +518,10 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) if (!pwalletMain) throw std::runtime_error("wallet not initialized"); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + RPCTypeCheck(request.params, {UniValue::VSTR}); CTxDestination changeAddress = CNoDestination(); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 2b5dd3355693..d60deca7796c 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -440,6 +440,10 @@ UniValue sethdseed(const JSONRPCRequest& request) throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, "Cannot set a new HD seed while still in Initial Block Download"); } + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwallet->cs_wallet); // Do not do anything to non-HD wallets @@ -1033,6 +1037,10 @@ UniValue sendtoaddress(const JSONRPCRequest& request) EnsureWalletIsUnlocked(); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + bool isStaking = false, isShielded = false; const std::string addrStr = request.params[0].get_str(); const CWDestination& destination = Standard::DecodeDestination(addrStr, isStaking, isShielded); @@ -1209,6 +1217,10 @@ UniValue delegatestake(const JSONRPCRequest& request) HelpExampleCli("delegatestake", "\"S1t2a3kab9c8c71VA78xxxy4MxZg6vgeS6\" 1000 \"DMJRSsuU9zfyrvxVaAEFQqK4MxZg34fk\"") + HelpExampleRpc("delegatestake", "\"S1t2a3kab9c8c71VA78xxxy4MxZg6vgeS6\", 1000, \"DMJRSsuU9zfyrvxVaAEFQqK4MxZg34fk\"")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); CTransactionRef wtx; @@ -1251,6 +1263,10 @@ UniValue rawdelegatestake(const JSONRPCRequest& request) HelpExampleCli("rawdelegatestake", "\"S1t2a3kab9c8c71VA78xxxy4MxZg6vgeS6\" 1000 \"DMJRSsuU9zfyrvxVaAEFQqK4MxZg34fk\"") + HelpExampleRpc("rawdelegatestake", "\"S1t2a3kab9c8c71VA78xxxy4MxZg6vgeS6\", 1000, \"DMJRSsuU9zfyrvxVaAEFQqK4MxZg34fk\"")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); CTransactionRef wtx; @@ -1375,6 +1391,11 @@ UniValue viewshieldtransaction(const JSONRPCRequest& request) } EnsureWalletIsUnlocked(); + + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); uint256 hash; @@ -1687,6 +1708,10 @@ UniValue shieldsendmany(const JSONRPCRequest& request) "\"DMJRSsuU9zfyrvxVaAEFQqK4MxZg6vgeS6\", [{\"address\": \"ps1ra969yfhvhp73rw5ak2xvtcm9fkuqsnmad7qln79mphhdrst3lwu9vvv03yuyqlh42p42st47qd\" ,\"amount\": 5.0}]") ); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + SaplingOperation operation = CreateShieldedTransaction(request); std::string txHash; auto res = operation.send(txHash); @@ -1731,6 +1756,10 @@ UniValue rawshieldsendmany(const JSONRPCRequest& request) "\"DMJRSsuU9zfyrvxVaAEFQqK4MxZg6vgeS6\", [{\"address\": \"ps1ra969yfhvhp73rw5ak2xvtcm9fkuqsnmad7qln79mphhdrst3lwu9vvv03yuyqlh42p42st47qd\" ,\"amount\": 5.0}]") ); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + CTransaction tx = CreateShieldedTransaction(request).getFinalTx(); return EncodeHexTx(tx); } @@ -1760,6 +1789,10 @@ UniValue listaddressgroupings(const JSONRPCRequest& request) "\nExamples:\n" + HelpExampleCli("listaddressgroupings", "") + HelpExampleRpc("listaddressgroupings", "")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); UniValue jsonGroupings(UniValue::VARR); @@ -1860,6 +1893,10 @@ UniValue getreceivedbyaddress(const JSONRPCRequest& request) "\nAs a json rpc call\n" + HelpExampleRpc("getreceivedbyaddress", "\"DMJRSsuU9zfyrvxVaAEFQqK4MxZg6vgeS6\", 6")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); // pivx address @@ -1916,6 +1953,10 @@ UniValue getreceivedbylabel(const JSONRPCRequest& request) "\nAs a json rpc call\n" + HelpExampleRpc("getreceivedbylabel", "\"tabby\", 6")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); // Minimum confirmations @@ -1971,6 +2012,10 @@ UniValue getbalance(const JSONRPCRequest& request) "\nAs a json rpc call\n" + HelpExampleRpc("getbalance", "6")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); const int paramsSize = request.params.size(); @@ -2002,6 +2047,10 @@ UniValue getcoldstakingbalance(const JSONRPCRequest& request) "\nAs a json rpc call\n" + HelpExampleRpc("getcoldstakingbalance", "\"*\"")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); return ValueFromAmount(pwalletMain->GetColdStakingBalance()); @@ -2024,6 +2073,10 @@ UniValue getdelegatedbalance(const JSONRPCRequest& request) "\nAs a json rpc call\n" + HelpExampleRpc("getdelegatedbalance", "\"*\"")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); return ValueFromAmount(pwalletMain->GetDelegatedBalance()); @@ -2036,6 +2089,10 @@ UniValue getunconfirmedbalance(const JSONRPCRequest& request) "getunconfirmedbalance\n" "Returns the server's total unconfirmed balance\n"); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); return ValueFromAmount(pwalletMain->GetUnconfirmedBalance()); @@ -2156,6 +2213,10 @@ UniValue sendmany(const JSONRPCRequest& request) EnsureWalletIsUnlocked(); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + // Read Params if (!request.params[0].isNull() && !request.params[0].get_str().empty()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Dummy value must be set to \"\""); @@ -2414,6 +2475,10 @@ UniValue listreceivedbyaddress(const JSONRPCRequest& request) HelpExampleRpc("listreceivedbyaddress", "6, true, true") + HelpExampleRpc("listreceivedbyaddress", "6, true, true, \"DMJRSsuU9zfyrvxVaAEFQqK4MxZg6vgeS6\"")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); return ListReceived(request.params, false); @@ -2448,6 +2513,10 @@ UniValue listreceivedbyshieldaddress(const JSONRPCRequest& request) + HelpExampleRpc("listreceivedbyshieldaddress", "\"ps1ra969yfhvhp73rw5ak2xvtcm9fkuqsnmad7qln79mphhdrst3lwu9vvv03yuyqlh42p42st47qd\"") ); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); int nMinDepth = 1; @@ -2542,6 +2611,10 @@ UniValue listreceivedbylabel(const JSONRPCRequest& request) "\nExamples:\n" + HelpExampleCli("listreceivedbylabel", "") + HelpExampleCli("listreceivedbylabel", "6 true") + HelpExampleRpc("listreceivedbylabel", "6, true, true")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); return ListReceived(request.params, true); @@ -2574,6 +2647,10 @@ UniValue listcoldutxos(const JSONRPCRequest& request) "\nExamples:\n" + HelpExampleCli("listcoldutxos", "") + HelpExampleCli("listcoldutxos", "true")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); bool fExcludeWhitelisted = false; @@ -2740,6 +2817,10 @@ UniValue listtransactions(const JSONRPCRequest& request) HelpExampleRpc("listtransactions", "\"*\", 20, 100") ); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); if (!request.params[0].isNull() && request.params[0].get_str() != "*") { @@ -2841,6 +2922,10 @@ UniValue listsinceblock(const JSONRPCRequest& request) HelpExampleCli("listsinceblock", "\"000000000000000bacf66f7497b7dc45ef753ee9a7d38571037cdb1a57f663ad\" 6") + HelpExampleRpc("listsinceblock", "\"000000000000000bacf66f7497b7dc45ef753ee9a7d38571037cdb1a57f663ad\", 6")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); CBlockIndex* pindex = NULL; @@ -2928,6 +3013,10 @@ UniValue gettransaction(const JSONRPCRequest& request) HelpExampleCli("gettransaction", "\"1075db55d416d3ca199f55b6084e2115b9345e16c5cf302fc80e9d5fbf5d48d\" true") + HelpExampleRpc("gettransaction", "\"1075db55d416d3ca199f55b6084e2115b9345e16c5cf302fc80e9d5fbf5d48d\"")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); uint256 hash; @@ -2984,6 +3073,10 @@ UniValue abandontransaction(const JSONRPCRequest& request) EnsureWalletIsUnlocked(); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); uint256 hash; @@ -3011,6 +3104,10 @@ UniValue backupwallet(const JSONRPCRequest& request) "\nExamples:\n" + HelpExampleCli("backupwallet", "\"backup.dat\"") + HelpExampleRpc("backupwallet", "\"backup.dat\"")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); std::string strDest = request.params[0].get_str(); @@ -3199,6 +3296,10 @@ UniValue walletlock(const JSONRPCRequest& request) "\nAs json rpc call\n" + HelpExampleRpc("walletlock", "")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); if (request.fHelp) @@ -3315,6 +3416,10 @@ UniValue listunspent(const JSONRPCRequest& request) RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VNUM, UniValue::VARR, UniValue::VNUM}); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + int nMinDepth = 1; if (request.params.size() > 0) nMinDepth = request.params[0].get_int(); @@ -3439,6 +3544,10 @@ UniValue lockunspent(const JSONRPCRequest& request) "\nAs a json rpc call\n" + HelpExampleRpc("lockunspent", "false, \"[{\\\"txid\\\":\\\"a08e6907dbbd3d809776dbfc5d82e371b764ed838b5655e72f463568df1aadf0\\\",\\\"vout\\\":1}]\"")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); if (request.params.size() == 1) @@ -3622,6 +3731,10 @@ UniValue getwalletinfo(const JSONRPCRequest& request) "\nExamples:\n" + HelpExampleCli("getwalletinfo", "") + HelpExampleRpc("getwalletinfo", "")); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); UniValue obj(UniValue::VOBJ); @@ -4052,6 +4165,10 @@ UniValue getsaplingnotescount(const JSONRPCRequest& request) + HelpExampleRpc("getsaplingnotescount", "0") ); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); int nMinDepth = !request.params.empty() ? request.params[0].get_int() : 1; From 31a8790f22ee7d2091b90bc311458f531f16c754 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 6 Feb 2021 21:15:29 -0300 Subject: [PATCH 06/41] Use callbacks to cache whether wallet transactions are in mempool This avoid calling out to mempool state during coin selection, balance calculation, etc. In the next commit we ensure all wallet callbacks from CValidationInterface happen in the same queue, serialized with each other. This helps to avoid re-introducing one of the issues described in #9584 [1] by further disconnecting wallet from current chain/mempool state. Thanks to @morcos for the suggestion to do this. --- src/wallet/wallet.cpp | 33 +++++++++++++++++++++++++++++---- src/wallet/wallet.h | 2 ++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 70838680f8fa..12ce5aed9322 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1251,6 +1251,19 @@ void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) { LOCK2(cs_main, cs_wallet); SyncTransaction(ptx, NULL, -1); + + auto it = mapWallet.find(ptx->GetHash()); + if (it != mapWallet.end()) { + it->second.fInMempool = true; + } +} + +void CWallet::TransactionRemovedFromMempool(const CTransactionRef &ptx) { + LOCK(cs_wallet); + auto it = mapWallet.find(ptx->GetHash()); + if (it != mapWallet.end()) { + it->second.fInMempool = false; + } } void CWallet::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) @@ -1266,9 +1279,11 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const for (const CTransactionRef& ptx : vtxConflicted) { SyncTransaction(ptx, nullptr, -1); + TransactionRemovedFromMempool(ptx); } for (size_t i = 0; i < pblock->vtx.size(); i++) { SyncTransaction(pblock->vtx[i], pindex, i); + TransactionRemovedFromMempool(pblock->vtx[i]); } // Sapling: notify about the connected block @@ -1882,8 +1897,7 @@ void CWallet::ReacceptWalletTransactions(bool fFirstLoad) bool CWalletTx::InMempool() const { - LOCK(mempool.cs); - return mempool.exists(GetHash()); + return fInMempool; } void CWalletTx::RelayWalletTransaction(CConnman* connman) @@ -3193,9 +3207,13 @@ CWallet::CommitResult CWallet::CommitTransaction(CTransactionRef tx, CReserveKey res.hashTx = wtxNew.GetHash(); + // Get the inserted-CWalletTx from mapWallet so that the + // fInMempool flag is cached properly + CWalletTx& wtx = mapWallet.at(wtxNew.GetHash()); + // Try ATMP. This must not fail. The transaction has already been signed and recorded. CValidationState state; - if (!wtxNew.AcceptToMemoryPool(state, true, true, false)) { + if (!wtx.AcceptToMemoryPool(state, true, true, false)) { res.state = state; // Abandon the transaction if (AbandonTransaction(res.hashTx)) { @@ -3211,7 +3229,7 @@ CWallet::CommitResult CWallet::CommitTransaction(CTransactionRef tx, CReserveKey res.status = CWallet::CommitStatus::OK; // Broadcast - wtxNew.RelayWalletTransaction(connman); + wtx.RelayWalletTransaction(connman); } return res; } @@ -4310,7 +4328,13 @@ bool CWalletTx::IsInMainChainImmature() const bool CWalletTx::AcceptToMemoryPool(CValidationState& state, bool fLimitFree, bool fRejectInsaneFee, bool ignoreFees) { + // We must set fInMempool here - while it will be re-set to true by the + // entered-mempool callback, if we did not there would be a race where a + // user could call sendmoney in a loop and hit spurious out of funds errors + // because we think that the transaction they just generated's change is + // unavailable as we're not yet aware its in mempool. bool fAccepted = ::AcceptToMemoryPool(mempool, state, tx, fLimitFree, nullptr, false, fRejectInsaneFee, ignoreFees); + fInMempool = fAccepted; if (!fAccepted) LogPrintf("%s : %s\n", __func__, state.GetRejectReason()); return fAccepted; @@ -4576,6 +4600,7 @@ void CWalletTx::Init(const CWallet* pwalletIn) nTimeSmart = 0; fFromMe = false; fChangeCached = false; + fInMempool = false; nChangeCached = 0; fStakeDelegationVoided = false; fShieldedChangeCached = false; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 5cd1ce610cc5..8febf68a530d 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -337,6 +337,7 @@ class CWalletTx mutable bool fStakeDelegationVoided; mutable bool fChangeCached; + mutable bool fInMempool; mutable CAmount nChangeCached; mutable bool fShieldedChangeCached; mutable CAmount nShieldedChangeCached; @@ -863,6 +864,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface bool ActivateSaplingWallet(bool memOnly = false); int ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate = false, bool fromStartup = false); + void TransactionRemovedFromMempool(const CTransactionRef &ptx) override; void ReacceptWalletTransactions(bool fFirstLoad = false); void ResendWalletTransactions(CConnman* connman) override; From c7ab4901f1406caeb17a660d41edef70dd5fd732 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 6 Feb 2021 21:29:37 -0300 Subject: [PATCH 07/41] Also call other wallet notify callbacks in scheduler thread This runs Block{Connected,Disconnected}, SetBestChain, Inventory, and TransactionAddedToMempool on the background scheduler thread. Of those, only BlockConnected is used outside of Wallet/ZMQ, and is used only for orphan transaction removal in net_processing, something which does not need to be synchronous with anything else. This partially reverts #9583, re-enabling some of the gains from #7946. This does not, however, re-enable the gains achieved by repeatedly releasing cs_main between each transaction processed. --- src/validation.cpp | 2 +- src/validationinterface.cpp | 24 ++++++++++++++++-------- src/validationinterface.h | 24 ++++++++++++++++++++++-- 3 files changed, 39 insertions(+), 11 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index bdb5e25bb0f1..d99efd4cf8b0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2285,7 +2285,7 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) { assert(trace.pblock && trace.pindex); - GetMainSignals().BlockConnected(trace.pblock, trace.pindex, *trace.conflictedTxs); + GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs); } break; diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index fe8854f62d0a..d83ca6710d6f 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -129,20 +129,28 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockInd m_internals->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); } -void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptxn) { - m_internals->TransactionAddedToMempool(ptxn); +void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) { + m_internals->m_schedulerClient.AddToProcessQueue([ptx, this] { + m_internals->TransactionAddedToMempool(ptx); + }); } -void CMainSignals::BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::vector &txnConflicted) { - m_internals->BlockConnected(block, pindex, txnConflicted); +void CMainSignals::BlockConnected(const std::shared_ptr &pblock, const CBlockIndex *pindex, const std::shared_ptr>& pvtxConflicted) { + m_internals->m_schedulerClient.AddToProcessQueue([pblock, pindex, pvtxConflicted, this] { + m_internals->BlockConnected(pblock, pindex, *pvtxConflicted); + }); } -void CMainSignals::BlockDisconnected(const std::shared_ptr &block, int nBlockHeight) { - m_internals->BlockDisconnected(block, nBlockHeight); +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::SetBestChain(const CBlockLocator& locator) { - m_internals->SetBestChain(locator); +void CMainSignals::SetBestChain(const CBlockLocator &locator) { + m_internals->m_schedulerClient.AddToProcessQueue([locator, this] { + m_internals->SetBestChain(locator); + }); } void CMainSignals::Broadcast(CConnman* connman) { diff --git a/src/validationinterface.h b/src/validationinterface.h index 9a6ce3453d30..d16f30e112db 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -50,6 +50,11 @@ class CValidationInterface { protected: /** Notifies listeners of updated block chain tip */ virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {} + /** + * Notifies listeners of a transaction having been added to mempool. + * + * Called on a background thread. + */ virtual void TransactionAddedToMempool(const CTransactionRef &ptxn) {} /** * Notifies listeners of a transaction leaving mempool. @@ -62,9 +67,24 @@ class CValidationInterface { * Called on a background thread. */ virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx) {} + /** + * Notifies listeners of a block being connected. + * Provides a vector of transactions evicted from the mempool as a result. + * + * Called on a background thread. + */ virtual void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::vector &txnConflicted) {} + /** + * Notifies listeners of a block being disconnected + * + * Called on a background thread. + */ virtual void BlockDisconnected(const std::shared_ptr &block, int nBlockHeight) {} - /** Notifies listeners of the new active block chain on-disk. */ + /** + * Notifies listeners of the new active block chain on-disk. + * + * Called on a background thread. + */ virtual void SetBestChain(const CBlockLocator &locator) {} /** Tells listeners to broadcast their data. */ virtual void ResendWalletTransactions(CConnman* connman) {} @@ -101,7 +121,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::vector &txnConflicted); + void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::shared_ptr> &); void BlockDisconnected(const std::shared_ptr &block, int nBlockHeight); void SetBestChain(const CBlockLocator &); void Broadcast(CConnman* connman); From 7d0599733ad9b309f50046901f962c54914c0031 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 6 Feb 2021 23:12:14 -0300 Subject: [PATCH 08/41] Fix wallet RPC race by waiting for callbacks in sendrawtransaction --- src/rpc/rawtransaction.cpp | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index c6268745caf1..2ccb6c3dddd4 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -9,6 +9,7 @@ #include "core_io.h" #include "init.h" #include "keystore.h" +#include "validationinterface.h" #include "net.h" #include "policy/policy.h" #include "primitives/transaction.h" @@ -25,6 +26,7 @@ #include "wallet/wallet.h" #endif +#include #include #include @@ -903,6 +905,8 @@ UniValue sendrawtransaction(const JSONRPCRequest& request) "\nSend the transaction (signed hex)\n" + HelpExampleCli("sendrawtransaction", "\"signedhex\"") + "\nAs a json rpc call\n" + HelpExampleRpc("sendrawtransaction", "\"signedhex\"")); + std::promise promise; + RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VBOOL}); // parse hex string from parameter @@ -915,7 +919,8 @@ UniValue sendrawtransaction(const JSONRPCRequest& request) if (request.params.size() > 1) fOverrideFees = request.params[1].get_bool(); - AssertLockNotHeld(cs_main); + { // cs_main scope + LOCK(cs_main); CCoinsViewCache& view = *pcoinsTip; bool fHaveChain = false; for (size_t o = 0; !fHaveChain && o < mtx.vout.size(); o++) { @@ -927,7 +932,6 @@ UniValue sendrawtransaction(const JSONRPCRequest& request) CValidationState state; bool fMissingInputs; { - LOCK(cs_main); if (!AcceptToMemoryPool(mempool, state, MakeTransactionRef(std::move(mtx)), true, &fMissingInputs, false, !fOverrideFees)) { if (state.IsInvalid()) { throw JSONRPCError(RPC_TRANSACTION_REJECTED, strprintf("%i: %s", state.GetRejectCode(), state.GetRejectReason())); @@ -937,11 +941,25 @@ UniValue sendrawtransaction(const JSONRPCRequest& request) } throw JSONRPCError(RPC_TRANSACTION_ERROR, state.GetRejectReason()); } + } else { + // If wallet is enabled, ensure that the wallet has been made aware + // of the new transaction prior to returning. This prevents a race + // where a user might call sendrawtransaction with a transaction + // to/from their wallet, immediately call some wallet RPC, and get + // a stale result because callbacks have not yet been processed. + CallFunctionInValidationInterfaceQueue([&promise] { + promise.set_value(); + }); } } } else if (fHaveChain) { throw JSONRPCError(RPC_TRANSACTION_ALREADY_IN_CHAIN, "transaction already in block chain"); } + + } // cs_main + + promise.get_future().wait(); + if(!g_connman) throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled"); @@ -950,6 +968,7 @@ UniValue sendrawtransaction(const JSONRPCRequest& request) { pnode->PushInventory(inv); }); + return hashTx.GetHex(); } From 5f521fd887c11f0a09a346d7b0aac995b87bff2e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 30 Sep 2017 23:49:59 -0400 Subject: [PATCH 09/41] Give ZMQ consistent order with UpdatedBlockTip on scheduler thread Note that UpdatedBlockTip is also used in net_processing to announce new blocks to peers. As this may need additional review, this change is included in its own commit. --- src/validationinterface.cpp | 4 +++- src/validationinterface.h | 6 +++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index d83ca6710d6f..2fbfaeba45c1 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -126,7 +126,9 @@ void CMainSignals::MempoolEntryRemoved(CTransactionRef ptx, MemPoolRemovalReason } void CMainSignals::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload) { - m_internals->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); + m_internals->m_schedulerClient.AddToProcessQueue([pindexNew, pindexFork, fInitialDownload, this] { + m_internals->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); + }); } void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) { diff --git a/src/validationinterface.h b/src/validationinterface.h index d16f30e112db..73406579d748 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -48,7 +48,11 @@ class CValidationInterface { public: virtual ~CValidationInterface() = default; protected: - /** Notifies listeners of updated block chain tip */ + /** + * Notifies listeners of updated block chain tip + * + * Called on a background thread. + */ virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {} /** * Notifies listeners of a transaction having been added to mempool. From 4d927b0113a242104968a4ab6791ad16150a0453 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 3 May 2017 15:57:56 -0400 Subject: [PATCH 10/41] Add a dev notes document describing the new wallet RPC blocking --- doc/developer-notes.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 30d93911bf97..16c2170cbc6d 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -907,3 +907,16 @@ A few guidelines for introducing and reviewing new RPC interfaces: - *Exception*: Using RPC method aliases may be appropriate in cases where a new RPC is replacing a deprecated RPC, to avoid both RPCs confusingly showing up in the command list. + +- Wallet RPCs call BlockUntilSyncedToCurrentChain to maintain consistency with + `getblockchaininfo`'s state immediately prior to the call's execution. Wallet + RPCs whose behavior does *not* depend on the current chainstate may omit this + call. + + - *Rationale*: In previous versions of Bitcoin Core, the wallet was always + in-sync with the chainstate (by virtue of them all being updated in the + same cs_main lock). In order to maintain the behavior that wallet RPCs + return results as of at least the highest best-known block an RPC + client may be aware of prior to entering a wallet RPC call, we must block + until the wallet is caught up to the chainstate as of the RPC call's entry. + This also makes the API much easier for RPC clients to reason about. From 1c9fe10e6353d05cc20f7d05c87181b42c036781 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 7 Feb 2021 13:02:00 -0300 Subject: [PATCH 11/41] RPC: listunspent remove redundant wallet check --- src/wallet/rpcwallet.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index d60deca7796c..fbf1c98b4a31 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -34,8 +34,6 @@ #include "zpiv/deterministicmint.h" #include -#include - int64_t nWalletUnlockTime; static RecursiveMutex cs_nWalletUnlockTime; @@ -3455,7 +3453,6 @@ UniValue listunspent(const JSONRPCRequest& request) UniValue results(UniValue::VARR); std::vector vecOutputs; - assert(pwalletMain != NULL); LOCK2(cs_main, pwalletMain->cs_wallet); CWallet::AvailableCoinsFilter coinFilter; coinFilter.fOnlyConfirmed = false; @@ -3464,7 +3461,7 @@ UniValue listunspent(const JSONRPCRequest& request) if (out.nDepth < nMinDepth || out.nDepth > nMaxDepth) continue; - if (destinations.size()) { + if (!destinations.empty()) { CTxDestination address; if (!ExtractDestination(out.tx->tx->vout[out.i].scriptPubKey, address)) continue; From 51dea23bfb4c6829d0badad1e04df5f0c41a0f42 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 8 Feb 2021 09:52:52 -0300 Subject: [PATCH 12/41] net_processing move-only: decouple tier two get data request into its own function. --- src/net_processing.cpp | 144 ++++++++++++++++++++++------------------- 1 file changed, 79 insertions(+), 65 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f4a7209223e0..ef86a81d3799 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -757,6 +757,79 @@ static void RelayAddress(const CAddress& addr, bool fReachable, CConnman& connma connman.ForEachNodeThen(std::move(sortfunc), std::move(pushfunc)); } +void static PushTierTwoGetDataRequest(const CInv& inv, + CNode* pfrom, + CConnman& connman, + CNetMsgMaker& msgMaker, + bool& pushed) +{ + if (!pushed && inv.type == MSG_SPORK) { + if (mapSporks.count(inv.hash)) { + CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss.reserve(1000); + ss << mapSporks[inv.hash]; + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::SPORK, ss)); + pushed = true; + } + } + if (!pushed && inv.type == MSG_MASTERNODE_WINNER) { + if (masternodePayments.mapMasternodePayeeVotes.count(inv.hash)) { + CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss.reserve(1000); + ss << masternodePayments.mapMasternodePayeeVotes[inv.hash]; + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::MNWINNER, ss)); + pushed = true; + } + } + if (!pushed && inv.type == MSG_BUDGET_VOTE) { + if (g_budgetman.HaveSeenProposalVote(inv.hash)) { + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::BUDGETVOTE, g_budgetman.GetProposalVoteSerialized(inv.hash))); + pushed = true; + } + } + + if (!pushed && inv.type == MSG_BUDGET_PROPOSAL) { + if (g_budgetman.HaveProposal(inv.hash)) { + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::BUDGETPROPOSAL, g_budgetman.GetProposalSerialized(inv.hash))); + pushed = true; + } + } + + if (!pushed && inv.type == MSG_BUDGET_FINALIZED_VOTE) { + if (g_budgetman.HaveSeenFinalizedBudgetVote(inv.hash)) { + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::FINALBUDGETVOTE, g_budgetman.GetFinalizedBudgetVoteSerialized(inv.hash))); + pushed = true; + } + } + + if (!pushed && inv.type == MSG_BUDGET_FINALIZED) { + if (g_budgetman.HaveFinalizedBudget(inv.hash)) { + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::FINALBUDGET, g_budgetman.GetFinalizedBudgetSerialized(inv.hash))); + pushed = true; + } + } + + if (!pushed && inv.type == MSG_MASTERNODE_ANNOUNCE) { + if (mnodeman.mapSeenMasternodeBroadcast.count(inv.hash)) { + CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss.reserve(1000); + ss << mnodeman.mapSeenMasternodeBroadcast[inv.hash]; + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::MNBROADCAST, ss)); + pushed = true; + } + } + + if (!pushed && inv.type == MSG_MASTERNODE_PING) { + if (mnodeman.mapSeenMasternodePing.count(inv.hash)) { + CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss.reserve(1000); + ss << mnodeman.mapSeenMasternodePing[inv.hash]; + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::MNPING, ss)); + pushed = true; + } + } +} + void static ProcessGetData(CNode* pfrom, CConnman& connman, std::atomic& interruptMsgProc) { AssertLockNotHeld(cs_main); @@ -854,71 +927,12 @@ void static ProcessGetData(CNode* pfrom, CConnman& connman, std::atomic& i } } - if (!pushed && inv.type == MSG_SPORK) { - if (mapSporks.count(inv.hash)) { - CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); - ss.reserve(1000); - ss << mapSporks[inv.hash]; - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::SPORK, ss)); - pushed = true; - } - } - if (!pushed && inv.type == MSG_MASTERNODE_WINNER) { - if (masternodePayments.mapMasternodePayeeVotes.count(inv.hash)) { - CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); - ss.reserve(1000); - ss << masternodePayments.mapMasternodePayeeVotes[inv.hash]; - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::MNWINNER, ss)); - pushed = true; - } - } - if (!pushed && inv.type == MSG_BUDGET_VOTE) { - if (g_budgetman.HaveSeenProposalVote(inv.hash)) { - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::BUDGETVOTE, g_budgetman.GetProposalVoteSerialized(inv.hash))); - pushed = true; - } - } - - if (!pushed && inv.type == MSG_BUDGET_PROPOSAL) { - if (g_budgetman.HaveProposal(inv.hash)) { - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::BUDGETPROPOSAL, g_budgetman.GetProposalSerialized(inv.hash))); - pushed = true; - } - } - - if (!pushed && inv.type == MSG_BUDGET_FINALIZED_VOTE) { - if (g_budgetman.HaveSeenFinalizedBudgetVote(inv.hash)) { - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::FINALBUDGETVOTE, g_budgetman.GetFinalizedBudgetVoteSerialized(inv.hash))); - pushed = true; - } - } - - if (!pushed && inv.type == MSG_BUDGET_FINALIZED) { - if (g_budgetman.HaveFinalizedBudget(inv.hash)) { - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::FINALBUDGET, g_budgetman.GetFinalizedBudgetSerialized(inv.hash))); - pushed = true; - } - } - - if (!pushed && inv.type == MSG_MASTERNODE_ANNOUNCE) { - if (mnodeman.mapSeenMasternodeBroadcast.count(inv.hash)) { - CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); - ss.reserve(1000); - ss << mnodeman.mapSeenMasternodeBroadcast[inv.hash]; - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::MNBROADCAST, ss)); - pushed = true; - } - } - - if (!pushed && inv.type == MSG_MASTERNODE_PING) { - if (mnodeman.mapSeenMasternodePing.count(inv.hash)) { - CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); - ss.reserve(1000); - ss << mnodeman.mapSeenMasternodePing[inv.hash]; - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::MNPING, ss)); - pushed = true; - } - } + // Now check if it's a tier two data request and push it. + PushTierTwoGetDataRequest(inv, + pfrom, + connman, + msgMaker, + pushed); if (!pushed) { vNotFound.push_back(inv); From 10efbe5da4d02aca7a0084ee06c02c59376bb5cf Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 8 Feb 2021 09:58:18 -0300 Subject: [PATCH 13/41] net_processing: making PushTierTwoGetDataRequest return a bool in case of pushing the message. --- src/net_processing.cpp | 52 ++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index ef86a81d3799..2a901ee74e6b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -757,77 +757,81 @@ static void RelayAddress(const CAddress& addr, bool fReachable, CConnman& connma connman.ForEachNodeThen(std::move(sortfunc), std::move(pushfunc)); } -void static PushTierTwoGetDataRequest(const CInv& inv, +bool static PushTierTwoGetDataRequest(const CInv& inv, CNode* pfrom, CConnman& connman, - CNetMsgMaker& msgMaker, - bool& pushed) + CNetMsgMaker& msgMaker) { - if (!pushed && inv.type == MSG_SPORK) { + if (inv.type == MSG_SPORK) { if (mapSporks.count(inv.hash)) { CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); ss.reserve(1000); ss << mapSporks[inv.hash]; connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::SPORK, ss)); - pushed = true; + return true; } } - if (!pushed && inv.type == MSG_MASTERNODE_WINNER) { + + if (inv.type == MSG_MASTERNODE_WINNER) { if (masternodePayments.mapMasternodePayeeVotes.count(inv.hash)) { CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); ss.reserve(1000); ss << masternodePayments.mapMasternodePayeeVotes[inv.hash]; connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::MNWINNER, ss)); - pushed = true; + return true; } } - if (!pushed && inv.type == MSG_BUDGET_VOTE) { + + if (inv.type == MSG_BUDGET_VOTE) { if (g_budgetman.HaveSeenProposalVote(inv.hash)) { connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::BUDGETVOTE, g_budgetman.GetProposalVoteSerialized(inv.hash))); - pushed = true; + return true; } } - if (!pushed && inv.type == MSG_BUDGET_PROPOSAL) { + if (inv.type == MSG_BUDGET_PROPOSAL) { if (g_budgetman.HaveProposal(inv.hash)) { connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::BUDGETPROPOSAL, g_budgetman.GetProposalSerialized(inv.hash))); - pushed = true; + return true; } } - if (!pushed && inv.type == MSG_BUDGET_FINALIZED_VOTE) { + if (inv.type == MSG_BUDGET_FINALIZED_VOTE) { if (g_budgetman.HaveSeenFinalizedBudgetVote(inv.hash)) { connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::FINALBUDGETVOTE, g_budgetman.GetFinalizedBudgetVoteSerialized(inv.hash))); - pushed = true; + return true; } } - if (!pushed && inv.type == MSG_BUDGET_FINALIZED) { + if (inv.type == MSG_BUDGET_FINALIZED) { if (g_budgetman.HaveFinalizedBudget(inv.hash)) { connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::FINALBUDGET, g_budgetman.GetFinalizedBudgetSerialized(inv.hash))); - pushed = true; + return true; } } - if (!pushed && inv.type == MSG_MASTERNODE_ANNOUNCE) { + if (inv.type == MSG_MASTERNODE_ANNOUNCE) { if (mnodeman.mapSeenMasternodeBroadcast.count(inv.hash)) { CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); ss.reserve(1000); ss << mnodeman.mapSeenMasternodeBroadcast[inv.hash]; connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::MNBROADCAST, ss)); - pushed = true; + return true; } } - if (!pushed && inv.type == MSG_MASTERNODE_PING) { + if (inv.type == MSG_MASTERNODE_PING) { if (mnodeman.mapSeenMasternodePing.count(inv.hash)) { CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); ss.reserve(1000); ss << mnodeman.mapSeenMasternodePing[inv.hash]; connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::MNPING, ss)); - pushed = true; + return true; } } + + // nothing was pushed. + return false; } void static ProcessGetData(CNode* pfrom, CConnman& connman, std::atomic& interruptMsgProc) @@ -927,12 +931,10 @@ void static ProcessGetData(CNode* pfrom, CConnman& connman, std::atomic& i } } - // Now check if it's a tier two data request and push it. - PushTierTwoGetDataRequest(inv, - pfrom, - connman, - msgMaker, - pushed); + if (!pushed) { + // Now check if it's a tier two data request and push it. + pushed = PushTierTwoGetDataRequest(inv, pfrom, connman, msgMaker); + } if (!pushed) { vNotFound.push_back(inv); From da7c0f728f6ca0a835600bd326198247427d40cd Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 8 Feb 2021 10:42:49 -0300 Subject: [PATCH 14/41] Refactor ProcessGetData avoiding to lock cs_main for its entire time. Base work inspiration btc@f69c4370d0a43f798af0c7b3c4c5e4b1929d92a3. --- src/net_processing.cpp | 211 ++++++++++++++++++++++------------------- 1 file changed, 116 insertions(+), 95 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 2a901ee74e6b..55edbcaeaa65 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -834,117 +834,138 @@ bool static PushTierTwoGetDataRequest(const CInv& inv, return false; } -void static ProcessGetData(CNode* pfrom, CConnman& connman, std::atomic& interruptMsgProc) +void static ProcessGetBlockData(CNode* pfrom, const CInv& inv, CConnman& connman, const std::atomic& interruptMsgProc) { - AssertLockNotHeld(cs_main); + LOCK(cs_main); + CNetMsgMaker msgMaker(pfrom->GetSendVersion()); + bool send = false; + BlockMap::iterator mi = mapBlockIndex.find(inv.hash); + if (mi != mapBlockIndex.end()) { + if (chainActive.Contains(mi->second)) { + send = true; + } else { + // To prevent fingerprinting attacks, only send blocks outside of the active + // chain if they are valid, and no more than a max reorg depth than the best header + // chain we know about. + send = mi->second->IsValid(BLOCK_VALID_SCRIPTS) && (pindexBestHeader != NULL) && + (chainActive.Height() - mi->second->nHeight < gArgs.GetArg("-maxreorg", DEFAULT_MAX_REORG_DEPTH)); + if (!send) { + LogPrint(BCLog::NET, "ProcessGetData(): ignoring request from peer=%i for old block that isn't in the main chain\n", pfrom->GetId()); + } + } + } + // Don't send not-validated blocks + if (send && (mi->second->nStatus & BLOCK_HAVE_DATA)) { + // Send block from disk + CBlock block; + if (!ReadBlockFromDisk(block, (*mi).second)) + assert(!"cannot load block from disk"); + if (inv.type == MSG_BLOCK) + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::BLOCK, block)); + else // MSG_FILTERED_BLOCK) + { + bool send_ = false; + CMerkleBlock merkleBlock; + { + LOCK(pfrom->cs_filter); + if (pfrom->pfilter) { + send_ = true; + merkleBlock = CMerkleBlock(block, *pfrom->pfilter); + } + } + if (send_) { + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::MERKLEBLOCK, merkleBlock)); + // CMerkleBlock just contains hashes, so also push any transactions in the block the client did not see + // This avoids hurting performance by pointlessly requiring a round-trip + // Note that there is currently no way for a node to request any single transactions we didnt send here - + // they must either disconnect and retry or request the full block. + // Thus, the protocol spec specified allows for us to provide duplicate txn here, + // however we MUST always provide at least what the remote peer needs + for (std::pair& pair : merkleBlock.vMatchedTxn) + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::TX, *block.vtx[pair.first])); + } + // else + // no response + } + + // Trigger them to send a getblocks request for the next batch of inventory + if (inv.hash == pfrom->hashContinue) { + // Bypass PushInventory, this must send even if redundant, + // and we want it right after the last block so they don't + // wait for other stuff first. + std::vector vInv; + vInv.emplace_back(MSG_BLOCK, chainActive.Tip()->GetBlockHash()); + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::INV, vInv)); + pfrom->hashContinue.SetNull(); + } + } +} + +// Only return true if the inv type can be answered, not supported types return false. +bool static IsTierTwoInventoryTypeKnown(int type) +{ + return type == MSG_SPORK || + type == MSG_MASTERNODE_WINNER || + type == MSG_BUDGET_VOTE || + type == MSG_BUDGET_PROPOSAL || + type == MSG_BUDGET_FINALIZED || + type == MSG_BUDGET_FINALIZED_VOTE || + type == MSG_MASTERNODE_ANNOUNCE || + type == MSG_MASTERNODE_PING; +} + +void static ProcessGetData(CNode* pfrom, CConnman& connman, const std::atomic& interruptMsgProc) +{ std::deque::iterator it = pfrom->vRecvGetData.begin(); std::vector vNotFound; CNetMsgMaker msgMaker(pfrom->GetSendVersion()); - LOCK(cs_main); - - while (it != pfrom->vRecvGetData.end()) { - // Don't bother if send buffer is too full to respond anyway - if (pfrom->fPauseSend) - break; + { + LOCK(cs_main); - const CInv& inv = *it; - { + while (it != pfrom->vRecvGetData.end() && (it->type == MSG_TX || IsTierTwoInventoryTypeKnown(it->type))) { if (interruptMsgProc) return; - it++; + // Don't bother if send buffer is too full to respond anyway + if (pfrom->fPauseSend) + break; - if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK) { - bool send = false; - BlockMap::iterator mi = mapBlockIndex.find(inv.hash); - if (mi != mapBlockIndex.end()) { - if (chainActive.Contains(mi->second)) { - send = true; - } else { - // To prevent fingerprinting attacks, only send blocks outside of the active - // chain if they are valid, and no more than a max reorg depth than the best header - // chain we know about. - send = mi->second->IsValid(BLOCK_VALID_SCRIPTS) && (pindexBestHeader != NULL) && - (chainActive.Height() - mi->second->nHeight < gArgs.GetArg("-maxreorg", DEFAULT_MAX_REORG_DEPTH)); - if (!send) { - LogPrint(BCLog::NET, "ProcessGetData(): ignoring request from peer=%i for old block that isn't in the main chain\n", pfrom->GetId()); - } - } - } - // Don't send not-validated blocks - if (send && (mi->second->nStatus & BLOCK_HAVE_DATA)) { - // Send block from disk - CBlock block; - if (!ReadBlockFromDisk(block, (*mi).second)) - assert(!"cannot load block from disk"); - if (inv.type == MSG_BLOCK) - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::BLOCK, block)); - else // MSG_FILTERED_BLOCK) - { - bool send = false; - CMerkleBlock merkleBlock; - { - LOCK(pfrom->cs_filter); - if (pfrom->pfilter) { - send = true; - merkleBlock = CMerkleBlock(block, *pfrom->pfilter); - } - } - if (send) { - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::MERKLEBLOCK, merkleBlock)); - // CMerkleBlock just contains hashes, so also push any transactions in the block the client did not see - // This avoids hurting performance by pointlessly requiring a round-trip - // Note that there is currently no way for a node to request any single transactions we didnt send here - - // they must either disconnect and retry or request the full block. - // Thus, the protocol spec specified allows for us to provide duplicate txn here, - // however we MUST always provide at least what the remote peer needs - for (std::pair& pair : merkleBlock.vMatchedTxn) - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::TX, *block.vtx[pair.first])); - } - // else - // no response - } + const CInv &inv = *it; + it++; - // Trigger them to send a getblocks request for the next batch of inventory - if (inv.hash == pfrom->hashContinue) { - // Bypass PushInventory, this must send even if redundant, - // and we want it right after the last block so they don't - // wait for other stuff first. - std::vector vInv; - vInv.emplace_back(MSG_BLOCK, chainActive.Tip()->GetBlockHash()); - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::INV, vInv)); - pfrom->hashContinue.SetNull(); - } - } - } else if (inv.IsKnownType()) { - // Send stream from relay memory - bool pushed = false; - - if (inv.type == MSG_TX) { - auto txinfo = mempool.info(inv.hash); - if (txinfo.tx) { // future: add timeLastMempoolReq check - CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); - ss.reserve(1000); - ss << *txinfo.tx; - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::TX, ss)); - pushed = true; - } + // Send stream from relay memory + bool pushed = false; + if (inv.type == MSG_TX) { + auto txinfo = mempool.info(inv.hash); + if (txinfo.tx) { // future: add timeLastMempoolReq check + CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss.reserve(1000); + ss << *txinfo.tx; + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::TX, ss)); + pushed = true; } + } - if (!pushed) { - // Now check if it's a tier two data request and push it. - pushed = PushTierTwoGetDataRequest(inv, pfrom, connman, msgMaker); - } + if (!pushed) { + // Now check if it's a tier two data request and push it. + pushed = PushTierTwoGetDataRequest(inv, pfrom, connman, msgMaker); + } - if (!pushed) { - vNotFound.push_back(inv); - } + if (!pushed) { + vNotFound.push_back(inv); } - if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK) - break; + // todo: inventory signal } - } + + if (it != pfrom->vRecvGetData.end()) { + const CInv &inv = *it; + it++; + if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK) { + ProcessGetBlockData(pfrom, inv, connman, interruptMsgProc); + } + } + } // release cs_main pfrom->vRecvGetData.erase(pfrom->vRecvGetData.begin(), it); From 31c7974126579c0e9d170aa462cf3d286581cf58 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 8 Feb 2021 11:03:19 -0300 Subject: [PATCH 15/41] Decouple block processing cs_main lock from the rest of inv get data requests --- src/net_processing.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 55edbcaeaa65..907cca1cab74 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -917,6 +917,8 @@ bool static IsTierTwoInventoryTypeKnown(int type) void static ProcessGetData(CNode* pfrom, CConnman& connman, const std::atomic& interruptMsgProc) { + AssertLockNotHeld(cs_main); + std::deque::iterator it = pfrom->vRecvGetData.begin(); std::vector vNotFound; CNetMsgMaker msgMaker(pfrom->GetSendVersion()); @@ -957,15 +959,15 @@ void static ProcessGetData(CNode* pfrom, CConnman& connman, const std::atomicvRecvGetData.end()) { - const CInv &inv = *it; - it++; - if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK) { - ProcessGetBlockData(pfrom, inv, connman, interruptMsgProc); - } + if (it != pfrom->vRecvGetData.end()) { + const CInv &inv = *it; + it++; + if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK) { + ProcessGetBlockData(pfrom, inv, connman, interruptMsgProc); } - } // release cs_main + } pfrom->vRecvGetData.erase(pfrom->vRecvGetData.begin(), it); From 0c68e2fba5008e7c3550ee37297ebb8ddb5710c5 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 4 Dec 2017 18:31:36 -0500 Subject: [PATCH 16/41] Add an interface to get the queue depth out of CValidationInterface --- src/scheduler.cpp | 5 +++++ src/scheduler.h | 2 ++ src/validationinterface.cpp | 5 +++++ src/validationinterface.h | 2 ++ 4 files changed, 14 insertions(+) diff --git a/src/scheduler.cpp b/src/scheduler.cpp index 0232ad2794ef..86c08510ce45 100644 --- a/src/scheduler.cpp +++ b/src/scheduler.cpp @@ -186,3 +186,8 @@ void SingleThreadedSchedulerClient::EmptyQueue() { should_continue = !m_callbacks_pending.empty(); } } + +size_t SingleThreadedSchedulerClient::CallbacksPending() { + LOCK(m_cs_callbacks_pending); + return m_callbacks_pending.size(); +} diff --git a/src/scheduler.h b/src/scheduler.h index 71ca40f4fb83..781bd52ea56f 100644 --- a/src/scheduler.h +++ b/src/scheduler.h @@ -109,6 +109,8 @@ class SingleThreadedSchedulerClient { // Processes all remaining queue members on the calling thread, blocking until queue is empty // Must be called after the CScheduler has no remaining processing threads! void EmptyQueue(); + + size_t CallbacksPending(); }; #endif diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 2fbfaeba45c1..6dc80d81f1f7 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -72,6 +72,11 @@ void CMainSignals::FlushBackgroundCallbacks() { } } +size_t CMainSignals::CallbacksPending() { + if (!m_internals) return 0; + return m_internals->m_schedulerClient.CallbacksPending(); +} + void CMainSignals::RegisterWithMempoolSignals(CTxMemPool& pool) { pool.NotifyEntryRemoved.connect(std::bind(&CMainSignals::MempoolEntryRemoved, this, std::placeholders::_1, std::placeholders::_2)); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 73406579d748..695738f38968 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -118,6 +118,8 @@ class CMainSignals { /** Call any remaining callbacks on the calling thread */ void FlushBackgroundCallbacks(); + size_t CallbacksPending(); + /** Register with mempool to call TransactionRemovedFromMempool callbacks */ void RegisterWithMempoolSignals(CTxMemPool& pool); /** Unregister with mempool */ From cc91d44287ef7dc5376b40b8fdb2a890925b7514 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 8 Feb 2021 11:08:04 -0300 Subject: [PATCH 17/41] Block ActivateBestChain to empty validationinterface queue --- src/validation.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index d99efd4cf8b0..4c8883073e51 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -53,6 +53,8 @@ #include "zpiv/zerocoin.h" #include "zpiv/zpivmodule.h" +#include + #include #include #include @@ -2258,6 +2260,17 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb do { boost::this_thread::interruption_point(); + if (GetMainSignals().CallbacksPending() > 10) { + // Block until the validation queue drains. This should largely + // never happen in normal operation, however may happen during + // reindex, causing memory blowup if we run too far ahead. + std::promise promise; + CallFunctionInValidationInterfaceQueue([&promise] { + promise.set_value(); + }); + promise.get_future().wait(); + } + const CBlockIndex *pindexFork; bool fInitialDownload; while (true) { From 0c4642c1dac0ea5b9f6a1bc7ce6473592d7c54c5 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 8 Feb 2021 11:10:43 -0300 Subject: [PATCH 18/41] Add helper to wait for validation interface queue to catch up --- src/validation.cpp | 6 +----- src/validationinterface.cpp | 12 ++++++++++++ src/validationinterface.h | 10 ++++++++++ src/wallet/wallet.cpp | 7 +------ 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 4c8883073e51..81c313a5d735 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2264,11 +2264,7 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb // Block until the validation queue drains. This should largely // never happen in normal operation, however may happen during // reindex, causing memory blowup if we run too far ahead. - std::promise promise; - CallFunctionInValidationInterfaceQueue([&promise] { - promise.set_value(); - }); - promise.get_future().wait(); + SyncWithValidationInterfaceQueue(); } const CBlockIndex *pindexFork; diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 6dc80d81f1f7..724e5dada54d 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -7,7 +7,9 @@ #include "validationinterface.h" #include "scheduler.h" #include "txmempool.h" +#include "validation.h" +#include #include #include #include @@ -122,6 +124,16 @@ void CallFunctionInValidationInterfaceQueue(std::function func) { g_signals.m_internals->m_schedulerClient.AddToProcessQueue(std::move(func)); } +void SyncWithValidationInterfaceQueue() { + AssertLockNotHeld(cs_main); + // Block until the validation queue drains + std::promise promise; + CallFunctionInValidationInterfaceQueue([&promise] { + promise.set_value(); + }); + promise.get_future().wait(); +} + void CMainSignals::MempoolEntryRemoved(CTransactionRef ptx, MemPoolRemovalReason reason) { if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) { m_internals->m_schedulerClient.AddToProcessQueue([ptx, this] { diff --git a/src/validationinterface.h b/src/validationinterface.h index 695738f38968..7377e13ae286 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -43,6 +43,16 @@ void UnregisterAllValidationInterfaces(); * will result in a deadlock (that DEBUG_LOCKORDER will miss). */ void CallFunctionInValidationInterfaceQueue(std::function func); +/** + * This is a synonym for the following, which asserts certain locks are not + * held: + * std::promise promise; + * CallFunctionInValidationInterfaceQueue([&promise] { + * promise.set_value(); + * }); + * promise.get_future().wait(); + */ +void SyncWithValidationInterfaceQueue(); class CValidationInterface { public: diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 12ce5aed9322..458d56ebc7ad 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1340,12 +1340,7 @@ void CWallet::BlockUntilSyncedToCurrentChain() { // ...otherwise put a callback in the validation interface queue and wait // for the queue to drain enough to execute it (indicating we are caught up // at least with the time we entered this function). - - std::promise promise; - CallFunctionInValidationInterfaceQueue([&promise] { - promise.set_value(); - }); - promise.get_future().wait(); + SyncWithValidationInterfaceQueue(); } void CWallet::MarkAffectedTransactionsDirty(const CTransaction& tx) From 596056cdfa4d34385613d9bfbda3467c874735dc Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 8 Feb 2021 11:17:42 -0300 Subject: [PATCH 19/41] [validation] Do not check for double spent zerocoins. they are networkely disabled and checkpoints are preventing the chain for any large reorganization. --- src/validation.cpp | 31 ++++--------------------------- 1 file changed, 4 insertions(+), 27 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 81c313a5d735..6bf8e7098644 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2771,33 +2771,10 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo return state.Invalid(false, state.GetRejectCode(), state.GetRejectReason(), strprintf("Transaction check failed (tx hash %s) %s", tx.GetHash().ToString(), state.GetDebugMessage())); - // double check that there are no double spent zPIV spends in this block - if (tx.HasZerocoinSpendInputs()) { - for (const CTxIn& txIn : tx.vin) { - bool isPublicSpend = txIn.IsZerocoinPublicSpend(); - if (txIn.IsZerocoinSpend() || isPublicSpend) { - libzerocoin::CoinSpend spend; - if (isPublicSpend) { - libzerocoin::ZerocoinParams* params = Params().GetConsensus().Zerocoin_Params(false); - PublicCoinSpend publicSpend(params); - if (!ZPIVModule::ParseZerocoinPublicSpend(txIn, tx, state, publicSpend)){ - return false; - } - spend = publicSpend; - // check that the version matches the one enforced with SPORK_18 (don't ban if it fails) - if (!IsInitialBlockDownload() && !CheckPublicCoinSpendVersion(spend.getVersion())) { - return state.DoS(0, error("%s : Public Zerocoin spend version %d not accepted. must be version %d.", - __func__, spend.getVersion(), CurrentPublicCoinSpendVersion()), REJECT_INVALID, "bad-zcspend-version"); - } - } else { - spend = TxInToZerocoinSpend(txIn); - } - if (std::count(vBlockSerials.begin(), vBlockSerials.end(), spend.getCoinSerialNumber())) - return state.DoS(100, error("%s : Double spending of zPIV serial %s in block\n Block: %s", - __func__, spend.getCoinSerialNumber().GetHex(), block.ToString())); - vBlockSerials.emplace_back(spend.getCoinSerialNumber()); - } - } + // No need to check for zerocoin anymore, they are networkely disabled + // and checkpoints are preventing the chain for any massive reorganization. + if (fSaplingActive && tx.ContainsZerocoins()) { + return state.DoS(100, error("%s : v5 upgrade enforced, zerocoin disabled", __func__)); } } From 67c754a73a82e7c7b4112ebb678320596fac4eb2 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 10 Feb 2021 18:06:04 -0300 Subject: [PATCH 20/41] qa: Sync with validationinterface queue in sync_mempools --- src/rpc/blockchain.cpp | 17 +++++++++++++++++ test/functional/test_framework/util.py | 6 +++--- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 704038da8ec9..632bd84322b9 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -21,6 +21,7 @@ #include "utilmoneystr.h" #include "utilstrencodings.h" #include "hash.h" +#include "validationinterface.h" #include "wallet/wallet.h" #include "zpiv/zpivmodule.h" #include "zpivchain.h" @@ -48,6 +49,21 @@ static CUpdatedBlock latestblock; extern void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry); void ScriptPubKeyToJSON(const CScript& scriptPubKey, UniValue& out, bool fIncludeHex); +UniValue syncwithvalidationinterfacequeue(const JSONRPCRequest& request) +{ + if (request.fHelp || request.params.size() > 0) { + throw std::runtime_error( + "syncwithvalidationinterfacequeue\n" + "\nWaits for the validation interface queue to catch up on everything that was there when we entered this function.\n" + "\nExamples:\n" + + HelpExampleCli("syncwithvalidationinterfacequeue","") + + HelpExampleRpc("syncwithvalidationinterfacequeue","") + ); + } + SyncWithValidationInterfaceQueue(); + return NullUniValue; +} + double GetDifficulty(const CBlockIndex* blockindex) { // Floating point number that is a multiple of the minimum difficulty, @@ -1448,6 +1464,7 @@ static const CRPCCommand commands[] = { "hidden", "waitfornewblock", &waitfornewblock, true }, { "hidden", "waitforblock", &waitforblock, true }, { "hidden", "waitforblockheight", &waitforblockheight, true }, + { "hidden", "syncwithvalidationinterfacequeue", &syncwithvalidationinterfacequeue, true }, }; diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index a5eb464dee2f..30495e47b296 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -420,9 +420,9 @@ def sync_mempools(rpc_connections, *, wait=1, timeout=60, flush_scheduler=True): while time.time() <= stop_time: pool = [set(r.getrawmempool()) for r in rpc_connections] if pool.count(pool[0]) == len(rpc_connections): - #if flush_scheduler: - # for r in rpc_connections: - # r.syncwithvalidationinterfacequeue() + if flush_scheduler: + for r in rpc_connections: + r.syncwithvalidationinterfacequeue() return # Check that each peer has at least one connection assert (all([len(x.getpeerinfo()) for x in rpc_connections])) From c3a281c9271163ed1b68da52c6c43af50c18d4e8 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 11 Feb 2021 11:20:39 -0300 Subject: [PATCH 21/41] fix mempool_persist.py dump issue, missing sync with validation interface. --- test/functional/mempool_persist.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/mempool_persist.py b/test/functional/mempool_persist.py index 52fa6d3e57d3..4a312f6f6557 100755 --- a/test/functional/mempool_persist.py +++ b/test/functional/mempool_persist.py @@ -69,7 +69,7 @@ def run_test(self): assert_equal(len(self.nodes[1].getrawmempool()), 0) # Verify accounting of mempool transactions after restart is correct - #self.nodes[2].syncwithvalidationinterfacequeue() # Flush mempool to wallet + self.nodes[2].syncwithvalidationinterfacequeue() # Flush mempool to wallet assert_equal(node2_balance, self.nodes[2].getbalance()) self.log.debug("Stop-start node0 with -persistmempool=0. Verify that it doesn't load its mempool.dat file.") From 0dfebf47a7075b104e9d91c3f90090db05781d80 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 18 Feb 2021 18:01:04 -0300 Subject: [PATCH 22/41] sapling_rpc_wallet_tests: remove unneeded cs_main and cs_wallet locks. --- src/test/librust/sapling_rpc_wallet_tests.cpp | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/test/librust/sapling_rpc_wallet_tests.cpp b/src/test/librust/sapling_rpc_wallet_tests.cpp index f6e8e0bca251..b08cc8b8735c 100644 --- a/src/test/librust/sapling_rpc_wallet_tests.cpp +++ b/src/test/librust/sapling_rpc_wallet_tests.cpp @@ -87,7 +87,11 @@ BOOST_AUTO_TEST_CASE(rpc_wallet_getbalance) { SelectParams(CBaseChainParams::TESTNET); - LOCK2(cs_main, pwalletMain->cs_wallet); + { + LOCK(pwalletMain->cs_wallet); + pwalletMain->SetMinVersion(FEATURE_SAPLING); + pwalletMain->SetupSPKM(false); + } BOOST_CHECK_THROW(CallRPC("getshieldbalance too many args"), std::runtime_error); BOOST_CHECK_THROW(CallRPC("getshieldbalance invalidaddress"), std::runtime_error); @@ -249,7 +253,11 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_parameters) { SelectParams(CBaseChainParams::TESTNET); - LOCK2(cs_main, pwalletMain->cs_wallet); + { + LOCK(pwalletMain->cs_wallet); + pwalletMain->SetMinVersion(FEATURE_SAPLING); + pwalletMain->SetupSPKM(false); + } BOOST_CHECK_THROW(CallRPC("shieldsendmany"), std::runtime_error); BOOST_CHECK_THROW(CallRPC("shieldsendmany toofewargs"), std::runtime_error); @@ -299,7 +307,6 @@ BOOST_AUTO_TEST_CASE(rpc_shieldsendmany_parameters) std::vector v (2 * (ZC_MEMO_SIZE+1)); // x2 for hexadecimal string format std::fill(v.begin(),v.end(), 'A'); std::string badmemo(v.begin(), v.end()); - pwalletMain->SetupSPKM(false); auto pa = pwalletMain->GenerateNewSaplingZKey(); std::string zaddr1 = KeyIO::EncodePaymentAddress(pa); BOOST_CHECK_THROW(CallRPC(std::string("shieldsendmany yBYhwgzufrZ6F5VVuK9nEChENArq934mqC ") @@ -543,8 +550,10 @@ BOOST_AUTO_TEST_CASE(rpc_listshieldunspent_parameters) { SelectParams(CBaseChainParams::TESTNET); - LOCK2(cs_main, pwalletMain->cs_wallet); - pwalletMain->SetupSPKM(false); + { + LOCK(pwalletMain->cs_wallet); + pwalletMain->SetupSPKM(false); + } UniValue retValue; From 296c95664e966eabe9b5efd4b3d7a084ff7e88e2 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 18 Feb 2021 20:29:13 -0300 Subject: [PATCH 23/41] wallet: guard null m_last_block_processed --- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 458d56ebc7ad..70312f895b26 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1323,7 +1323,7 @@ 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 diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8febf68a530d..c01e445e5ff0 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -529,7 +529,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface * * Protected by cs_main (see BlockUntilSyncedToCurrentChain) */ - const CBlockIndex* m_last_block_processed; + const CBlockIndex* m_last_block_processed{nullptr}; int64_t nNextResend; int64_t nLastResend; From b9249c50a89ca62d7e39d31839a219d0300208f4 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 18 Feb 2021 23:35:16 -0300 Subject: [PATCH 24/41] Miner: generate RPC, fix coinbase script key not marked as used --- src/miner.cpp | 6 +++--- src/miner.h | 2 +- src/rpc/mining.cpp | 13 +++++++++++-- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index bef8aba8715e..7e2e1499425c 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -39,10 +39,10 @@ double dHashesPerSec = 0.0; int64_t nHPSTimerStart = 0; -std::unique_ptr CreateNewBlockWithKey(CReserveKey& reservekey, CWallet* pwallet) +std::unique_ptr CreateNewBlockWithKey(CReserveKey* reservekey, CWallet* pwallet) { CPubKey pubkey; - if (!reservekey.GetReservedKey(pubkey)) + if (!reservekey->GetReservedKey(pubkey)) return nullptr; const int nHeightNext = chainActive.Tip()->nHeight + 1; @@ -169,7 +169,7 @@ void BitcoinMiner(CWallet* pwallet, bool fProofOfStake) std::unique_ptr pblocktemplate((fProofOfStake ? BlockAssembler(Params(), DEFAULT_PRINTPRIORITY).CreateNewBlock(CScript(), pwallet, true, &availableCoins) : - CreateNewBlockWithKey(*opReservekey, pwallet))); + CreateNewBlockWithKey(opReservekey.get_ptr(), pwallet))); if (!pblocktemplate) continue; std::shared_ptr pblock = std::make_shared(pblocktemplate->block); diff --git a/src/miner.h b/src/miner.h index 5755097ac189..c4feb04cfe09 100644 --- a/src/miner.h +++ b/src/miner.h @@ -27,7 +27,7 @@ static const bool DEFAULT_PRINTPRIORITY = false; /** Run the miner threads */ void GenerateBitcoins(bool fGenerate, CWallet* pwallet, int nThreads); /** Generate a new block, without valid proof-of-work */ - std::unique_ptr CreateNewBlockWithKey(CReserveKey& reservekey, CWallet* pwallet); + std::unique_ptr CreateNewBlockWithKey(CReserveKey* reservekey, CWallet* pwallet); void BitcoinMiner(CWallet* pwallet, bool fProofOfStake); void ThreadStakeMinter(); diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 9c8ad36bf38d..d1f2ce630ed8 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -61,14 +61,17 @@ UniValue generate(const JSONRPCRequest& request) const Consensus::Params& consensus = Params().GetConsensus(); bool fPoS = consensus.NetworkUpgradeActive(nHeight + 1, Consensus::UPGRADE_POS); + std::unique_ptr reservekey; if (fPoS) { // If we are in PoS, wallet must be unlocked. EnsureWalletIsUnlocked(); + } else { + // Coinbase key + reservekey = MakeUnique(pwalletMain); } UniValue blockHashes(UniValue::VARR); - CReserveKey reservekey(pwalletMain); unsigned int nExtraNonce = 0; while (nHeight < nHeightEnd && !ShutdownRequested()) { @@ -81,7 +84,7 @@ UniValue generate(const JSONRPCRequest& request) std::unique_ptr pblocktemplate((fPoS ? BlockAssembler(Params(), DEFAULT_PRINTPRIORITY).CreateNewBlock(CScript(), pwalletMain, true, &availableCoins) : - CreateNewBlockWithKey(reservekey, pwalletMain))); + CreateNewBlockWithKey(reservekey.get(), pwalletMain))); if (!pblocktemplate.get()) break; std::shared_ptr pblock = std::make_shared(pblocktemplate->block); @@ -114,6 +117,12 @@ UniValue generate(const JSONRPCRequest& request) if (nGenerated == 0 || (!fPoS && nGenerated < nGenerate)) throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new blocks"); + // mark key as used, only for PoW coinbases + if (reservekey) { + // Remove key from key pool + reservekey->KeepKey(); + } + return blockHashes; } #endif // ENABLE_WALLET From 4ed70248a6bdc73eb7b946067559711adbb7e78f Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 18 Feb 2021 23:41:16 -0300 Subject: [PATCH 25/41] fix invalid numbers in wallet_labels.py --- test/functional/wallet_labels.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/functional/wallet_labels.py b/test/functional/wallet_labels.py index 981de7f30aad..21488d9c3d49 100755 --- a/test/functional/wallet_labels.py +++ b/test/functional/wallet_labels.py @@ -30,17 +30,17 @@ def run_test(self): assert_equal(node.getbalance(), 500) # there should be 2 address groups - # each with 1 address with a balance of 50 Bitcoins + # each with 1 address with a balance of 250 PIVs address_groups = node.listaddressgroupings() - assert_equal(len(address_groups), 1) + assert_equal(len(address_groups), 2) # the addresses aren't linked now, but will be after we send to the # common address linked_addresses = set() - #for address_group in address_groups: - # assert_equal(len(address_group), 1) - # assert_equal(len(address_group[0]), 2) - # assert_equal(address_group[0][1], 250) - # linked_addresses.add(address_group[0][0]) + for address_group in address_groups: + assert_equal(len(address_group), 1) + assert_equal(len(address_group[0]), 2) + assert_equal(address_group[0][1], 250) + linked_addresses.add(address_group[0][0]) # send 50 from each address to a third address not in this wallet # There's some fee that will come back to us when the miner reward From e6770c8ce7faf5be3030e9130b966899ea601d5f Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 19 Feb 2021 17:13:34 -0300 Subject: [PATCH 26/41] fixing invalid wallet_dump.py, generated PoW blocks use a P2PKH coinbase script that now is properly marked as used inside the wallet. Reverting the commit that was hiding this issue ebdc552408e748a78fa4273ffd8091a0c3e57da1 . --- test/functional/wallet_dump.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/functional/wallet_dump.py b/test/functional/wallet_dump.py index 58969c2b9620..749f26ce4e64 100755 --- a/test/functional/wallet_dump.py +++ b/test/functional/wallet_dump.py @@ -103,7 +103,7 @@ def run_test (self): found_addr, found_addr_chg, found_addr_rsv, hd_master_addr_unenc = \ read_dump(dumpUnencrypted, addrs, None) assert_equal(found_addr, test_addr_count) # all keys must be in the dump - assert_equal(found_addr_chg, 0) # 0 blocks where mined + assert_equal(found_addr_chg, 50) # 50 blocks where mined assert_equal(found_addr_rsv, 90 * 3) # 90 keys external plus 100% internal keys plus 100% staking keys #encrypt wallet, restart, unlock and dump @@ -118,7 +118,7 @@ def run_test (self): found_addr, found_addr_chg, found_addr_rsv, hd_master_addr_enc = \ read_dump(dumpEncrypted, addrs, hd_master_addr_unenc) assert_equal(found_addr, test_addr_count) - assert_equal(found_addr_chg, 90 * 3 + 1) # old reserve keys are marked as change now. todo: The +1 needs to be removed once this is updated (master seed taken as an internal key) + assert_equal(found_addr_chg, 90 * 3 + 1 + 50) # old reserve keys are marked as change now. todo: The +1 needs to be removed once this is updated (master seed taken as an internal key) assert_equal(found_addr_rsv, 90 * 3) # 90 external + 90 internal + 90 staking # Overwriting should fail From de3c7ae8159ef23945477e95d506ea9f11bbe23c Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 19 Feb 2021 17:51:42 -0300 Subject: [PATCH 27/41] fix wallet_upgrade.py test, wasn't counting the coinbase script. --- test/functional/wallet_upgrade.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/functional/wallet_upgrade.py b/test/functional/wallet_upgrade.py index ea995034b105..4c07713ef303 100755 --- a/test/functional/wallet_upgrade.py +++ b/test/functional/wallet_upgrade.py @@ -76,7 +76,7 @@ def set_test_params(self): self.setup_clean_chain = True self.num_nodes = 1 - def check_keys(self, addrs, mined_blocks = 0): + def check_keys(self, addrs): self.log.info("Checking old keys existence in the upgraded wallet..") # Now check that all of the pre upgrade addresses are still in the wallet for addr in addrs: @@ -87,7 +87,7 @@ def check_keys(self, addrs, mined_blocks = 0): self.log.info("All pre-upgrade keys found in the wallet :)") # Use all of the keys in the pre-HD keypool - for _ in range(0, 60 + mined_blocks): + for _ in range(0, 60): self.nodes[0].getnewaddress() self.log.info("All pre-upgrade keys should have been marked as used by now, creating new HD keys") @@ -135,7 +135,7 @@ def run_test(self): copyPreHDWallet(self.options.tmpdir, False) self.start_node(0) - # Generating a block to not be in IBD + # Generating a block to not be in IBD, here we create a new key for the coinbase script self.nodes[0].generate(1) self.log.info("Upgrading wallet..") @@ -143,7 +143,7 @@ def run_test(self): self.log.info("upgrade completed, checking keys now..") # Now check if the upgrade went fine - self.check_keys(addrs, 1) + self.check_keys(addrs) self.log.info("Upgrade via RPC completed, all good :)") From 815667d5c777b58a048c67035dcddd445f841f3e Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 19 Feb 2021 20:31:07 -0300 Subject: [PATCH 28/41] unit test framework: missing scheduler service loop start added. --- src/test/test_pivx.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/test/test_pivx.cpp b/src/test/test_pivx.cpp index 8537114b1285..d1aa0e6baca5 100644 --- a/src/test/test_pivx.cpp +++ b/src/test/test_pivx.cpp @@ -54,10 +54,15 @@ TestingSetup::TestingSetup() fs::create_directories(pathTemp); gArgs.ForceSetArg("-datadir", pathTemp.string()); + // Start the lightweight task scheduler thread + CScheduler::Function serviceLoop = std::bind(&CScheduler::serviceQueue, &scheduler); + threadGroup.create_thread(std::bind(&TraceThread, "scheduler", serviceLoop)); + // Note that because we don't bother running a scheduler thread here, // callbacks via CValidationInterface are unreliable, but that's OK, // our unit tests aren't testing multiple parts of the code at once. GetMainSignals().RegisterBackgroundSignalScheduler(scheduler); + GetMainSignals().RegisterWithMempoolSignals(mempool); // Ideally we'd move all the RPC tests to the functional testing framework // instead of unit tests, but for now we need these here. @@ -87,7 +92,9 @@ TestingSetup::~TestingSetup() threadGroup.interrupt_all(); threadGroup.join_all(); GetMainSignals().FlushBackgroundCallbacks(); + UnregisterAllValidationInterfaces(); GetMainSignals().UnregisterBackgroundSignalScheduler(); + GetMainSignals().UnregisterWithMempoolSignals(mempool); UnloadBlockIndex(); delete pcoinsTip; delete pcoinsdbview; From 1ed753f3273856e770295619c5dac65684120ce6 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 19 Feb 2021 23:13:14 -0300 Subject: [PATCH 29/41] Fix wallet_tests.cpp, missing fInMempool flag set. --- src/wallet/test/wallet_tests.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 0e26f9002167..e1822f6a356f 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -11,7 +11,6 @@ #include "wallet/wallet.h" #include -#include #include #include @@ -334,6 +333,7 @@ CBlockIndex* SimpleFakeMine(CWalletTx& wtx, CBlockIndex* pprev = nullptr) BOOST_CHECK(chainActive.Contains(fakeIndex)); wtx.SetMerkleBranch(fakeIndex->GetBlockHash(), 0); removeTxFromMempool(wtx); + wtx.fInMempool = false; return fakeIndex; } @@ -436,6 +436,7 @@ BOOST_AUTO_TEST_CASE(cached_balances_tests) // GetUnconfirmedBalance requires tx in mempool. fakeMempoolInsertion(wtxCredit.tx); + wtxCredit.fInMempool = true; BOOST_CHECK_EQUAL(wallet.GetUnconfirmedBalance(), nCredit); // 2) Confirm tx and verify From d97ace94cd9a0b79f57a8776b07685283cb0de80 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 20 Feb 2021 09:49:12 -0300 Subject: [PATCH 30/41] [Test] notes_double_spend: sync wallet before check balances. --- .../test/wallet_sapling_transactions_validations_tests.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp b/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp index 439bea930f96..1503a721061e 100644 --- a/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp +++ b/src/wallet/test/wallet_sapling_transactions_validations_tests.cpp @@ -33,6 +33,9 @@ void generateBlock(const CScript& scriptPubKey, int expectedBlockHeight) CValidationState state; BOOST_CHECK_MESSAGE(ProcessNewBlock(state, nullptr, pblock, nullptr), strprintf("Failed creating block at height %d", expectedBlockHeight)); BOOST_CHECK(state.IsValid()); + + // Let the wallet sync the blocks + SyncWithValidationInterfaceQueue(); } SaplingOperation createOperationAndBuildTx(std::vector recipients, From 1423dba79a62ab6293f2b26f820f8c4358d66dad Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Thu, 19 Jan 2017 23:45:02 -0500 Subject: [PATCH 31/41] [bugfix] save feeDelta instead of priorityDelta in DumpMempool --- src/validation.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index 6bf8e7098644..1e47489528bd 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4156,7 +4156,7 @@ void DumpMempool(void) { LOCK(mempool.cs); for (const auto &i : mempool.mapDeltas) { - mapDeltas[i.first] = i.second.first; + mapDeltas[i.first] = i.second.second; } vinfo = mempool.infoAll(); } From 756d0fabd83fd29a7699fc5702d32950379dc1ce Mon Sep 17 00:00:00 2001 From: practicalswift Date: Fri, 27 Nov 2020 12:41:07 +0000 Subject: [PATCH 32/41] Handle rename failure in DumpMempool(...) by using RenameOver(...) return value --- src/validation.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index 1e47489528bd..3b4764058ec5 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4185,7 +4185,9 @@ void DumpMempool(void) file << mapDeltas; FileCommit(file.Get()); file.fclose(); - RenameOver(GetDataDir() / "mempool.dat.new", GetDataDir() / "mempool.dat"); + if (!RenameOver(GetDataDir() / "mempool.dat.new", GetDataDir() / "mempool.dat")) { + throw std::runtime_error("Rename failed"); + } int64_t last = GetTimeMicros(); LogPrintf("Dumped mempool: %gs to copy, %gs to dump\n", (mid-start)*0.000001, (last-mid)*0.000001); } catch (const std::exception& e) { From 65cf7e1547dd6943003a3ac32d75e63c1b8e1ac4 Mon Sep 17 00:00:00 2001 From: instagibbs Date: Sat, 20 Feb 2021 11:09:16 -0300 Subject: [PATCH 33/41] don't attempt mempool entry for wallet transactions on startup if already in mempool --- src/wallet/wallet.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 70312f895b26..417011cbb355 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4323,6 +4323,11 @@ bool CWalletTx::IsInMainChainImmature() const bool CWalletTx::AcceptToMemoryPool(CValidationState& state, bool fLimitFree, bool fRejectInsaneFee, bool ignoreFees) { + // Quick check to avoid re-setting fInMempool to false + if (mempool.exists(tx->GetHash())) { + return false; + } + // We must set fInMempool here - while it will be re-set to true by the // entered-mempool callback, if we did not there would be a race where a // user could call sendmoney in a loop and hit spurious out of funds errors From 3ace13b3210211b62939d90f00318f53c255c18c Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 20 Feb 2021 11:15:50 -0300 Subject: [PATCH 34/41] qa: Fix some tests to work on native windows Coming from btc@fa3528a85b05ea9507077f3eb340c9fb189251a6 --- test/functional/mempool_persist.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/functional/mempool_persist.py b/test/functional/mempool_persist.py index 4a312f6f6557..4f5eeded70ae 100755 --- a/test/functional/mempool_persist.py +++ b/test/functional/mempool_persist.py @@ -59,7 +59,9 @@ def run_test(self): self.log.debug("Stop-start node0 and node1. Verify that node0 has the transactions in its mempool and node1 does not.") self.stop_nodes() - self.start_node(1) # Give this one a head-start, so we can be "extra-sure" that it didn't load anything later + # Give this node a head-start, so we can be "extra-sure" that it didn't load anything later + # Also don't store the mempool, to keep the datadir clean + self.start_node(1, extra_args=["-persistmempool=0"]) self.start_node(0) self.start_node(2) # Give pivxd a second to reload the mempool From f8cd3717b1de11b0d709473b1f9df37c1a04e3d1 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 21 Feb 2021 13:02:41 -0300 Subject: [PATCH 35/41] [Miner] Sync wallet state before try to solve proof of stake. --- src/blockassembler.cpp | 4 ++++ src/validationinterface.cpp | 3 +++ 2 files changed, 7 insertions(+) diff --git a/src/blockassembler.cpp b/src/blockassembler.cpp index 4de1bc80462a..dd0d19db5cfa 100644 --- a/src/blockassembler.cpp +++ b/src/blockassembler.cpp @@ -70,6 +70,10 @@ bool SolveProofOfStake(CBlock* pblock, CBlockIndex* pindexPrev, CWallet* pwallet { boost::this_thread::interruption_point(); pblock->nBits = GetNextWorkRequired(pindexPrev, pblock); + + // Sync wallet before create coinstake + pwallet->BlockUntilSyncedToCurrentChain(); + CMutableTransaction txCoinStake; int64_t nTxNewTime = 0; if (!pwallet->CreateCoinStake(*pwallet, pindexPrev, pblock->nBits, txCoinStake, nTxNewTime, availableCoins)) { diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 724e5dada54d..ad4af45dd7b1 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -126,6 +126,9 @@ void CallFunctionInValidationInterfaceQueue(std::function func) { void SyncWithValidationInterfaceQueue() { AssertLockNotHeld(cs_main); + // if queue is empty, do not wait for nothing.s + if (g_signals.CallbacksPending() == 0) return; + // Block until the validation queue drains std::promise promise; CallFunctionInValidationInterfaceQueue([&promise] { From 53497f0aa67b1c8dcebfd854433cd7d0b8a29624 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 22 Feb 2021 15:29:00 -0300 Subject: [PATCH 36/41] Validation: DisconnectTip doesn't need to force a flush to disk. This is highly slowing down the invalidate block/chain process for no reason. --- src/validation.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index 3b4764058ec5..82a8208dc541 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1899,7 +1899,7 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara LogPrint(BCLog::BENCH, "- Disconnect block: %.2fms\n", (GetTimeMicros() - nStart) * 0.001); const uint256& saplingAnchorAfterDisconnect = pcoinsTip->GetBestAnchor(); // Write the chain state to disk, if necessary. - if (!FlushStateToDisk(state, FLUSH_STATE_ALWAYS)) + if (!FlushStateToDisk(state, FLUSH_STATE_IF_NEEDED)) return false; // Resurrect mempool transactions from the disconnected block. std::vector vHashUpdate; From bfd9a15c2c84117ab70cb2741f96106ee9644e72 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 22 Feb 2021 15:29:48 -0300 Subject: [PATCH 37/41] test: sapling_fillblock.py sync mempool every 200 transactions instead of only at the end. --- test/functional/sapling_fillblock.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/functional/sapling_fillblock.py b/test/functional/sapling_fillblock.py index 9dc69af05eaa..82eb0710bac5 100755 --- a/test/functional/sapling_fillblock.py +++ b/test/functional/sapling_fillblock.py @@ -77,6 +77,7 @@ def send_shielded(self, node, n_txes, from_address, shield_to): txids.append(node.shieldsendmany(from_address, shield_to)) if (i + 1) % 200 == 0: self.log.info("...%d Transactions created..." % (i + 1)) + sync_mempools(self.nodes) return txids From 00cc6ec8c7a17bc93811f47fb61f23e2e2e61ff7 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 23 Feb 2021 09:59:29 -0300 Subject: [PATCH 38/41] dumpwallet: Add missing BlockUntilSyncedToCurrentChain --- src/wallet/rpcdump.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index f5dced6e313f..63ece49de707 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -468,6 +468,10 @@ UniValue dumpwallet(const JSONRPCRequest& request) throw JSONRPCError(RPC_MISC_ERROR, "Scam attempt detected!"); } + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + pwalletMain->BlockUntilSyncedToCurrentChain(); + LOCK2(cs_main, pwalletMain->cs_wallet); EnsureWalletIsUnlocked(); From ded2e8ef00f76271cf13dd8860f83883a2027a86 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 23 Feb 2021 10:15:33 -0300 Subject: [PATCH 39/41] feature_dbcrash.py using blockmaxsize instead of blockmaxweight that we currently don't support. --- test/functional/feature_dbcrash.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/feature_dbcrash.py b/test/functional/feature_dbcrash.py index 5067beadf8da..60382bd1ff78 100755 --- a/test/functional/feature_dbcrash.py +++ b/test/functional/feature_dbcrash.py @@ -67,7 +67,7 @@ def set_test_params(self): self.node2_args = ["-dbcrashratio=24", "-dbcache=16", "-dbbatchsize=200000"] + self.base_args # Node3 is a normal node with default args, except will mine full blocks - self.node3_args = ["-blockmaxweight=4000000"] + self.chain_params # future: back port blockmaxweight + self.node3_args = ["-blockmaxsize=1999000"] + self.chain_params # future: back port blockmaxweight self.extra_args = [self.node0_args, self.node1_args, self.node2_args, self.node3_args] def setup_network(self): From 046386b79a74e7da4c77db9a94ed0457863c4b70 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 23 Feb 2021 11:03:31 -0300 Subject: [PATCH 40/41] IsNoteSaplingChange: Add missing cs_wallet lock. GetNullifiersForAddresses() asserts for cs_wallet lock held. --- src/sapling/saplingscriptpubkeyman.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sapling/saplingscriptpubkeyman.cpp b/src/sapling/saplingscriptpubkeyman.cpp index 3d1955b93ecf..decc1753e4d5 100644 --- a/src/sapling/saplingscriptpubkeyman.cpp +++ b/src/sapling/saplingscriptpubkeyman.cpp @@ -760,7 +760,7 @@ CAmount SaplingScriptPubKeyMan::GetShieldedChange(const CWalletTx& wtx) const bool SaplingScriptPubKeyMan::IsNoteSaplingChange(const SaplingOutPoint& op, libzcash::SaplingPaymentAddress address) const { - LOCK(wallet->cs_KeyStore); + LOCK2(wallet->cs_wallet, wallet->cs_KeyStore); std::set shieldedAddresses = {address}; std::set> nullifierSet = GetNullifiersForAddresses(shieldedAddresses); return IsNoteSaplingChange(nullifierSet, address, op); From 3d11027c9798c278e42a48687938dff4c1e2d197 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 23 Feb 2021 14:17:01 -0300 Subject: [PATCH 41/41] wallet:CreateCoinStake, solving IsSpent() missing cs_main lock. for now, IsSpent() requires cs_main lock due its internal call to GetDepthInMainChain. This dependency will be completely removed moving forward, in #2209 --- src/wallet/wallet.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 417011cbb355..6489db9a6551 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3039,7 +3039,9 @@ bool CWallet::CreateCoinStake( if (IsLocked() || ShutdownRequested()) return false; // Make sure the stake input hasn't been spent since last check - if (IsSpent(outPoint)) { + // 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))) { // remove it from the available coins it = availableCoins->erase(it); continue;