From 0cf4064c574a2e1321d2c64f090eb26089836173 Mon Sep 17 00:00:00 2001 From: Volodia Date: Tue, 9 Mar 2021 17:45:34 +0200 Subject: [PATCH 01/31] Make box of PIVX address return to purple when it's empty Github-Pull: #2249 Rebased-From: a6a4af11554dc04cfda17c9cf35d74d02691d46f --- src/qt/pivx/sendmultirow.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qt/pivx/sendmultirow.cpp b/src/qt/pivx/sendmultirow.cpp index a4ed7d309356c..431bcc6ed88b3 100644 --- a/src/qt/pivx/sendmultirow.cpp +++ b/src/qt/pivx/sendmultirow.cpp @@ -149,6 +149,10 @@ bool SendMultiRow::addressChanged(const QString& str, bool fOnlyValidate) updateStyle(ui->lineEditAddress); return valid; } + + setCssProperty(ui->lineEditAddress, "edit-primary-multi-book"); + updateStyle(ui->lineEditAddress); + return false; } From 775e5322d371ea115dac606686c7216da38f13e2 Mon Sep 17 00:00:00 2001 From: Volodia Date: Thu, 25 Feb 2021 18:24:07 +0200 Subject: [PATCH 02/31] Fixes double fade-in animation when clicking the question mark next to the 'Available' label in the top bar Github-Pull: #2247 Rebased-From: e21a56048ab574b07e79683d3004d913fe70dadd --- src/qt/pivx/balancebubble.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/qt/pivx/balancebubble.cpp b/src/qt/pivx/balancebubble.cpp index 631d21d94acc4..7ecbacc8a5594 100644 --- a/src/qt/pivx/balancebubble.cpp +++ b/src/qt/pivx/balancebubble.cpp @@ -44,12 +44,12 @@ void BalanceBubble::showEvent(QShowEvent *event) { QGraphicsOpacityEffect *eff = new QGraphicsOpacityEffect(this); this->setGraphicsEffect(eff); - QPropertyAnimation *a = new QPropertyAnimation(eff,"opacity"); - a->setDuration(400); - a->setStartValue(0.1); - a->setEndValue(1); - a->setEasingCurve(QEasingCurve::InBack); - a->start(QPropertyAnimation::DeleteWhenStopped); + QPropertyAnimation *anim = new QPropertyAnimation(eff,"opacity"); + anim->setDuration(400); + anim->setStartValue(0); + anim->setEndValue(1); + anim->setEasingCurve(QEasingCurve::Linear); + anim->start(QPropertyAnimation::DeleteWhenStopped); if (!hideTimer) hideTimer = new QTimer(this); connect(hideTimer, &QTimer::timeout, this, &BalanceBubble::hideTimeout); @@ -77,4 +77,4 @@ void BalanceBubble::hideTimeout() BalanceBubble::~BalanceBubble() { delete ui; -} \ No newline at end of file +} From e10c1b68efd16dda2453fdfd356812a1ddb8318b Mon Sep 17 00:00:00 2001 From: dnchk <> Date: Mon, 8 Mar 2021 17:44:00 +0200 Subject: [PATCH 03/31] Set fee to highest possible if input is too big #fixes 2234 Set custom transaction fee to highest possible value in case input is too big number. Github-Pull: #2237 Rebased-From: ae1837eba55ecf17aaafd4f5a9b34e75f0a79c6f --- src/qt/pivx/sendcustomfeedialog.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qt/pivx/sendcustomfeedialog.cpp b/src/qt/pivx/sendcustomfeedialog.cpp index 21179611b9dd1..f70c896fcb4c8 100644 --- a/src/qt/pivx/sendcustomfeedialog.cpp +++ b/src/qt/pivx/sendcustomfeedialog.cpp @@ -123,6 +123,7 @@ void SendCustomFeeDialog::accept() // Check insane fee const CAmount insaneFee = ::minRelayTxFee.GetFeePerK() * 10000; if (customFee >= insaneFee) { + ui->lineEditCustomFee->setText(BitcoinUnits::format(walletModel->getOptionsModel()->getDisplayUnit(), insaneFee - CWallet::GetRequiredFee(1000))); inform(tr("Fee too high. Must be below: %1").arg( BitcoinUnits::formatWithUnit(walletModel->getOptionsModel()->getDisplayUnit(), insaneFee))); } else if (customFee < CWallet::GetRequiredFee(1000)) { From 584bbbad1dd7e65493f39446331fbd03f6b08fbf Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Fri, 1 Feb 2019 14:09:36 -0800 Subject: [PATCH 04/31] rpc: Expose g_is_mempool_loaded via getmempoolinfo and /rest/mempool/info.json And use it to fix a race condition in mempool_persist.py: https://travis-ci.org/Empact/bitcoin/jobs/487577243 Since e.g. getrawmempool returns errors based on this status, this enables users to test it for readiness. Github-Pull: #2254 Rebased-From: 0169a75b48b57c39b9e188eb8162e5b689ff9492 --- doc/REST-interface.md | 1 + src/rpc/blockchain.cpp | 9 ++++ test/functional/mempool_persist.py | 67 +++++++++++++++++------------- 3 files changed, 48 insertions(+), 29 deletions(-) diff --git a/doc/REST-interface.md b/doc/REST-interface.md index f4fb8ee57cfe1..a9eed451e637f 100644 --- a/doc/REST-interface.md +++ b/doc/REST-interface.md @@ -79,6 +79,7 @@ $ curl localhost:18332/rest/getutxos/checkmempool/b2cdfd7b89def827ff8af7cd9bff76 Returns various information about the TX mempool. Only supports JSON as output format. +* loaded : (boolean) if the mempool is fully loaded * size : (numeric) the number of transactions in the TX mempool * bytes : (numeric) size of the TX mempool in bytes diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index eb900d1dffb90..d6f607bad8d95 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -14,6 +14,7 @@ #include "kernel.h" #include "masternodeman.h" #include "policy/feerate.h" +#include "policy/policy.h" #include "rpc/server.h" #include "sync.h" #include "txdb.h" @@ -1149,9 +1150,13 @@ UniValue getchaintips(const JSONRPCRequest& request) UniValue mempoolInfoToJSON() { UniValue ret(UniValue::VOBJ); + ret.pushKV("loaded", g_is_mempool_loaded); ret.pushKV("size", (int64_t) mempool.size()); ret.pushKV("bytes", (int64_t) mempool.GetTotalTxSize()); ret.pushKV("usage", (int64_t) mempool.DynamicMemoryUsage()); + size_t maxmempool = gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000; + ret.pushKV("mempoolminfee", ValueFromAmount(std::max(mempool.GetMinFee(maxmempool), ::minRelayTxFee).GetFeePerK())); + ret.pushKV("minrelaytxfee", ValueFromAmount(::minRelayTxFee.GetFeePerK())); return ret; } @@ -1165,9 +1170,13 @@ UniValue getmempoolinfo(const JSONRPCRequest& request) "\nResult:\n" "{\n" + " \"loaded\": true|false (boolean) True if the mempool is fully loaded\n" " \"size\": xxxxx (numeric) Current tx count\n" " \"bytes\": xxxxx (numeric) Sum of all tx sizes\n" " \"usage\": xxxxx (numeric) Total memory usage for the mempool\n" + " \"maxmempool\": xxxxx, (numeric) Maximum memory usage for the mempool\n" + " \"mempoolminfee\": xxxxx (numeric) Minimum fee rate in " + CURRENCY_UNIT + "/kB for tx to be accepted. Is the maximum of minrelaytxfee and minimum mempool fee\n" + " \"minrelaytxfee\": xxxxx (numeric) Current minimum relay fee for transactions\n" "}\n" "\nExamples:\n" + diff --git a/test/functional/mempool_persist.py b/test/functional/mempool_persist.py index 4f5eeded70ae0..d6d2e2105b2f4 100755 --- a/test/functional/mempool_persist.py +++ b/test/functional/mempool_persist.py @@ -31,8 +31,14 @@ """ +from decimal import Decimal +import os + from test_framework.test_framework import PivxTestFramework -from test_framework.util import * +from test_framework.util import ( + assert_equal, + wait_until, +) class MempoolPersistTest(PivxTestFramework): def set_test_params(self): @@ -64,10 +70,12 @@ def run_test(self): self.start_node(1, extra_args=["-persistmempool=0"]) self.start_node(0) self.start_node(2) - # Give pivxd a second to reload the mempool - wait_until(lambda: len(self.nodes[0].getrawmempool()) == 5, timeout=1) - wait_until(lambda: len(self.nodes[2].getrawmempool()) == 5, timeout=1) - # The others loaded their mempool. If node_1 loaded anything, we'd probably notice by now: + + wait_until(lambda: self.nodes[0].getmempoolinfo()["loaded"], timeout=1) + wait_until(lambda: self.nodes[2].getmempoolinfo()["loaded"], timeout=1) + assert_equal(len(self.nodes[0].getrawmempool()), 5) + assert_equal(len(self.nodes[2].getrawmempool()), 5) + # The others have loaded their mempool. If node_1 loaded anything, we'd probably notice by now: assert_equal(len(self.nodes[1].getrawmempool()), 0) # Verify accounting of mempool transactions after restart is correct @@ -77,38 +85,39 @@ def run_test(self): self.log.debug("Stop-start node0 with -persistmempool=0. Verify that it doesn't load its mempool.dat file.") self.stop_nodes() self.start_node(0, extra_args=["-persistmempool=0"]) - # Give bitcoind a second to reload the mempool - time.sleep(1) + wait_until(lambda: self.nodes[0].getmempoolinfo()["loaded"]) assert_equal(len(self.nodes[0].getrawmempool()), 0) self.log.debug("Stop-start node0. Verify that it has the transactions in its mempool.") self.stop_nodes() self.start_node(0) - wait_until(lambda: len(self.nodes[0].getrawmempool()) == 5) + wait_until(lambda: self.nodes[0].getmempoolinfo()["loaded"]) + assert_equal(len(self.nodes[0].getrawmempool()), 5) # Following code is ahead of our current repository state. Future back port. + ''' + mempooldat0 = os.path.join(self.nodes[0].datadir, 'regtest', 'mempool.dat') + mempooldat1 = os.path.join(self.nodes[1].datadir, 'regtest', 'mempool.dat') + self.log.debug("Remove the mempool.dat file. Verify that savemempool to disk via RPC re-creates it") + os.remove(mempooldat0) + self.nodes[0].savemempool() + assert os.path.isfile(mempooldat0) + + self.log.debug("Stop nodes, make node1 use mempool.dat from node0. Verify it has 5 transactions") + os.rename(mempooldat0, mempooldat1) + self.stop_nodes() + self.start_node(1, extra_args=[]) + wait_until(lambda: self.nodes[1].getmempoolinfo()["loaded"]) + assert_equal(len(self.nodes[1].getrawmempool()), 5) - # mempooldat0 = os.path.join(self.options.tmpdir, 'node0', 'regtest', 'mempool.dat') - # mempooldat1 = os.path.join(self.options.tmpdir, 'node1', 'regtest', 'mempool.dat') - # self.log.debug("Remove the mempool.dat file. Verify that savemempool to disk via RPC re-creates it") - # os.remove(mempooldat0) - # self.nodes[0].savemempool() - # assert os.path.isfile(mempooldat0) - # - # self.log.debug("Stop nodes, make node1 use mempool.dat from node0. Verify it has 5 transactions") - # os.rename(mempooldat0, mempooldat1) - # self.stop_nodes() - # self.start_node(1, extra_args=[]) - # wait_until(lambda: len(self.nodes[1].getrawmempool()) == 5) - # - # self.log.debug("Prevent bitcoind from writing mempool.dat to disk. Verify that `savemempool` fails") - # # to test the exception we are setting bad permissions on a tmp file called mempool.dat.new - # # which is an implementation detail that could change and break this test - # mempooldotnew1 = mempooldat1 + '.new' - # with os.fdopen(os.open(mempooldotnew1, os.O_CREAT, 0o000), 'w'): - # pass - # assert_raises_rpc_error(-1, "Unable to dump mempool to disk", self.nodes[1].savemempool) - # os.remove(mempooldotnew1) + self.log.debug("Prevent bitcoind from writing mempool.dat to disk. Verify that `savemempool` fails") + # to test the exception we are creating a tmp folder called mempool.dat.new + # which is an implementation detail that could change and break this test + mempooldotnew1 = mempooldat1 + '.new' + os.mkdir(mempooldotnew1) + assert_raises_rpc_error(-1, "Unable to dump mempool to disk", self.nodes[1].savemempool) + os.rmdir(mempooldotnew1) + ''' if __name__ == '__main__': MempoolPersistTest().main() From a1d6c0caf9a624f8803ee5295886f97cd9d5015e Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Thu, 7 Mar 2019 09:54:44 -0500 Subject: [PATCH 05/31] Move g_is_mempool_loaded into CTxMemPool::m_is_loaded So the loaded state is explicitly mempool-specific. Github-Pull: #2254 Rebased-From: f0de7894d013b568bb2b482053fd301dd25cd803 --- src/init.cpp | 8 ++++---- src/rpc/blockchain.cpp | 2 +- src/txmempool.cpp | 12 ++++++++++++ src/txmempool.h | 10 +++++++++- src/validation.cpp | 25 +++++++++++++++---------- src/validation.h | 5 ++--- 6 files changed, 43 insertions(+), 19 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index e47aa2aa0b02a..ee7785a9c809c 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -248,8 +248,8 @@ void PrepareShutdown() DumpBudgets(g_budgetman); DumpMasternodePayments(); UnregisterNodeSignals(GetNodeSignals()); - if (g_is_mempool_loaded && gArgs.GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) { - DumpMempool(); + if (::mempool.IsLoaded() && gArgs.GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) { + DumpMempool(::mempool); } if (fFeeEstimatesInitialized) { @@ -728,9 +728,9 @@ void ThreadImport(const std::vector& vImportFiles) } if (gArgs.GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) { - LoadMempool(); + LoadMempool(::mempool); } - g_is_mempool_loaded = !fRequestShutdown; + ::mempool.SetIsLoaded(!ShutdownRequested()); } /** Sanity checks diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index d6f607bad8d95..c3a37a41896d6 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1150,7 +1150,7 @@ UniValue getchaintips(const JSONRPCRequest& request) UniValue mempoolInfoToJSON() { UniValue ret(UniValue::VOBJ); - ret.pushKV("loaded", g_is_mempool_loaded); + ret.pushKV("loaded", mempool.IsLoaded()); ret.pushKV("size", (int64_t) mempool.size()); ret.pushKV("bytes", (int64_t) mempool.GetTotalTxSize()); ret.pushKV("usage", (int64_t) mempool.DynamicMemoryUsage()); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 66f9b16f7baf6..91747ba11f4e9 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1213,3 +1213,15 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector* pvNoSpends } +bool CTxMemPool::IsLoaded() const +{ + LOCK(cs); + return m_is_loaded; +} + +void CTxMemPool::SetIsLoaded(bool loaded) +{ + LOCK(cs); + m_is_loaded = loaded; +} + diff --git a/src/txmempool.h b/src/txmempool.h index 635606d2aa970..9a3218ea3f557 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -419,6 +419,8 @@ class CTxMemPool std::map mapSaplingNullifiers; void checkNullifiers() const; + bool m_is_loaded GUARDED_BY(cs){false}; + public: static const int ROLLING_FEE_HALFLIFE = 60 * 60 * 12; // public only for testing @@ -618,7 +620,13 @@ class CTxMemPool /** Expire all transaction (and their dependencies) in the mempool older than time. Return the number of removed transactions. */ int Expire(int64_t time); - unsigned long size() + /** @returns true if the mempool is fully loaded */ + bool IsLoaded() const; + + /** Sets the current loaded state */ + void SetIsLoaded(bool loaded); + + unsigned long size() const { LOCK(cs); return mapTx.size(); diff --git a/src/validation.cpp b/src/validation.cpp index 71cd45954f028..0f9be3ec1da21 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -110,7 +110,6 @@ int64_t nMaxTipAge = DEFAULT_MAX_TIP_AGE; CFeeRate minRelayTxFee = CFeeRate(10000); CTxMemPool mempool(::minRelayTxFee); -std::atomic_bool g_is_mempool_loaded{false}; std::map mapRejectedBlocks; @@ -4066,7 +4065,7 @@ std::string CBlockFileInfo::ToString() const static const uint64_t MEMPOOL_DUMP_VERSION = 1; -bool LoadMempool(void) +bool LoadMempool(CTxMemPool& pool) { int64_t nExpiryTimeout = gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60; FILE* filestr = fopen((GetDataDir() / "mempool.dat").string().c_str(), "r"); @@ -4100,12 +4099,12 @@ bool LoadMempool(void) CAmount amountdelta = nFeeDelta; if (amountdelta) { - mempool.PrioritiseTransaction(tx->GetHash(), tx->GetHash().ToString(), prioritydummy, amountdelta); + pool.PrioritiseTransaction(tx->GetHash(), tx->GetHash().ToString(), prioritydummy, amountdelta); } CValidationState state; if (nTime + nExpiryTimeout > nNow) { LOCK(cs_main); - AcceptToMemoryPoolWithTime(mempool, state, tx, true, NULL, nTime); + AcceptToMemoryPoolWithTime(pool, state, tx, true, NULL, nTime); if (state.IsValid()) { ++count; } else { @@ -4121,7 +4120,7 @@ bool LoadMempool(void) file >> mapDeltas; for (const auto& i : mapDeltas) { - mempool.PrioritiseTransaction(i.first, i.first.ToString(), prioritydummy, i.second); + pool.PrioritiseTransaction(i.first, i.first.ToString(), prioritydummy, i.second); } } catch (const std::exception& e) { LogPrintf("Failed to deserialize mempool data on disk: %s. Continuing anyway.\n", e.what()); @@ -4132,19 +4131,22 @@ bool LoadMempool(void) return true; } -void DumpMempool(void) +bool DumpMempool(const CTxMemPool& pool) { int64_t start = GetTimeMicros(); std::map mapDeltas; std::vector vinfo; + static Mutex dump_mutex; + LOCK(dump_mutex); + { - LOCK(mempool.cs); - for (const auto &i : mempool.mapDeltas) { + LOCK(pool.cs); + for (const auto &i : pool.mapDeltas) { mapDeltas[i.first] = i.second.second; } - vinfo = mempool.infoAll(); + vinfo = pool.infoAll(); } int64_t mid = GetTimeMicros(); @@ -4152,7 +4154,7 @@ void DumpMempool(void) try { FILE* filestr = fopen((GetDataDir() / "mempool.dat.new").string().c_str(), "w"); if (!filestr) { - return; + return false; } CAutoFile file(filestr, SER_DISK, CLIENT_VERSION); @@ -4178,7 +4180,9 @@ void DumpMempool(void) LogPrintf("Dumped mempool: %gs to copy, %gs to dump\n", (mid-start)*0.000001, (last-mid)*0.000001); } catch (const std::exception& e) { LogPrintf("Failed to dump mempool: %s. Continuing anyway.\n", e.what()); + return false; } + return true; } class CMainCleanup @@ -4194,3 +4198,4 @@ class CMainCleanup mapBlockIndex.clear(); } } instance_of_cmaincleanup; + diff --git a/src/validation.h b/src/validation.h index f8d2242b8b4c5..1bbe01aa687d4 100644 --- a/src/validation.h +++ b/src/validation.h @@ -124,7 +124,6 @@ struct BlockHasher { extern CScript COINBASE_FLAGS; extern RecursiveMutex cs_main; extern CTxMemPool mempool; -extern std::atomic_bool g_is_mempool_loaded; typedef std::unordered_map BlockMap; extern BlockMap mapBlockIndex; extern uint64_t nLastBlockTx; @@ -401,9 +400,9 @@ static const unsigned int REJECT_ALREADY_KNOWN = 0x101; static const unsigned int REJECT_CONFLICT = 0x102; /** Dump the mempool to disk. */ -void DumpMempool(); +bool DumpMempool(const CTxMemPool& pool); /** Load the mempool from disk. */ -bool LoadMempool(); +bool LoadMempool(CTxMemPool& pool); #endif // BITCOIN_MAIN_H From d902128b12df51dbdcb604d0d88059a4860926d3 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Fri, 19 Mar 2021 23:34:20 +0100 Subject: [PATCH 06/31] [Test] Fix intermittent sync_blocks failures >>> backports bitcoin/bitcoin@fa3f9a05660687bf4146e089050e944a1d6cbe3c Github-Pull: #2254 Rebased-From: d076c814bd9b8bdbcf98c7df87297cc8cd2a533c --- test/functional/feature_reindex.py | 6 ++---- test/functional/mempool_persist.py | 11 +++++------ test/functional/test_framework/test_node.py | 21 +++++++++++++++++++-- 3 files changed, 26 insertions(+), 12 deletions(-) diff --git a/test/functional/feature_reindex.py b/test/functional/feature_reindex.py index 234d1ad899f30..5d2333a481c8a 100755 --- a/test/functional/feature_reindex.py +++ b/test/functional/feature_reindex.py @@ -10,7 +10,7 @@ """ from test_framework.test_framework import PivxTestFramework -from test_framework.util import wait_until +from test_framework.util import assert_equal import time class ReindexTest(PivxTestFramework): @@ -23,11 +23,9 @@ def reindex(self): self.nodes[0].generate(3) blockcount = self.nodes[0].getblockcount() self.stop_nodes() - time.sleep(5) extra_args = [["-reindex", "-checkblockindex=1"]] self.start_nodes(extra_args) - time.sleep(15) - wait_until(lambda: self.nodes[0].getblockcount() == blockcount) + assert_equal(self.nodes[0].getblockcount(), blockcount) # start_node is blocking on reindex self.log.info("Success") def run_test(self): diff --git a/test/functional/mempool_persist.py b/test/functional/mempool_persist.py index d6d2e2105b2f4..e81a9244d938f 100755 --- a/test/functional/mempool_persist.py +++ b/test/functional/mempool_persist.py @@ -70,9 +70,8 @@ def run_test(self): self.start_node(1, extra_args=["-persistmempool=0"]) self.start_node(0) self.start_node(2) - - wait_until(lambda: self.nodes[0].getmempoolinfo()["loaded"], timeout=1) - wait_until(lambda: self.nodes[2].getmempoolinfo()["loaded"], timeout=1) + assert self.nodes[0].getmempoolinfo()["loaded"] # start_node is blocking on the mempool being loaded + assert self.nodes[2].getmempoolinfo()["loaded"] assert_equal(len(self.nodes[0].getrawmempool()), 5) assert_equal(len(self.nodes[2].getrawmempool()), 5) # The others have loaded their mempool. If node_1 loaded anything, we'd probably notice by now: @@ -85,13 +84,13 @@ def run_test(self): self.log.debug("Stop-start node0 with -persistmempool=0. Verify that it doesn't load its mempool.dat file.") self.stop_nodes() self.start_node(0, extra_args=["-persistmempool=0"]) - wait_until(lambda: self.nodes[0].getmempoolinfo()["loaded"]) + assert self.nodes[0].getmempoolinfo()["loaded"] assert_equal(len(self.nodes[0].getrawmempool()), 0) self.log.debug("Stop-start node0. Verify that it has the transactions in its mempool.") self.stop_nodes() self.start_node(0) - wait_until(lambda: self.nodes[0].getmempoolinfo()["loaded"]) + assert self.nodes[0].getmempoolinfo()["loaded"] assert_equal(len(self.nodes[0].getrawmempool()), 5) # Following code is ahead of our current repository state. Future back port. @@ -107,7 +106,7 @@ def run_test(self): os.rename(mempooldat0, mempooldat1) self.stop_nodes() self.start_node(1, extra_args=[]) - wait_until(lambda: self.nodes[1].getmempoolinfo()["loaded"]) + assert self.nodes[0].getmempoolinfo()["loaded"] assert_equal(len(self.nodes[1].getrawmempool()), 5) self.log.debug("Prevent bitcoind from writing mempool.dat to disk. Verify that `savemempool` fails") diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 5436a3b40c6b8..e9664b7200235 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -121,8 +121,25 @@ def wait_for_rpc_connection(self): assert self.process.poll() is None, "pivxd exited with status %i during initialization" % self.process.returncode try: self.rpc = get_rpc_proxy(rpc_url(self.datadir, self.index, self.rpchost), self.index, timeout=self.rpc_timeout, coveragedir=self.coverage_dir) - while self.rpc.getblockcount() < 0: - time.sleep(1) + self.rpc.getblockcount() + wait_until(lambda: self.rpc.getmempoolinfo()['loaded']) + # Wait for the node to finish reindex, block import, and + # loading the mempool. Usually importing happens fast or + # even "immediate" when the node is started. However, there + # is no guarantee and sometimes ThreadImport might finish + # later. This is going to cause intermittent test failures, + # because generally the tests assume the node is fully + # ready after being started. + # + # For example, the node will reject block messages from p2p + # when it is still importing with the error "Unexpected + # block message received" + # + # The wait is done here to make tests as robust as possible + # and prevent racy tests and intermittent failures as much + # as possible. Some tests might not need this, but the + # overhead is trivial, and the added gurantees are worth + # the minimal performance cost. # If the call to getblockcount() succeeds then the RPC connection is up self.rpc_connected = True self.url = self.rpc.url From f806584edd4cbd2e06d021fc4666c8ae311edab1 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Sat, 20 Mar 2021 00:25:57 +0100 Subject: [PATCH 07/31] test: move sync_blocks and sync_mempool functions to test_framework.py >>> backports bitcoin/bitcoin@cc84460c164bcb2a874d4f08b3a2624e5ee9ff0a Github-Pull: #2254 Rebased-From: 2488ebccb4eb8e9f4fdd7a4d321b37616efe9a8a --- test/functional/example_test.py | 4 +- test/functional/feature_fee_estimation.py | 6 +- test/functional/mempool_packages.py | 8 +-- test/functional/mining_pos_coldStaking.py | 47 ++++++-------- test/functional/mining_pos_fakestake.py | 7 +- test/functional/mining_pos_reorg.py | 8 +-- test/functional/p2p_feefilter.py | 4 +- test/functional/p2p_sendheaders.py | 4 +- test/functional/p2p_unrequested_blocks.py | 2 +- test/functional/rpc_getchaintips.py | 3 +- test/functional/rpc_invalidateblock.py | 4 +- test/functional/sapling_fillblock.py | 10 ++- test/functional/sapling_mempool.py | 5 +- test/functional/sapling_wallet.py | 3 +- test/functional/sapling_wallet_anchorfork.py | 4 +- .../test_framework/test_framework.py | 64 +++++++++++++++---- test/functional/test_framework/util.py | 42 ------------ .../tiertwo_masternode_activation.py | 6 +- test/functional/tiertwo_masternode_ping.py | 11 ++-- test/functional/wallet_abandonconflict.py | 12 ++-- test/functional/wallet_backup.py | 24 ++++--- test/functional/wallet_basic.py | 20 +++--- test/functional/wallet_bumpfee.py | 8 +-- test/functional/wallet_import_rescan.py | 6 +- .../wallet_import_stakingaddress.py | 3 +- test/functional/wallet_keypool_topup.py | 3 +- test/functional/wallet_listreceivedby.py | 3 +- test/functional/wallet_listsinceblock.py | 3 +- test/functional/wallet_listtransactions.py | 5 +- test/functional/wallet_reorgsrestore.py | 5 +- test/functional/wallet_txn_clone.py | 4 +- test/functional/wallet_txn_doublespend.py | 4 +- test/functional/wallet_zapwallettxes.py | 3 +- 33 files changed, 161 insertions(+), 184 deletions(-) diff --git a/test/functional/example_test.py b/test/functional/example_test.py index 16bf2842365bc..757a065dd2b6e 100755 --- a/test/functional/example_test.py +++ b/test/functional/example_test.py @@ -113,7 +113,7 @@ def setup_network(self): # sync_all() should not include node2, since we're not expecting it to # sync. connect_nodes(self.nodes[0], 1) - self.sync_all([self.nodes[0:1]]) + self.sync_all(self.nodes[0:1]) # Use setup_nodes() to customize the node start behaviour (for example if # you don't want to start all nodes at the start of the test). @@ -143,7 +143,7 @@ def run_test(self): # Generating a block on one of the nodes will get us out of IBD blocks = [int(self.nodes[0].generate(1)[0], 16)] - self.sync_all([self.nodes[0:1]]) + self.sync_all(self.nodes[0:1]) # Notice above how we called an RPC by calling a method with the same # name on the node object. Notice also how we used a keyword argument diff --git a/test/functional/feature_fee_estimation.py b/test/functional/feature_fee_estimation.py index b13f1df94cb33..e9b419b4974d0 100755 --- a/test/functional/feature_fee_estimation.py +++ b/test/functional/feature_fee_estimation.py @@ -191,9 +191,9 @@ def transact_and_mine(self, numblocks, mining_node): self.memutxo, Decimal("0.05"), MIN_FEE, MIN_FEE) tx_kbytes = (len(txhex) // 2) / 1000.0 self.fees_per_kb.append(float(fee)/tx_kbytes) - sync_mempools(self.nodes[0:3], wait=.1) + self.sync_mempools(self.nodes[0:3], wait=.1) mined = mining_node.getblock(mining_node.generate(1)[0],True)["tx"] - sync_blocks(self.nodes[0:3], wait=.1) + self.sync_blocks(self.nodes[0:3], wait=.1) # update which txouts are confirmed newmem = [] for utx in self.memutxo: @@ -270,7 +270,7 @@ def run_test(self): while len(self.nodes[1].getrawmempool()) > 0: self.nodes[1].generate(1) - sync_blocks(self.nodes[0:3], wait=.1) + self.sync_blocks(self.nodes[0:3], wait=.1) self.log.info("Final estimates after emptying mempools") check_estimates(self.nodes[1], self.fees_per_kb, 2) diff --git a/test/functional/mempool_packages.py b/test/functional/mempool_packages.py index 14f9df5265275..aa2b1a7256e2a 100755 --- a/test/functional/mempool_packages.py +++ b/test/functional/mempool_packages.py @@ -12,8 +12,6 @@ Decimal, ROUND_DOWN, JSONRPCException, - sync_blocks, - sync_mempools ) def satoshi_round(amount): @@ -141,7 +139,7 @@ def run_test(self): # Test reorg handling # First, the basics: self.nodes[0].generate(1) - sync_blocks(self.nodes) + self.sync_blocks() self.nodes[1].invalidateblock(self.nodes[0].getbestblockhash()) self.nodes[1].reconsiderblock(self.nodes[0].getbestblockhash()) @@ -196,12 +194,12 @@ def run_test(self): rawtx = self.nodes[0].createrawtransaction(inputs, outputs) signedtx = self.nodes[0].signrawtransaction(rawtx) txid = self.nodes[0].sendrawtransaction(signedtx['hex']) - sync_mempools(self.nodes) + self.sync_mempools() # Now try to disconnect the tip on each node... self.nodes[1].invalidateblock(self.nodes[1].getbestblockhash()) self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash()) - sync_blocks(self.nodes) + self.sync_blocks() if __name__ == '__main__': MempoolPackagesTest().main() \ No newline at end of file diff --git a/test/functional/mining_pos_coldStaking.py b/test/functional/mining_pos_coldStaking.py index edd459037e4d5..07a6b2e0e2b3b 100755 --- a/test/functional/mining_pos_coldStaking.py +++ b/test/functional/mining_pos_coldStaking.py @@ -19,8 +19,6 @@ p2p_port, bytes_to_hex_str, set_node_times, - sync_blocks, - sync_mempools, ) from decimal import Decimal @@ -102,7 +100,7 @@ def run_test(self): for peer in [0, 2]: for j in range(25): self.mocktime = self.generate_pow(peer, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() # 2) node[1] sends his entire balance (50 mature rewards) to node[2] # - node[2] stakes a block - node[1] locks the change @@ -112,9 +110,9 @@ def run_test(self): assert_equal(self.nodes[1].getbalance(), 50 * 250) txid = self.nodes[1].sendtoaddress(self.nodes[2].getnewaddress(), (50 * 250 - 0.01)) assert (txid is not None) - sync_mempools(self.nodes) + self.sync_mempools() self.mocktime = self.generate_pos(2, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() # lock the change output (so it's not used as stake input in generate_pos) for x in self.nodes[1].listunspent(): assert (self.nodes[1].lockunspent(False, [{"txid": x['txid'], "vout": x['vout']}])) @@ -128,7 +126,7 @@ def run_test(self): self.sync_all() for i in range(6): self.mocktime = self.generate_pow(0, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() assert_equal(self.nodes[0].getshieldbalance(), 250) # 3) nodes[0] generates a owner address @@ -193,9 +191,9 @@ def run_test(self): assert_equal(res["staker_address"], staker_address) fee = self.nodes[0].viewshieldtransaction(res["txid"])['fee'] # sync and mine 2 blocks - sync_mempools(self.nodes) + self.sync_mempools() self.mocktime = self.generate_pos(2, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() self.log.info("%d Txes created." % NUM_OF_INPUTS) # check balances: self.expected_balance = NUM_OF_INPUTS * INPUT_VALUE @@ -215,9 +213,9 @@ def run_test(self): txhash = self.spendUTXOwithNode(u, 0) assert(txhash != None) self.log.info("Good. Owner was able to spend - tx: %s" % str(txhash)) - sync_mempools(self.nodes) + self.sync_mempools() self.mocktime = self.generate_pos(2, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() # check tx self.check_tx_in_chain(0, txhash) self.check_tx_in_chain(1, txhash) @@ -251,7 +249,7 @@ def run_test(self): self.spendUTXOwithNode, u, 1) self.log.info("Good. Cold staker was NOT able to spend (failed OP_CHECKCOLDSTAKEVERIFY)") self.mocktime = self.generate_pos(2, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() # 9) check that the staker can use the coins to stake a block with internal miner. # -------------------------------------------------------------------------------- @@ -264,7 +262,7 @@ def run_test(self): self.log.info("Block %s submitted" % newblockhash) # Verify that nodes[0] accepts it - sync_blocks(self.nodes) + self.sync_blocks() assert_equal(self.nodes[0].getblockcount(), self.nodes[1].getblockcount()) assert_equal(newblockhash, self.nodes[0].getbestblockhash()) self.log.info("Great. Cold-staked block was accepted!") @@ -292,7 +290,7 @@ def run_test(self): assert_equal(new_block.hash, self.nodes[1].getbestblockhash()) # Verify that nodes[0] accepts it - sync_blocks(self.nodes) + self.sync_blocks() assert_equal(self.nodes[0].getblockcount(), self.nodes[1].getblockcount()) assert_equal(new_block.hash, self.nodes[0].getbestblockhash()) self.log.info("Great. Cold-staked block was accepted!") @@ -321,7 +319,7 @@ def run_test(self): assert("rejected" in ret) # Verify that nodes[0] rejects it - sync_blocks(self.nodes) + self.sync_blocks() assert_raises_rpc_error(-5, "Block not found", self.nodes[0].getblock, new_block.hash) self.log.info("Great. Malicious cold-staked block was NOT accepted!") self.checkBalances() @@ -345,7 +343,7 @@ def run_test(self): assert_equal(ret, "bad-p2cs-outs") # Verify that nodes[0] rejects it - sync_blocks(self.nodes) + self.sync_blocks() assert_raises_rpc_error(-5, "Block not found", self.nodes[0].getblock, new_block.hash) self.log.info("Great. Malicious cold-staked block was NOT accepted!") self.checkBalances() @@ -355,7 +353,7 @@ def run_test(self): # ---------------------------------------------------------------------------------------- self.log.info("Let's void the contracts.") self.mocktime = self.generate_pos(2, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() print("*** 13 ***") self.log.info("Cancel the stake delegation spending the delegated utxos...") delegated_utxos = getDelegatedUtxos(self.nodes[0].listunspent()) @@ -364,9 +362,9 @@ def run_test(self): txhash = self.spendUTXOsWithNode(delegated_utxos, 0) assert(txhash != None) self.log.info("Good. Owner was able to void the stake delegations - tx: %s" % str(txhash)) - sync_mempools(self.nodes) + self.sync_blocks() self.mocktime = self.generate_pos(2, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() # deactivate SPORK 17 and check that the owner can still spend the last utxo self.setColdStakingEnforcement(False) @@ -374,9 +372,9 @@ def run_test(self): txhash = self.spendUTXOsWithNode([final_spend], 0) assert(txhash != None) self.log.info("Good. Owner was able to void a stake delegation (with SPORK 17 disabled) - tx: %s" % str(txhash)) - sync_mempools(self.nodes) + self.sync_mempools() self.mocktime = self.generate_pos(2, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() # check tx self.check_tx_in_chain(0, txhash) self.check_tx_in_chain(1, txhash) @@ -403,7 +401,7 @@ def run_test(self): for peer in [0, 2]: for j in range(25): self.mocktime = self.generate_pos(peer, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() self.expected_balance = self.expected_immature_balance self.expected_immature_balance = 0 self.checkBalances() @@ -411,9 +409,9 @@ def run_test(self): txhash = self.spendUTXOsWithNode(delegated_utxos, 0) assert (txhash != None) self.log.info("Good. Owner was able to spend the cold staked coins - tx: %s" % str(txhash)) - sync_mempools(self.nodes) + self.sync_mempools() self.mocktime = self.generate_pos(2, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() # check tx self.check_tx_in_chain(0, txhash) self.check_tx_in_chain(1, txhash) @@ -481,8 +479,5 @@ def add_output_to_coinstake(self, block, value, peer=1): block.re_sign_block() - - - if __name__ == '__main__': PIVX_ColdStakingTest().main() diff --git a/test/functional/mining_pos_fakestake.py b/test/functional/mining_pos_fakestake.py index 8c10db256cca2..49321c8c9f13a 100755 --- a/test/functional/mining_pos_fakestake.py +++ b/test/functional/mining_pos_fakestake.py @@ -50,7 +50,6 @@ from test_framework.messages import COutPoint from test_framework.test_framework import PivxTestFramework from test_framework.util import ( - sync_blocks, assert_equal, bytes_to_hex_str, set_node_times @@ -93,7 +92,7 @@ def run_test(self): self.log.info("Mining 50 blocks to reach PoS phase...") for i in range(50): self.mocktime = self.generate_pow(0, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() # Check Tests 1-3 self.test_1() @@ -116,7 +115,7 @@ def test_1(self): self.log.info("Mining 5 blocks as fork depth...") for i in range(5): self.mocktime = self.generate_pow(0, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() # nodes[1] spams 3 blocks with height 256 --> [REJECTED] assert_equal(self.nodes[1].getblockcount(), 255) @@ -138,7 +137,7 @@ def test_2(self): self.log.info("Mining 5 blocks to include the spends...") for i in range(5): self.mocktime = self.generate_pow(0, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() self.check_tx_in_chain(0, txid) assert_equal(self.nodes[1].getbalance(), 0) diff --git a/test/functional/mining_pos_reorg.py b/test/functional/mining_pos_reorg.py index 831b8c3c4ef93..d59ee8d8006d8 100755 --- a/test/functional/mining_pos_reorg.py +++ b/test/functional/mining_pos_reorg.py @@ -3,10 +3,8 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. -from test_framework.authproxy import JSONRPCException from test_framework.test_framework import PivxTestFramework from test_framework.util import ( - sync_blocks, assert_equal, assert_raises_rpc_error, connect_nodes, @@ -113,7 +111,7 @@ def findUtxoInList(txid, vout, utxo_list): # Connect with node 2 and sync self.log.info("Reconnecting node 0 and node 2") connect_nodes(self.nodes[0], 2) - sync_blocks([self.nodes[i] for i in [0, 2]]) + self.sync_blocks([self.nodes[i] for i in [0, 2]]) # verify that the stakeinput can't be spent stakeinput_tx_json = self.nodes[0].getrawtransaction(stakeinput["txid"], True) @@ -143,7 +141,7 @@ def findUtxoInList(txid, vout, utxo_list): self.log.info("Connecting and syncing nodes...") set_node_times(self.nodes, block_time_1) connect_nodes_clique(self.nodes) - sync_blocks(self.nodes) + self.sync_blocks() for i in [0, 2]: assert_equal(self.nodes[i].getbestblockhash(), new_best_hash) @@ -158,7 +156,7 @@ def findUtxoInList(txid, vout, utxo_list): stakeinput["txid"][:9], stakeinput["txid"][-4:], stakeinput["vout"])) self.nodes[0].sendrawtransaction(rawtx["hex"]) self.nodes[1].generate(1) - sync_blocks(self.nodes) + self.sync_blocks() res, utxo = findUtxoInList(stakeinput["txid"], stakeinput["vout"], self.nodes[0].listunspent()) assert (not res or not utxo["spendable"]) diff --git a/test/functional/p2p_feefilter.py b/test/functional/p2p_feefilter.py index a3fa5713d9902..e7e77429a638b 100755 --- a/test/functional/p2p_feefilter.py +++ b/test/functional/p2p_feefilter.py @@ -45,7 +45,7 @@ def run_test(self): node0 = self.nodes[0] # Get out of IBD node1.generate(1) - sync_blocks(self.nodes) + self.sync_blocks() # Setup the p2p connections and start up the network thread. self.nodes[0].add_p2p_connection(TestNode()) @@ -69,7 +69,7 @@ def run_test(self): # Change tx fee rate to 10 sat/byte and test they are no longer received node1.settxfee(float(0.00010000)) [node1.sendtoaddress(node1.getnewaddress(), 1) for x in range(3)] - sync_mempools(self.nodes) # must be sure node 0 has received all txs + self.sync_mempools() # must be sure node 0 has received all txs # Send one transaction from node0 that should be received, so that we # we can sync the test on receipt (if node1's txs were relayed, they'd diff --git a/test/functional/p2p_sendheaders.py b/test/functional/p2p_sendheaders.py index 950bf60af09ce..e4de0fb59054a 100755 --- a/test/functional/p2p_sendheaders.py +++ b/test/functional/p2p_sendheaders.py @@ -247,7 +247,7 @@ def mine_blocks(self, count): # return the list of block hashes newly mined def mine_reorg(self, length): self.nodes[0].generate(length) # make sure all invalidated blocks are node0's - sync_blocks(self.nodes, wait=0.1) + self.sync_blocks(self.nodes, wait=0.1) for x in self.p2p_connections: x.wait_for_block_announcement(int(self.nodes[0].getbestblockhash(), 16)) x.clear_last_announcement() @@ -256,7 +256,7 @@ def mine_reorg(self, length): hash_to_invalidate = self.nodes[1].getblockhash(tip_height-(length-1)) self.nodes[1].invalidateblock(hash_to_invalidate) all_hashes = self.nodes[1].generate(length+1) # Must be longer than the orig chain - sync_blocks(self.nodes, wait=0.1) + self.sync_blocks(self.nodes, wait=0.1) return [int(x, 16) for x in all_hashes] def run_test(self): diff --git a/test/functional/p2p_unrequested_blocks.py b/test/functional/p2p_unrequested_blocks.py index 17abd8fc35e3e..3b873d4efed54 100755 --- a/test/functional/p2p_unrequested_blocks.py +++ b/test/functional/p2p_unrequested_blocks.py @@ -316,7 +316,7 @@ def run_test(self): # 9. Connect node1 to node0 and ensure it is able to sync connect_nodes(self.nodes[0], 1) - sync_blocks([self.nodes[0], self.nodes[1]]) + self.sync_blocks([self.nodes[0], self.nodes[1]]) self.log.info("Successfully synced nodes 1 and 0") if __name__ == '__main__': diff --git a/test/functional/rpc_getchaintips.py b/test/functional/rpc_getchaintips.py index d7fc7d52fc354..fd0646079a975 100755 --- a/test/functional/rpc_getchaintips.py +++ b/test/functional/rpc_getchaintips.py @@ -28,7 +28,8 @@ def run_test (self): self.split_network () self.nodes[0].generate(10) self.nodes[2].generate(20) - self.sync_all([self.nodes[:2], self.nodes[2:]]) + self.sync_all(self.nodes[:2]) + self.sync_all(self.nodes[2:]) tips = self.nodes[1].getchaintips () assert_equal (len (tips), 1) diff --git a/test/functional/rpc_invalidateblock.py b/test/functional/rpc_invalidateblock.py index 8e1f2352df7ab..eeb8b1be245c8 100755 --- a/test/functional/rpc_invalidateblock.py +++ b/test/functional/rpc_invalidateblock.py @@ -28,7 +28,7 @@ def run_test(self): self.log.info("Connect nodes to force a reorg") connect_nodes(self.nodes[0], 1) - sync_blocks(self.nodes[0:2]) + self.sync_blocks(self.nodes[0:2]) assert_equal(self.nodes[0].getblockcount(), 6) badhash = self.nodes[1].getblockhash(2) @@ -40,7 +40,7 @@ def run_test(self): self.log.info("Make sure we won't reorg to a lower work chain:") connect_nodes(self.nodes[1], 2) self.log.info("Sync node 2 to node 1 so both have 6 blocks") - sync_blocks(self.nodes[1:3]) + self.sync_blocks(self.nodes[1:3]) assert_equal(self.nodes[2].getblockcount(), 6) self.log.info("Invalidate block 5 on node 1 so its tip is now at 4") self.nodes[1].invalidateblock(self.nodes[1].getblockhash(5)) diff --git a/test/functional/sapling_fillblock.py b/test/functional/sapling_fillblock.py index 82eb0710bac54..e1b64e5bdf2ff 100755 --- a/test/functional/sapling_fillblock.py +++ b/test/functional/sapling_fillblock.py @@ -11,8 +11,6 @@ assert_equal, Decimal, satoshi_round, - sync_blocks, - sync_mempools, ) import time @@ -53,7 +51,7 @@ def utxo_splitter(self, node_from, n_inputs, node_to): def check_mempool(self, miner, txids): self.log.info("Checking mempool...") - sync_mempools(self.nodes) + self.sync_mempools() mempool_info = miner.getmempoolinfo() assert_equal(mempool_info['size'], len(txids)) mempool_bytes = mempool_info['bytes'] @@ -77,7 +75,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) + self.sync_mempools() return txids @@ -87,7 +85,7 @@ def run_test(self): # First mine 300 blocks self.log.info("Generating 300 blocks...") miner.generate(300) - sync_blocks(self.nodes) + self.sync_blocks() assert_equal(self.nodes[0].getblockchaininfo()['upgrades']['v5 shield']['status'], 'active') ## -- First check that the miner never produces blocks with more than 750kB of shielded txes @@ -99,7 +97,7 @@ def run_test(self): txids = self.utxo_splitter(miner, UTXOS_TO_SPLIT, alice) assert_equal(len(txids), UTXOS_TO_SPLIT) miner.generate(2) - sync_blocks(self.nodes) + self.sync_blocks() new_utxos = alice.listunspent() assert_equal(len(new_utxos), UTXOS_TO_SHIELD) diff --git a/test/functional/sapling_mempool.py b/test/functional/sapling_mempool.py index 711c3f97f50dc..1e3586c64da37 100755 --- a/test/functional/sapling_mempool.py +++ b/test/functional/sapling_mempool.py @@ -7,7 +7,6 @@ from test_framework.util import ( assert_equal, assert_raises_rpc_error, - sync_mempools, ) from decimal import Decimal @@ -53,7 +52,7 @@ def run_test(self): # Miner receives tx_B and accepts it in the mempool assert (txid_B in alice.getrawmempool()) - sync_mempools(self.nodes) + self.sync_mempools() assert(txid_B in miner.getrawmempool()) self.log.info("tx_B accepted in the memory pool.") @@ -87,7 +86,7 @@ def run_test(self): txid_C = alice.sendrawtransaction(txC_hex) # Miner receives tx_C and accepts it in the mempool - sync_mempools(self.nodes) + self.sync_mempools() assert(txid_C in miner.getrawmempool()) self.log.info("tx_C accepted in the memory pool.") diff --git a/test/functional/sapling_wallet.py b/test/functional/sapling_wallet.py index 0af64085b9185..c9600cc342c28 100755 --- a/test/functional/sapling_wallet.py +++ b/test/functional/sapling_wallet.py @@ -11,7 +11,6 @@ connect_nodes, disconnect_nodes, satoshi_round, - sync_mempools, get_coinstake_address, wait_until, ) @@ -29,7 +28,7 @@ def set_test_params(self): self.extra_args[0].append('-sporkkey=932HEevBSujW2ud7RfB1YF91AFygbBRQj3de3LyaCRqNzKKgWXi') def check_tx_priority(self, txids): - sync_mempools(self.nodes) + self.sync_mempools() mempool = self.nodes[0].getrawmempool(True) for txid in txids: assert(Decimal(mempool[txid]['startingpriority']) == Decimal('1E+25')) diff --git a/test/functional/sapling_wallet_anchorfork.py b/test/functional/sapling_wallet_anchorfork.py index 3230015a33026..92a4666885f34 100755 --- a/test/functional/sapling_wallet_anchorfork.py +++ b/test/functional/sapling_wallet_anchorfork.py @@ -63,7 +63,7 @@ def run_test (self): # Partition B, node 1 mines an empty block self.nodes[1].generate(1) - sync_blocks(self.nodes[1:3]) + self.sync_blocks(self.nodes[1:3]) # Check partition assert_equal(self.nodes[1].getblockcount(), self.nodes[2].getblockcount()) @@ -77,7 +77,7 @@ def run_test (self): # Partition A, node 0 mines a block with the transaction self.nodes[0].generate(1) - self.sync_all([self.nodes[1:3]]) + self.sync_all(self.nodes[1:3]) # Partition B, node 1 mines the same shield transaction txid2 = self.nodes[1].sendrawtransaction(rawhex) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 436d68694eb43..46805f7d01f1f 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -57,8 +57,6 @@ set_node_times, SPORK_ACTIVATION_TIME, SPORK_DEACTIVATION_TIME, - sync_blocks, - sync_mempools, vZC_DENOMS, wait_until, ) @@ -365,7 +363,8 @@ def split_network(self): """ disconnect_nodes(self.nodes[1], 2) disconnect_nodes(self.nodes[2], 1) - self.sync_all([self.nodes[:2], self.nodes[2:]]) + self.sync_all(self.nodes[:2]) + self.sync_all(self.nodes[2:]) def join_network(self): """ @@ -374,13 +373,52 @@ def join_network(self): connect_nodes(self.nodes[1], 2) self.sync_all() - def sync_all(self, node_groups=None): - if not node_groups: - node_groups = [self.nodes] - - for group in node_groups: - sync_blocks(group) - sync_mempools(group) + def sync_blocks(self, nodes=None, wait=1, timeout=60): + """ + Wait until everybody has the same tip. + sync_blocks needs to be called with an rpc_connections set that has least + one node already synced to the latest, stable tip, otherwise there's a + chance it might return before all nodes are stably synced. + """ + rpc_connections = nodes or self.nodes + stop_time = time.time() + timeout + while time.time() <= stop_time: + best_hash = [x.getbestblockhash() for x in rpc_connections] + if best_hash.count(best_hash[0]) == len(rpc_connections): + return + # Check that each peer has at least one connection + assert (all([len(x.getpeerinfo()) for x in rpc_connections])) + time.sleep(wait) + raise AssertionError("Block sync timed out after {}s:{}".format( + timeout, + "".join("\n {!r}".format(b) for b in best_hash), + )) + + def sync_mempools(self, nodes=None, wait=1, timeout=60, flush_scheduler=True): + """ + Wait until everybody has the same transactions in their memory + pools + """ + rpc_connections = nodes or self.nodes + stop_time = time.time() + timeout + 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() + return + # Check that each peer has at least one connection + assert (all([len(x.getpeerinfo()) for x in rpc_connections])) + time.sleep(wait) + raise AssertionError("Mempool sync timed out after {}s:{}".format( + timeout, + "".join("\n {!r}".format(m) for m in pool), + )) + + def sync_all(self, nodes=None): + self.sync_blocks(nodes) + self.sync_mempools(nodes) def enable_mocktime(self): """Enable mocktime for the script. @@ -551,7 +589,7 @@ def generate_pow_cache(): self.nodes[peer].generate(1) block_time += 60 # Must sync before next peer starts generating blocks - sync_blocks(self.nodes) + self.sync_blocks() # Shut them down, and clean up cache directories: self.log.info("Stopping nodes") @@ -1018,7 +1056,7 @@ def send_pings(self, mnodes): def stake_and_sync(self, node_id, num_blocks): for i in range(num_blocks): self.mocktime = self.generate_pos(node_id, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() time.sleep(1) @@ -1181,7 +1219,7 @@ def setup_2_masternodes_network(self): # First mine 250 PoW blocks for i in range(250): self.mocktime = self.generate_pow(self.minerPos, self.mocktime) - sync_blocks(self.nodes) + self.sync_blocks() # Then start staking self.stake(9) diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 30495e47b296a..795b7f5d25933 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -390,48 +390,6 @@ def connect_nodes_clique(nodes): connect_nodes(nodes[a], b) connect_nodes(nodes[b], a) -def sync_blocks(rpc_connections, *, wait=1, timeout=60): - """ - Wait until everybody has the same tip. - - sync_blocks needs to be called with an rpc_connections set that has least - one node already synced to the latest, stable tip, otherwise there's a - chance it might return before all nodes are stably synced. - """ - stop_time = time.time() + timeout - while time.time() <= stop_time: - best_hash = [x.getbestblockhash() for x in rpc_connections] - if best_hash.count(best_hash[0]) == len(rpc_connections): - return - # Check that each peer has at least one connection - assert (all([len(x.getpeerinfo()) for x in rpc_connections])) - time.sleep(wait) - raise AssertionError("Block sync timed out after {}s:{}".format( - timeout, - "".join("\n {!r}".format(b) for b in best_hash), - )) - -def sync_mempools(rpc_connections, *, wait=1, timeout=60, flush_scheduler=True): - """ - Wait until everybody has the same transactions in their memory - pools - """ - stop_time = time.time() + timeout - 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() - return - # Check that each peer has at least one connection - assert (all([len(x.getpeerinfo()) for x in rpc_connections])) - time.sleep(wait) - raise AssertionError("Mempool sync timed out after {}s:{}".format( - timeout, - "".join("\n {!r}".format(m) for m in pool), - )) - # Transaction/Block functions ############################# diff --git a/test/functional/tiertwo_masternode_activation.py b/test/functional/tiertwo_masternode_activation.py index d04a4961b2716..e1c7ab08f77fb 100755 --- a/test/functional/tiertwo_masternode_activation.py +++ b/test/functional/tiertwo_masternode_activation.py @@ -9,8 +9,6 @@ connect_nodes_clique, disconnect_nodes, satoshi_round, - sync_blocks, - sync_mempools, wait_until, ) @@ -56,7 +54,7 @@ def spend_collateral(self): rawtx = self.ownerOne.createrawtransaction(inputs, outputs) signedtx = self.ownerOne.signrawtransaction(rawtx) txid = self.miner.sendrawtransaction(signedtx['hex']) - sync_mempools(self.nodes) + self.sync_mempools() self.log.info("Collateral spent in %s" % txid) self.send_pings([self.remoteTwo]) self.stake(1, [self.remoteTwo]) @@ -111,7 +109,7 @@ def run_test(self): self.advance_mocktime(30) self.log.info("spending the collateral now..") self.spend_collateral() - sync_blocks(self.nodes) + self.sync_blocks() self.log.info("checking mn status..") time.sleep(3) # wait a little bit self.wait_until_mn_vinspent(self.mnOneTxHash, 30, [self.remoteTwo]) diff --git a/test/functional/tiertwo_masternode_ping.py b/test/functional/tiertwo_masternode_ping.py index 96f43cb9c1591..ab7fc3fccf7c0 100755 --- a/test/functional/tiertwo_masternode_ping.py +++ b/test/functional/tiertwo_masternode_ping.py @@ -9,7 +9,6 @@ assert_greater_than, Decimal, p2p_port, - sync_blocks, ) import os @@ -38,7 +37,7 @@ def run_test(self): self.log.info("generating 141 blocks...") miner.generate(141) - sync_blocks(self.nodes) + self.sync_blocks() # Create collateral self.log.info("funding masternode controller...") @@ -46,7 +45,7 @@ def run_test(self): mnAddress = owner.getnewaddress(masternodeAlias) collateralTxId = miner.sendtoaddress(mnAddress, Decimal('10000')) miner.generate(2) - sync_blocks(self.nodes) + self.sync_blocks() time.sleep(1) collateral_rawTx = owner.getrawtransaction(collateralTxId, 1) assert_equal(owner.getbalance(), Decimal('10000')) @@ -88,14 +87,14 @@ def run_test(self): self.wait_until_mnsync_finished() self.log.info("MnSync completed in %d seconds" % (time.time() - start_time)) miner.generate(1) - sync_blocks(self.nodes) + self.sync_blocks() time.sleep(1) # Send Start message self.log.info("sending masternode broadcast...") self.controller_start_masternode(owner, masternodeAlias) miner.generate(1) - sync_blocks(self.nodes) + self.sync_blocks() time.sleep(1) # Wait until masternode is enabled everywhere (max 180 secs) @@ -106,7 +105,7 @@ def run_test(self): self.log.info("Masternode enabled in %d seconds" % (time.time() - start_time)) self.log.info("Good. Masternode enabled") miner.generate(1) - sync_blocks(self.nodes) + self.sync_blocks() time.sleep(1) last_seen = [self.get_mn_lastseen(node, collateralTxId) for node in self.nodes] diff --git a/test/functional/wallet_abandonconflict.py b/test/functional/wallet_abandonconflict.py index 81b4474a21be3..35a1b9e059ec0 100755 --- a/test/functional/wallet_abandonconflict.py +++ b/test/functional/wallet_abandonconflict.py @@ -11,8 +11,6 @@ connect_nodes, Decimal, disconnect_nodes, - sync_blocks, - sync_mempools ) class AbandonConflictTest(PivxTestFramework): @@ -23,14 +21,14 @@ def set_test_params(self): def run_test(self): self.nodes[0].generate(5) - sync_blocks(self.nodes) + self.sync_blocks() self.nodes[1].generate(110) - sync_blocks(self.nodes) + self.sync_blocks() balance = self.nodes[0].getbalance() txA = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 10) txB = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 10) txC = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 10) - sync_mempools(self.nodes) + self.sync_mempools() self.nodes[1].generate(1) # Can not abandon non-wallet transaction @@ -38,7 +36,7 @@ def run_test(self): # Can not abandon confirmed transaction assert_raises_rpc_error(-5, 'Transaction not eligible for abandonment', lambda: self.nodes[0].abandontransaction(txA)) - sync_blocks(self.nodes) + self.sync_blocks() newbalance = self.nodes[0].getbalance() assert(balance - newbalance < Decimal("0.001")) #no more than fees lost balance = newbalance @@ -156,7 +154,7 @@ def run_test(self): self.nodes[1].generate(1) connect_nodes(self.nodes[0], 1) - sync_blocks(self.nodes) + self.sync_blocks() # Verify that B and C's 10 BTC outputs are available for spending again because AB1 is now conflicted newbalance = self.nodes[0].getbalance() diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index 62d4ff731a3ad..1d2f519364b70 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -30,11 +30,17 @@ Shutdown again, restore using importwallet, and confirm again balances are correct. """ +from decimal import Decimal +import os from random import randint import shutil from test_framework.test_framework import PivxTestFramework -from test_framework.util import * +from test_framework.util import ( + assert_equal, + assert_raises_rpc_error, + connect_nodes, +) class WalletBackupTest(PivxTestFramework): def set_test_params(self): @@ -70,9 +76,9 @@ def do_one_round(self): # Have the miner (node3) mine a block. # Must sync mempools before mining. - sync_mempools(self.nodes) + self.sync_mempools() self.nodes[3].generate(1) - sync_blocks(self.nodes) + self.sync_blocks() # As above, this mirrors the original bash test. def start_three(self): @@ -97,13 +103,13 @@ def erase_three(self): def run_test(self): self.log.info("Generating initial blockchain") self.nodes[0].generate(1) - sync_blocks(self.nodes) + self.sync_blocks() self.nodes[1].generate(1) - sync_blocks(self.nodes) + self.sync_blocks() self.nodes[2].generate(1) - sync_blocks(self.nodes) + self.sync_blocks() self.nodes[3].generate(100) - sync_blocks(self.nodes) + self.sync_blocks() assert_equal(self.nodes[0].getbalance(), 250) assert_equal(self.nodes[1].getbalance(), 250) @@ -160,7 +166,7 @@ def run_test(self): self.log.info("Re-starting nodes") self.start_three() - sync_blocks(self.nodes) + self.sync_blocks() assert_equal(self.nodes[0].getbalance(), balance0) assert_equal(self.nodes[1].getbalance(), balance1) @@ -184,7 +190,7 @@ def run_test(self): self.nodes[1].importwallet(tmpdir + "/node1/wallet.dump") self.nodes[2].importwallet(tmpdir + "/node2/wallet.dump") - sync_blocks(self.nodes) + self.sync_blocks() assert_equal(self.nodes[0].getbalance(), balance0) assert_equal(self.nodes[1].getbalance(), balance1) diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index f3c6af3edd544..2b8f7af43767f 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -27,7 +27,7 @@ def setup_network(self): connect_nodes(self.nodes[0], 1) connect_nodes(self.nodes[1], 2) connect_nodes(self.nodes[0], 2) - self.sync_all([self.nodes[0:3]]) + self.sync_all(self.nodes[0:3]) def get_vsize(self, txn): return self.nodes[0].decoderawtransaction(txn)['size'] @@ -46,9 +46,9 @@ def run_test(self): assert_equal(walletinfo['immature_balance'], 250) assert_equal(walletinfo['balance'], 0) - self.sync_all([self.nodes[0:3]]) + self.sync_all(self.nodes[0:3]) self.nodes[1].generate(101) - self.sync_all([self.nodes[0:3]]) + self.sync_all(self.nodes[0:3]) assert_equal(self.nodes[0].getbalance(), 250) assert_equal(self.nodes[1].getbalance(), 250) @@ -75,7 +75,7 @@ def run_test(self): # Send 21 PIV from 1 to 0 using sendtoaddress call. self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 21) self.nodes[1].generate(1) - self.sync_all([self.nodes[0:3]]) + self.sync_all(self.nodes[0:3]) # Node0 should have two unspent outputs. # Create a couple of transactions to send them to node2, submit them through @@ -100,7 +100,7 @@ def run_test(self): # Have node1 mine a block to confirm transactions: self.nodes[1].generate(1) - self.sync_all([self.nodes[0:3]]) + self.sync_all(self.nodes[0:3]) assert_equal(self.nodes[0].getbalance(), 0) node_2_expected_bal = Decimal('250') + Decimal('21') - 2 * fee_per_kbyte @@ -115,7 +115,7 @@ def run_test(self): node_2_bal -= (Decimal('10') - fee) assert_equal(self.nodes[2].getbalance(), node_2_bal) self.nodes[2].generate(1) - self.sync_all([self.nodes[0:3]]) + self.sync_all(self.nodes[0:3]) node_0_bal = self.nodes[0].getbalance() assert_equal(node_0_bal, Decimal('10')) @@ -123,7 +123,7 @@ def run_test(self): txid = self.nodes[2].sendmany('', {address: 10}, 0, "") fee = self.nodes[2].gettransaction(txid)["fee"] self.nodes[2].generate(1) - self.sync_all([self.nodes[0:3]]) + self.sync_all(self.nodes[0:3]) node_0_bal += Decimal('10') node_2_bal -= (Decimal('10') - fee) assert_equal(self.nodes[2].getbalance(), node_2_bal) @@ -138,7 +138,7 @@ def run_test(self): address_to_import = self.nodes[2].getnewaddress() self.nodes[0].sendtoaddress(address_to_import, 1) self.nodes[0].generate(1) - self.sync_all([self.nodes[0:3]]) + self.sync_all(self.nodes[0:3]) # 2. Import address from node2 to node1 self.nodes[1].importaddress(address_to_import) @@ -162,9 +162,9 @@ def run_test(self): {"spendable": True}) # check if wallet or blochchain maintenance changes the balance - self.sync_all([self.nodes[0:3]]) + self.sync_all(self.nodes[0:3]) blocks = self.nodes[0].generate(2) - self.sync_all([self.nodes[0:3]]) + self.sync_all(self.nodes[0:3]) balance_nodes = [self.nodes[i].getbalance() for i in range(3)] block_count = self.nodes[0].getblockcount() diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index 2db13b27b7ea1..32f26a9793d0a 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -61,7 +61,7 @@ def run_test(self): self.log.info("Running tests") dest_address = peer_node.getnewaddress() - test_simple_bumpfee_succeeds(rbf_node, peer_node, dest_address) + test_simple_bumpfee_succeeds(self, rbf_node, peer_node, dest_address) test_segwit_bumpfee_succeeds(rbf_node, dest_address) test_nonrbf_bumpfee_fails(peer_node, dest_address) test_notmine_bumpfee_fails(rbf_node, peer_node, dest_address) @@ -77,16 +77,16 @@ def run_test(self): self.log.info("Success") -def test_simple_bumpfee_succeeds(rbf_node, peer_node, dest_address): +def test_simple_bumpfee_succeeds(test, rbf_node, peer_node, dest_address): rbfid = spend_one_input(rbf_node, dest_address) rbftx = rbf_node.gettransaction(rbfid) - sync_mempools((rbf_node, peer_node)) + test.sync_mempools((rbf_node, peer_node)) assert rbfid in rbf_node.getrawmempool() and rbfid in peer_node.getrawmempool() bumped_tx = rbf_node.bumpfee(rbfid) assert_equal(bumped_tx["errors"], []) assert bumped_tx["fee"] - abs(rbftx["fee"]) > 0 # check that bumped_tx propagates, original tx was evicted and has a wallet conflict - sync_mempools((rbf_node, peer_node)) + test.sync_mempools((rbf_node, peer_node)) assert bumped_tx["txid"] in rbf_node.getrawmempool() assert bumped_tx["txid"] in peer_node.getrawmempool() assert rbfid not in rbf_node.getrawmempool() diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py index 3d56c223007ed..86c8bd7544c4a 100755 --- a/test/functional/wallet_import_rescan.py +++ b/test/functional/wallet_import_rescan.py @@ -20,7 +20,7 @@ """ from test_framework.test_framework import PivxTestFramework -from test_framework.util import (assert_raises_rpc_error, connect_nodes, sync_blocks, assert_equal, set_node_times) +from test_framework.util import (assert_raises_rpc_error, connect_nodes, assert_equal, set_node_times) import collections import enum @@ -137,7 +137,7 @@ def run_test(self): timestamp = self.nodes[0].getblockheader(self.nodes[0].getbestblockhash())["time"] set_node_times(self.nodes, timestamp + TIMESTAMP_WINDOW + 1) self.nodes[0].generate(1) - sync_blocks(self.nodes) + self.sync_blocks() # For each variation of wallet key import, invoke the import RPC and # check the results from getbalance and listtransactions. @@ -163,7 +163,7 @@ def run_test(self): # Generate a block containing the new transactions. self.nodes[0].generate(1) assert_equal(self.nodes[0].getrawmempool(), []) - sync_blocks(self.nodes) + self.sync_blocks() # Check the latest results from getbalance and listtransactions. for variant in IMPORT_VARIANTS: diff --git a/test/functional/wallet_import_stakingaddress.py b/test/functional/wallet_import_stakingaddress.py index 23040c4c52228..c768ace002350 100755 --- a/test/functional/wallet_import_stakingaddress.py +++ b/test/functional/wallet_import_stakingaddress.py @@ -15,7 +15,6 @@ from test_framework.util import ( assert_equal, DecimalAmt, - sync_blocks, ) class ImportStakingTest(PivxTestFramework): @@ -45,7 +44,7 @@ def run_test(self): # mine a block and check staking balance self.nodes[0].generate(1) assert_equal(self.nodes[0].getdelegatedbalance(), DecimalAmt(10 * (i+1))) - sync_blocks(self.nodes) + self.sync_blocks() # Export keys self.log.info("Exporting keys and importing in node 1") diff --git a/test/functional/wallet_keypool_topup.py b/test/functional/wallet_keypool_topup.py index 797e8a3cc9aea..f634538a9a406 100755 --- a/test/functional/wallet_keypool_topup.py +++ b/test/functional/wallet_keypool_topup.py @@ -16,7 +16,6 @@ from test_framework.util import ( assert_equal, connect_nodes, - sync_blocks, ) class KeypoolRestoreTest(PivxTestFramework): @@ -51,7 +50,7 @@ def run_test(self): self.nodes[0].generate(1) self.nodes[0].sendtoaddress(addr_extpool, 5) self.nodes[0].generate(1) - sync_blocks(self.nodes) + self.sync_blocks() self.log.info("Restart node with wallet backup") diff --git a/test/functional/wallet_listreceivedby.py b/test/functional/wallet_listreceivedby.py index 9134bde56dd8a..0cfeabd3e9634 100755 --- a/test/functional/wallet_listreceivedby.py +++ b/test/functional/wallet_listreceivedby.py @@ -10,7 +10,6 @@ assert_array_result, assert_equal, assert_raises_rpc_error, - sync_blocks, ) @@ -21,7 +20,7 @@ def set_test_params(self): def run_test(self): # Generate block to get out of IBD self.nodes[0].generate(1) - sync_blocks(self.nodes) + self.sync_blocks() self.log.info("listreceivedbyaddress Test") diff --git a/test/functional/wallet_listsinceblock.py b/test/functional/wallet_listsinceblock.py index 40f9c02c02fe7..a3b95f3be2e02 100755 --- a/test/functional/wallet_listsinceblock.py +++ b/test/functional/wallet_listsinceblock.py @@ -99,7 +99,8 @@ def test_reorg(self): self.nodes[2].generate(7) self.log.info('lastblockhash=%s' % (lastblockhash)) - self.sync_all([self.nodes[:2], self.nodes[2:]]) + self.sync_all(self.nodes[:2]) + self.sync_all(self.nodes[2:]) self.join_network() diff --git a/test/functional/wallet_listtransactions.py b/test/functional/wallet_listtransactions.py index 485cab08e843d..20222cc4b968b 100755 --- a/test/functional/wallet_listtransactions.py +++ b/test/functional/wallet_listtransactions.py @@ -6,14 +6,11 @@ from decimal import Decimal from io import BytesIO -from test_framework.mininode import CTransaction, COIN +from test_framework.mininode import CTransaction from test_framework.test_framework import PivxTestFramework from test_framework.util import ( assert_array_result, - assert_equal, - bytes_to_hex_str, hex_str_to_bytes, - sync_mempools, ) def txFromHex(hexstring): diff --git a/test/functional/wallet_reorgsrestore.py b/test/functional/wallet_reorgsrestore.py index 6d254c784bb2a..a2d5ba2bc57de 100755 --- a/test/functional/wallet_reorgsrestore.py +++ b/test/functional/wallet_reorgsrestore.py @@ -23,7 +23,6 @@ assert_equal, connect_nodes, disconnect_nodes, - sync_blocks, ) class ReorgsRestoreTest(PivxTestFramework): @@ -37,7 +36,7 @@ def run_test(self): # Send a tx from which to conflict outputs later txid_conflict_from = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("10")) self.nodes[0].generate(1) - sync_blocks(self.nodes) + self.sync_blocks() # Disconnect node1 from others to reorg its chain later disconnect_nodes(self.nodes[0], 1) @@ -72,7 +71,7 @@ def run_test(self): # Reconnect node0 and node2 and check that conflicted_txid is effectively conflicted connect_nodes(self.nodes[0], 2) - sync_blocks([self.nodes[0], self.nodes[2]]) + self.sync_blocks([self.nodes[0], self.nodes[2]]) conflicted = self.nodes[0].gettransaction(conflicted_txid) conflicting = self.nodes[0].gettransaction(conflicting_txid) assert_equal(conflicted["confirmations"], -9) diff --git a/test/functional/wallet_txn_clone.py b/test/functional/wallet_txn_clone.py index 2d63fae78033d..4fcbb0e13a443 100755 --- a/test/functional/wallet_txn_clone.py +++ b/test/functional/wallet_txn_clone.py @@ -79,7 +79,7 @@ def run_test(self): # Have node0 mine a block, if requested: if (self.options.mine_block): self.nodes[0].generate(1) - sync_blocks(self.nodes[0:2]) + self.sync_blocks(self.nodes[0:2]) tx1 = self.nodes[0].gettransaction(txid1) tx2 = self.nodes[0].gettransaction(txid2) @@ -114,7 +114,7 @@ def run_test(self): self.nodes[2].sendrawtransaction(node0_tx2["hex"]) self.nodes[2].sendrawtransaction(tx2["hex"]) self.nodes[2].generate(1) # Mine another block to make sure we sync - sync_blocks(self.nodes) + self.sync_blocks() # Re-fetch transaction info: tx1 = self.nodes[0].gettransaction(txid1) diff --git a/test/functional/wallet_txn_doublespend.py b/test/functional/wallet_txn_doublespend.py index 93559581ab721..4ea0994a17ace 100755 --- a/test/functional/wallet_txn_doublespend.py +++ b/test/functional/wallet_txn_doublespend.py @@ -69,7 +69,7 @@ def run_test(self): # Have node0 mine a block: if (self.options.mine_block): self.nodes[0].generate(1) - sync_blocks(self.nodes[0:2]) + self.sync_blocks(self.nodes[0:2]) tx1 = self.nodes[0].gettransaction(txid1) tx2 = self.nodes[0].gettransaction(txid2) @@ -105,7 +105,7 @@ def run_test(self): connect_nodes(self.nodes[2], 0) connect_nodes(self.nodes[2], 1) self.nodes[2].generate(1) # Mine another block to make sure we sync - sync_blocks(self.nodes) + self.sync_blocks() assert_equal(self.nodes[0].gettransaction(doublespend_txid)["confirmations"], 2) # Re-fetch transaction info: diff --git a/test/functional/wallet_zapwallettxes.py b/test/functional/wallet_zapwallettxes.py index 19afe606810fd..4b5f085ca709d 100755 --- a/test/functional/wallet_zapwallettxes.py +++ b/test/functional/wallet_zapwallettxes.py @@ -18,7 +18,6 @@ from test_framework.util import ( assert_equal, assert_raises_rpc_error, - sync_mempools, ) class ZapWalletTXesTest (PivxTestFramework): @@ -44,7 +43,7 @@ def run_test(self): # This transaction will not be confirmed txid2 = self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 20) - sync_mempools(self.nodes, wait=.1) + self.sync_mempools(wait=.1) # Confirmed and unconfirmed transactions are now in the wallet. assert_equal(self.nodes[0].gettransaction(txid1)['txid'], txid1) From a582560da3dc0141f0fdf642d840735ec1117664 Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 5 Jul 2019 09:07:00 +0800 Subject: [PATCH 08/31] docs: add reduce-memory.md Co-Authored-By: Wladimir J. van der Laan Plus adapted it to PIVX. Github-Pull: #2262 Rebased-From: caa67344d0824be17224556cfd238fb7b1eab45f --- doc/README.md | 1 + doc/reduce-memory.md | 45 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 doc/reduce-memory.md diff --git a/doc/README.md b/doc/README.md index 9d607ca762b46..041aa96785bfb 100644 --- a/doc/README.md +++ b/doc/README.md @@ -64,6 +64,7 @@ The PIVX repo's [root README](/README.md) contains relevant information on the d ### Miscellaneous - [Assets Attribution](assets-attribution.md) - [Files](files.md) +- [Reduce Memory](reduce-memory.md) - [Tor Support](tor.md) - [Init Scripts (systemd/upstart/openrc)](init.md) diff --git a/doc/reduce-memory.md b/doc/reduce-memory.md new file mode 100644 index 0000000000000..dd137aef3cdab --- /dev/null +++ b/doc/reduce-memory.md @@ -0,0 +1,45 @@ +# Reduce Memory + +There are a few parameters that can be dialed down to reduce the memory usage of `pivxd`. This can be useful on embedded systems or small VPSes. + +## In-memory caches + +The size of some in-memory caches can be reduced. As caches trade off memory usage for performance, reducing these will usually have a negative effect on performance. + +- `-dbcache=` - the UTXO database cache size, this defaults to `450`. The unit is MiB (1024). + - The minimum value for `-dbcache` is 4. + - A lower `-dbcache` makes initial sync time much longer. After the initial sync, the effect is less pronounced for most use-cases, unless fast validation of blocks is important, such as for mining. + +## Memory pool + +- In PIVX Core there is a memory pool limiter which can be configured with `-maxmempool=`, where `` is the size in MB (1000). The default value is `300`. + - The minimum value for `-maxmempool` is 5. + - A lower maximum mempool size means that transactions will be evicted sooner. This will affect any uses of `pivxd` that process unconfirmed transactions. + +- Unused memory allocated to the mempool (default: 300MB) is shared with the UTXO cache, so when trying to reduce memory usage you should limit the mempool, with the `-maxmempool` command line argument. + +## Number of peers + +- `-maxconnections=` - the maximum number of connections, this defaults to `125`. Each active connection takes up some memory. Only significant if incoming + connections are enabled, otherwise the number of connections will never be more than `8`. + +## Thread configuration + +For each thread a thread stack needs to be allocated. By default on Linux, +threads take up 8MiB for the thread stack on a 64-bit system, and 4MiB in a +32-bit system. + +- `-par=` - the number of script verification threads, defaults to the number of cores in the system minus one. +- `-rpcthreads=` - the number of threads used for processing RPC requests, defaults to `4`. + +## Linux specific + +By default, since glibc `2.10`, the C library will create up to two heap arenas per core. This is known to cause excessive memory usage in some scenarios. To avoid this make a script that sets `MALLOC_ARENA_MAX` before starting pivxd: + +```bash +#!/usr/bin/env bash +export MALLOC_ARENA_MAX=1 +pivxd +``` + +The behavior was introduced to increase CPU locality of allocated memory and performance with concurrent allocation, so this setting could in theory reduce performance. However, in PIVX Core very little parallel allocation happens, so the impact is expected to be small or absent. From c865148782744fd8e795d80235f2bf2521f8c0df Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 1 Apr 2021 12:50:07 -0300 Subject: [PATCH 09/31] [validation] Do not actively wait for cs_main lock in `ActivateBestChain()` Github-Pull: #2284 Rebased-From: e560afd2d822e0ab685d8e1ae1987ef0997f6eaa --- src/validation.cpp | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 0f9be3ec1da21..e99d8355fa3a6 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2241,12 +2241,8 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb const CBlockIndex *pindexFork; bool fInitialDownload; - while (true) { - TRY_LOCK(cs_main, lockMain); - if (!lockMain) { - MilliSleep(50); - continue; - } + { + LOCK(cs_main); ConnectTrace connectTrace(mempool); // Destructed before cs_main is unlocked CBlockIndex *pindexOldTip = chainActive.Tip(); @@ -2268,8 +2264,6 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb assert(trace.pblock && trace.pindex); GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs); } - - break; } // Notify external listeners about the new tip. From 4903f310c26c9e3388dfdcef845a7c8c0932b4de Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 1 Apr 2021 13:43:38 -0300 Subject: [PATCH 10/31] Removing blocksizenotify abomination. Essentially, we were re-serializing every new connected block tip just to calculate the size and broadcast a notification for external listeners if the block size is above 1mb. Which well.. (1) the block arrives serialized from the network and it's parsed into a block object by the core, there is no need to re-serialize the entire block.. (2) the complete serialization is just done to notify external listeners if a block exceeds the 1mb block size.. --> which can easily be done from outside of the core without having the most important node processing thread's work notifying nor calculating it, blocking cs_main for its whole time.. Github-Pull: #2284 Rebased-From: 479d065715ab03d19ef2c5ec75951146c2d3933b --- src/guiinterface.h | 3 --- src/init.cpp | 14 -------------- src/validation.cpp | 9 --------- 3 files changed, 26 deletions(-) diff --git a/src/guiinterface.h b/src/guiinterface.h index 03307315948e9..2d20536810285 100644 --- a/src/guiinterface.h +++ b/src/guiinterface.h @@ -100,9 +100,6 @@ class CClientUIInterface /** New block has been accepted */ boost::signals2::signal NotifyBlockTip; - /** New block has been accepted and is over a certain size */ - boost::signals2::signal NotifyBlockSize; - /** Banlist did change. */ boost::signals2::signal BannedListChanged; }; diff --git a/src/init.cpp b/src/init.cpp index ee7785a9c809c..d7b4cbbcf53ac 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -422,7 +422,6 @@ std::string HelpMessage(HelpMessageMode mode) strUsage += HelpMessageOpt("-version", _("Print version and exit")); strUsage += HelpMessageOpt("-alertnotify=", _("Execute command when a relevant alert is received or we see a really long fork (%s in cmd is replaced by message)")); strUsage += HelpMessageOpt("-blocknotify=", _("Execute command when the best block changes (%s in cmd is replaced by block hash)")); - strUsage += HelpMessageOpt("-blocksizenotify=", _("Execute command when the best block changes and its size is over (%s in cmd is replaced by block hash, %d with the block size)")); strUsage += HelpMessageOpt("-checkblocks=", strprintf(_("How many blocks to check at startup (default: %u, 0 = all)"), DEFAULT_CHECKBLOCKS)); strUsage += HelpMessageOpt("-conf=", strprintf(_("Specify configuration file (default: %s)"), PIVX_CONF_FILENAME)); if (mode == HMM_BITCOIND) { @@ -622,16 +621,6 @@ static void BlockNotifyCallback(bool initialSync, const CBlockIndex *pBlockIndex } } -static void BlockSizeNotifyCallback(int size, const uint256& hashNewTip) -{ - std::string strCmd = gArgs.GetArg("-blocksizenotify", ""); - - boost::replace_all(strCmd, "%s", hashNewTip.GetHex()); - boost::replace_all(strCmd, "%d", std::to_string(size)); - std::thread t(runCommand, strCmd); - t.detach(); // thread runs free -} - //////////////////////////////////////////////////// static bool fHaveGenesis = false; @@ -1786,9 +1775,6 @@ bool AppInitMain() if (gArgs.IsArgSet("-blocknotify")) uiInterface.NotifyBlockTip.connect(BlockNotifyCallback); - if (gArgs.IsArgSet("-blocksizenotify")) - uiInterface.NotifyBlockSize.connect(BlockSizeNotifyCallback); - // scan for better chains in the block chain database, that are not yet connected in the active best chain CValidationState state; if (!ActivateBestChain(state)) { diff --git a/src/validation.cpp b/src/validation.cpp index e99d8355fa3a6..c9e6380fa884a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2271,17 +2271,8 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb // Always notify the UI if a new block tip was connected if (pindexFork != pindexNewTip) { - // Notify the UI uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip); - - unsigned size = 0; - if (pblock) - size = GetSerializeSize(*pblock, SER_NETWORK, PROTOCOL_VERSION); - // If the size is over 1 MB notify external listeners, and it is within the last 5 minutes - if (size > MAX_BLOCK_SIZE_LEGACY && pblock->GetBlockTime() > GetAdjustedTime() - 300) { - uiInterface.NotifyBlockSize(static_cast(size), pindexNewTip->GetBlockHash()); - } } } while (pindexMostWork != chainActive.Tip()); From 9a9425f893231e16c1d4ca0670790b72cd50ee2b Mon Sep 17 00:00:00 2001 From: Jesse Cohen Date: Tue, 17 Apr 2018 17:05:08 -0400 Subject: [PATCH 11/31] Add Unit Test for SingleThreadedSchedulerClient Ensures ordering of callbacks within a SingleThreadedSchedulerClient with respect to each other Github-Pull: #2290 Rebased-From: 4ea2048180b85dbf80b8c89e0cbd700eff4c1170 --- src/test/scheduler_tests.cpp | 44 +++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/src/test/scheduler_tests.cpp b/src/test/scheduler_tests.cpp index ce46cbf5887ff..3f3b0a660718f 100644 --- a/src/test/scheduler_tests.cpp +++ b/src/test/scheduler_tests.cpp @@ -61,7 +61,7 @@ BOOST_AUTO_TEST_CASE(manythreads) size_t nTasks = microTasks.getQueueInfo(first, last); BOOST_CHECK(nTasks == 0); - for (int i = 0; i < 100; i++) { + for (int i = 0; i < 100; ++i) { boost::chrono::system_clock::time_point t = now + boost::chrono::microseconds(randomMsec(rng)); boost::chrono::system_clock::time_point tReschedule = now + boost::chrono::microseconds(500 + randomMsec(rng)); int whichCounter = zeroToNine(rng); @@ -108,4 +108,46 @@ BOOST_AUTO_TEST_CASE(manythreads) BOOST_CHECK_EQUAL(counterSum, 200); } +BOOST_AUTO_TEST_CASE(singlethreadedscheduler_ordered) +{ + CScheduler scheduler; + + // each queue should be well ordered with respect to itself but not other queues + SingleThreadedSchedulerClient queue1(&scheduler); + SingleThreadedSchedulerClient queue2(&scheduler); + + // create more threads than queues + // if the queues only permit execution of one task at once then + // the extra threads should effectively be doing nothing + // if they don't we'll get out of order behaviour + boost::thread_group threads; + for (int i = 0; i < 5; ++i) { + threads.create_thread(boost::bind(&CScheduler::serviceQueue, &scheduler)); + } + + // these are not atomic, if SinglethreadedSchedulerClient prevents + // parallel execution at the queue level no synchronization should be required here + int counter1 = 0; + int counter2 = 0; + + // just simply count up on each queue - if execution is properly ordered then + // the callbacks should run in exactly the order in which they were enqueued + for (int i = 0; i < 100; ++i) { + queue1.AddToProcessQueue([i, &counter1]() { + BOOST_CHECK_EQUAL(i, counter1++); + }); + + queue2.AddToProcessQueue([i, &counter2]() { + BOOST_CHECK_EQUAL(i, counter2++); + }); + } + + // finish up + scheduler.stop(true); + threads.join_all(); + + BOOST_CHECK_EQUAL(counter1, 100); + BOOST_CHECK_EQUAL(counter2, 100); +} + BOOST_AUTO_TEST_SUITE_END() From f499e6e6040f130a2872bf9ec783f8fff7a0b926 Mon Sep 17 00:00:00 2001 From: Jesse Cohen Date: Wed, 16 May 2018 11:10:31 -0400 Subject: [PATCH 12/31] Update documentation for SingleThreadedSchedulerClient() to specify the memory model Github-Pull: #2290 Rebased-From: cb9bb251ea288d2ec29ff965d13dd2747142b9fd --- src/scheduler.h | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/scheduler.h b/src/scheduler.h index 781bd52ea56fc..6661c032c82f9 100644 --- a/src/scheduler.h +++ b/src/scheduler.h @@ -87,9 +87,13 @@ class CScheduler /** * Class used by CScheduler clients which may schedule multiple jobs - * which are required to be run serially. Does not require such jobs - * to be executed on the same thread, but no two jobs will be executed - * at the same time. + * which are required to be run serially. Jobs may not be run on the + * same thread, but no two jobs will be executed + * at the same time and memory will be release-acquire consistent + * (the scheduler will internally do an acquire before invoking a callback + * as well as a release at the end). In practice this means that a callback + * B() will be able to observe all of the effects of callback A() which executed + * before it. */ class SingleThreadedSchedulerClient { private: @@ -104,6 +108,13 @@ class SingleThreadedSchedulerClient { public: explicit SingleThreadedSchedulerClient(CScheduler *pschedulerIn) : m_pscheduler(pschedulerIn) {} + + /** + * Add a callback to be executed. Callbacks are executed serially + * and memory is release-acquire consistent between callback executions. + * Practially, this means that callbacks can behave as if they are executed + * in order by a single thread. + */ void AddToProcessQueue(std::function func); // Processes all remaining queue members on the calling thread, blocking until queue is empty From c202bc19d4f7285bbbd69e673a2ef009f2ecbc8b Mon Sep 17 00:00:00 2001 From: Jesse Cohen Date: Wed, 16 May 2018 16:20:37 -0400 Subject: [PATCH 13/31] Update ValidationInterface() documentation to explicitly specify threading and memory model Github-Pull: #2290 Rebased-From: f9d2ab3f3f0cd471f7f0a13bb15ffabc6f129d87 --- src/validationinterface.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/validationinterface.h b/src/validationinterface.h index 27bfd9d4ce173..008ec204b8f6b 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -54,6 +54,21 @@ void CallFunctionInValidationInterfaceQueue(std::function func); */ void SyncWithValidationInterfaceQueue(); +/** + * Implement this to subscribe to events generated in validation + * + * Each CValidationInterface() subscriber will receive event callbacks + * in the order in which the events were generated by validation. + * Furthermore, each ValidationInterface() subscriber may assume that + * callbacks effectively run in a single thread with single-threaded + * memory consistency. That is, for a given ValidationInterface() + * instantiation, each callback will complete before the next one is + * invoked. This means, for example when a block is connected that the + * UpdatedBlockTip() callback may depend on an operation performed in + * the BlockConnected() callback without worrying about explicit + * synchronization. No ordering should be assumed across + * ValidationInterface() subscribers. + */ class CValidationInterface { public: virtual ~CValidationInterface() = default; From 7ab7112c1f2efa55d4a4ecd7facb7a49881cbed6 Mon Sep 17 00:00:00 2001 From: Jesse Cohen Date: Sun, 15 Apr 2018 11:22:28 -0400 Subject: [PATCH 14/31] Hold cs_main while calling UpdatedBlockTip() and ui.NotifyBlockTip Ensures that callbacks are invoked in the order in which the chain is updated Resolves #12978 Github-Pull: #2290 Rebased-From: ef24337204712d08c932363bce9e1e32d9ee5e51 --- src/validation.cpp | 15 ++++++++------- src/validationinterface.cpp | 4 ++++ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index c9e6380fa884a..fb5889f2c63b9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2264,16 +2264,17 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb assert(trace.pblock && trace.pindex); GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs); } - } - // Notify external listeners about the new tip. - GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload); + // Notify external listeners about the new tip. + // Enqueue while holding cs_main to ensure that UpdatedBlockTip is called in the order in which blocks are connected + GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload); - // Always notify the UI if a new block tip was connected - if (pindexFork != pindexNewTip) { - // Notify the UI - uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip); + // Always notify the UI if a new block tip was connected + if (pindexFork != pindexNewTip) { + uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip); + } } + // When we reach this point, we switched to a new tip (stored in pindexNewTip). } while (pindexMostWork != chainActive.Tip()); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index ab2b92527642d..76a5ff27be390 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -146,6 +146,10 @@ void CMainSignals::MempoolEntryRemoved(CTransactionRef ptx, MemPoolRemovalReason } void CMainSignals::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload) { + // Dependencies exist that require UpdatedBlockTip events to be delivered in the order in which + // the chain actually updates. One way to ensure this is for the caller to invoke this signal + // in the same critical section where the chain is updated + m_internals->m_schedulerClient.AddToProcessQueue([pindexNew, pindexFork, fInitialDownload, this] { m_internals->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); }); From c2ae5ffc6d9c6019f371ac4696c85ac5616f88e7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 6 Feb 2018 13:38:54 -0500 Subject: [PATCH 15/31] Fix fast-shutdown crash if genesis block was not loaded If the ShutdownRequested() check at the top of ActivateBestChain() returns false during initial genesis block load we will fail an assertion in UTXO DB flush as the best block hash IsNull(). To work around this, we move the check until after one round of ActivateBestChainStep(), ensuring the genesis block gets connected. Github-Pull: #2290 Rebased-From: 8640be1088b6307898af26c40e987028ee109827 --- src/validation.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index fb5889f2c63b9..ef9a3e728d69c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2276,6 +2276,12 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb } // When we reach this point, we switched to a new tip (stored in pindexNewTip). + // We check shutdown only after giving ActivateBestChainStep a chance to run once so that we + // never shutdown before connecting the genesis block during LoadChainTip(). Previously this + // caused an assert() failure during shutdown in such cases as the UTXO DB flushing checks + // that the best block hash is non-null. + if (ShutdownRequested()) + break; } while (pindexMostWork != chainActive.Tip()); CheckBlockIndex(); From fd79bb753080ca98b368a9c0437c477b1f7477b3 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 1 Apr 2021 17:46:22 -0300 Subject: [PATCH 16/31] Optimize ActivateBestChain for long chains Github-Pull: #2290 Rebased-From: 8d15cf58ca0fd3f97fd6bcaf671b2b7e9f62caa9 --- src/validation.cpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index ef9a3e728d69c..3ec2b5773ccc2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2138,12 +2138,11 @@ static void PruneBlockIndexCandidates() * Try to make some progress towards making pindexMostWork the active block. * pblock is either NULL or a pointer to a CBlock corresponding to pindexMostWork. */ -static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr& pblock, bool fAlreadyChecked, ConnectTrace& connectTrace) +static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr& pblock, bool fAlreadyChecked, bool& fInvalidFound, ConnectTrace& connectTrace) { AssertLockHeld(cs_main); if (pblock == NULL) fAlreadyChecked = false; - bool fInvalidFound = false; const CBlockIndex* pindexOldTip = chainActive.Tip(); const CBlockIndex* pindexFork = chainActive.FindFork(pindexMostWork); @@ -2246,16 +2245,23 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb ConnectTrace connectTrace(mempool); // Destructed before cs_main is unlocked CBlockIndex *pindexOldTip = chainActive.Tip(); - pindexMostWork = FindMostWorkChain(); + if (pindexMostWork == nullptr) { + pindexMostWork = FindMostWorkChain(); + } // Whether we have anything to do at all. - if (pindexMostWork == NULL || pindexMostWork == chainActive.Tip()) + if (pindexMostWork == nullptr || pindexMostWork == chainActive.Tip()) return true; + bool fInvalidFound = false; std::shared_ptr nullBlockPtr; - if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fAlreadyChecked, connectTrace)) + if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fAlreadyChecked, fInvalidFound, connectTrace)) return false; + if (fInvalidFound) { + // Wipe cache, we may need another branch now. + pindexMostWork = nullptr; + } pindexNewTip = chainActive.Tip(); pindexFork = chainActive.FindFork(pindexOldTip); fInitialDownload = IsInitialBlockDownload(); From fb19c3aacee03a8c03916402f94570380141536f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 9 Oct 2017 11:19:10 -0400 Subject: [PATCH 17/31] Do not unlock cs_main in ABC unless we've actually made progress. Technically, some internal datastructures may be in an inconsistent state if we do this, though there are no known bugs there. Still, for future safety, its much better to only unlock cs_main if we've made progress (not just tried a reorg which may make progress). Github-Pull: #2290 Rebased-From: 198f43516a8581ce16843b0bf8c5675fc75088d6 --- src/validation.cpp | 64 ++++++++++++++++++++++----------------- src/validationinterface.h | 6 +++- 2 files changed, 41 insertions(+), 29 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 3ec2b5773ccc2..8bc8b7d96f6cf 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2238,45 +2238,53 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb SyncWithValidationInterfaceQueue(); } - const CBlockIndex *pindexFork; - bool fInitialDownload; { LOCK(cs_main); - ConnectTrace connectTrace(mempool); // Destructed before cs_main is unlocked + CBlockIndex* starting_tip = chainActive.Tip(); + bool blocks_connected = false; + do { + // We absolutely may not unlock cs_main until we've made forward progress + // (with the exception of shutdown due to hardware issues, low disk space, etc). + ConnectTrace connectTrace(mempool); // Destructed before cs_main is unlocked + + if (pindexMostWork == nullptr) { + pindexMostWork = FindMostWorkChain(); + } - CBlockIndex *pindexOldTip = chainActive.Tip(); - if (pindexMostWork == nullptr) { - pindexMostWork = FindMostWorkChain(); - } + // Whether we have anything to do at all. + if (pindexMostWork == nullptr || pindexMostWork == chainActive.Tip()) { + break; + } - // Whether we have anything to do at all. - if (pindexMostWork == nullptr || pindexMostWork == chainActive.Tip()) - return true; + bool fInvalidFound = false; + std::shared_ptr nullBlockPtr; + if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fAlreadyChecked, fInvalidFound, connectTrace)) + return false; + blocks_connected = true; - bool fInvalidFound = false; - std::shared_ptr nullBlockPtr; - if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fAlreadyChecked, fInvalidFound, connectTrace)) - return false; + if (fInvalidFound) { + // Wipe cache, we may need another branch now. + pindexMostWork = nullptr; + } + pindexNewTip = chainActive.Tip(); - if (fInvalidFound) { - // Wipe cache, we may need another branch now. - pindexMostWork = nullptr; - } - pindexNewTip = chainActive.Tip(); - pindexFork = chainActive.FindFork(pindexOldTip); - fInitialDownload = IsInitialBlockDownload(); + for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) { + assert(trace.pblock && trace.pindex); + GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs); + } + } while (!chainActive.Tip() || (starting_tip && CBlockIndexWorkComparator()(chainActive.Tip(), starting_tip))); + if (!blocks_connected) return true; - for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) { - assert(trace.pblock && trace.pindex); - GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs); - } + const CBlockIndex* pindexFork = chainActive.FindFork(starting_tip); + bool fInitialDownload = IsInitialBlockDownload(); // Notify external listeners about the new tip. // Enqueue while holding cs_main to ensure that UpdatedBlockTip is called in the order in which blocks are connected - GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload); - - // Always notify the UI if a new block tip was connected if (pindexFork != pindexNewTip) { + // Notify ValidationInterface subscribers + GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload); + + // Always notify the UI if a new block tip was connected uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip); } } diff --git a/src/validationinterface.h b/src/validationinterface.h index 008ec204b8f6b..91ec2ee5b46c6 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -74,7 +74,11 @@ class CValidationInterface { virtual ~CValidationInterface() = default; protected: /** - * Notifies listeners of updated block chain tip + * Notifies listeners when the block chain tip advances. + * + * When multiple blocks are connected at once, UpdatedBlockTip will be called on the final tip + * but may not be called on every intermediate tip. If the latter behavior is desired, + * subscribe to BlockConnected() instead. * * Called on a background thread. */ From 39f6eaf696fbff638769bab2bbb0784c2d6ac3a8 Mon Sep 17 00:00:00 2001 From: Jesse Cohen Date: Thu, 1 Apr 2021 18:05:10 -0300 Subject: [PATCH 18/31] Fix concurrency-related bugs in ActivateBestChain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If multiple threads are invoking ActivateBestChain, it was possible to have them working towards different tips, and we could arrive at a less work tip than we should.  Fix this by introducing a ChainState lock which must be held for the entire duration of ActivateBestChain to enforce exclusion in ABC. Github-Pull: #2290 Rebased-From: a51a75529ad2dfcb1b47a346ba7656baa367bcfb --- src/validation.cpp | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 8bc8b7d96f6cf..417abbbf70189 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -147,11 +147,17 @@ struct CBlockIndexWorkComparator { CBlockIndex* pindexBestInvalid; /** - * The set of all CBlockIndex entries with BLOCK_VALID_TRANSACTIONS (for itself and all ancestors) and - * as good as our current tip or better. Entries may be failed, though. - */ + * The set of all CBlockIndex entries with BLOCK_VALID_TRANSACTIONS (for itself and all ancestors) and + * as good as our current tip or better. Entries may be failed, though. + */ std::set setBlockIndexCandidates; +/** + * the ChainState Mutex + * A lock that must be held when modifying this ChainState - held in ActivateBestChain() + */ +Mutex m_cs_chainstate; + /** All pairs A->B, where A (or one if its ancestors) misses transactions, but B has transactions. */ std::multimap mapBlocksUnlinked; @@ -2226,6 +2232,12 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb // sanely for performance or correctness! AssertLockNotHeld(cs_main); + // ABC maintains a fair degree of expensive-to-calculate internal state + // because this function periodically releases cs_main so that it does not lock up other threads for too long + // during large connects - and to allow for e.g. the callback queue to drain + // we use m_cs_chainstate to enforce mutual exclusion so that only one caller may execute this function at a time + LOCK(m_cs_chainstate); + CBlockIndex* pindexNewTip = nullptr; CBlockIndex* pindexMostWork = nullptr; do { From c4dd07ff547b799a453b84206b8aea58317b0bdf Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 2 Apr 2021 12:51:15 -0300 Subject: [PATCH 19/31] Validation: rename one of the two instances using "bad-prevblk" to its correct description of "prevblk-not-found" Github-Pull: #2290 Rebased-From: f68251d9316f319c84fb92d5d659bbd3d88607c0 --- src/validation.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 417abbbf70189..59ff2cc0d1272 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2949,8 +2949,10 @@ bool GetPrevIndex(const CBlock& block, CBlockIndex** pindexPrevRet, CValidationS pindexPrev = nullptr; if (block.GetHash() != Params().GetConsensus().hashGenesisBlock) { BlockMap::iterator mi = mapBlockIndex.find(block.hashPrevBlock); - if (mi == mapBlockIndex.end()) - return state.DoS(0, error("%s : prev block %s not found", __func__, block.hashPrevBlock.GetHex()), 0, "bad-prevblk"); + if (mi == mapBlockIndex.end()) { + return state.DoS(0, error("%s : prev block %s not found", __func__, block.hashPrevBlock.GetHex()), 0, + "prevblk-not-found"); + } pindexPrev = (*mi).second; if (pindexPrev->nStatus & BLOCK_FAILED_MASK) { //If this "invalid" block is an exact match from the checkpoints, then reconsider it From e51dcee74e173f929c1d8dcb1c3cd213c7d69a48 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 2 Apr 2021 12:49:49 -0300 Subject: [PATCH 20/31] Add unit tests for signals generated by ProcessNewBlock() After a recent bug discovered in callback ordering in MainSignals, this test checks invariants in ordering of BlockConnected / BlockDisconnected / UpdatedChainTip signals Adaptation of btc@dd435ad40267f5c50ff17533c696f9302829a6a6 Github-Pull: #2290 Rebased-From: 50dbec59277e25f3e24f447773c12b60f360427a --- src/Makefile.test.include | 3 +- src/blockassembler.h | 4 +- src/test/CMakeLists.txt | 1 + src/test/test_pivx.cpp | 6 + src/test/test_pivx.h | 3 + src/test/validation_block_tests.cpp | 178 ++++++++++++++++++++++++++++ 6 files changed, 192 insertions(+), 3 deletions(-) create mode 100644 src/test/validation_block_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 7f0c51f29148f..d4f6a77eb3466 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -101,7 +101,8 @@ BITCOIN_TESTS =\ test/univalue_tests.cpp \ test/util_tests.cpp \ test/sha256compress_tests.cpp \ - test/upgrades_tests.cpp + test/upgrades_tests.cpp \ + test/validation_block_tests.cpp SAPLING_TESTS =\ test/librust/libsapling_utils_tests.cpp \ diff --git a/src/blockassembler.h b/src/blockassembler.h index eaf183dec6a06..753b100482016 100644 --- a/src/blockassembler.h +++ b/src/blockassembler.h @@ -66,8 +66,8 @@ class BlockAssembler BlockAssembler(const CChainParams& chainparams, const bool defaultPrintPriority); /** Construct a new block template with coinbase to scriptPubKeyIn */ std::unique_ptr CreateNewBlock(const CScript& scriptPubKeyIn, - CWallet* pwallet, - bool fProofOfStake, + CWallet* pwallet = nullptr, + bool fProofOfStake = false, std::vector* availableCoins = nullptr); private: diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt index 98d670fbc5e89..b80d39141f2ac 100644 --- a/src/test/CMakeLists.txt +++ b/src/test/CMakeLists.txt @@ -133,6 +133,7 @@ set(BITCOIN_TESTS ${CMAKE_CURRENT_SOURCE_DIR}/validation_tests.cpp ${CMAKE_CURRENT_SOURCE_DIR}/sha256compress_tests.cpp ${CMAKE_CURRENT_SOURCE_DIR}/upgrades_tests.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/validation_block_tests.cpp ${CMAKE_CURRENT_SOURCE_DIR}/librust/sapling_rpc_wallet_tests.cpp ${CMAKE_SOURCE_DIR}/src/wallet/test/wallet_tests.cpp ${CMAKE_SOURCE_DIR}/src/wallet/test/crypto_tests.cpp diff --git a/src/test/test_pivx.cpp b/src/test/test_pivx.cpp index 2d4b2c3b5423d..adad49eb91921 100644 --- a/src/test/test_pivx.cpp +++ b/src/test/test_pivx.cpp @@ -32,6 +32,12 @@ FastRandomContext insecure_rand_ctx(insecure_rand_seed); extern bool fPrintToConsole; extern void noui_connect(); +std::ostream& operator<<(std::ostream& os, const uint256& num) +{ + os << num.ToString(); + return os; +} + BasicTestingSetup::BasicTestingSetup() { RandomInit(); diff --git a/src/test/test_pivx.h b/src/test/test_pivx.h index d22da8ecd1bd9..251c9ffb929df 100644 --- a/src/test/test_pivx.h +++ b/src/test/test_pivx.h @@ -117,4 +117,7 @@ struct TestMemPoolEntryHelper TestMemPoolEntryHelper &SigOps(unsigned int _sigops) { sigOpCount = _sigops; return *this; } }; +// define an implicit conversion here so that uint256 may be used directly in BOOST_CHECK_* +std::ostream& operator<<(std::ostream& os, const uint256& num); + #endif diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp new file mode 100644 index 0000000000000..dba3c04f1b63c --- /dev/null +++ b/src/test/validation_block_tests.cpp @@ -0,0 +1,178 @@ +// Copyright (c) 2018 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://www.opensource.org/licenses/mit-license.php. + +#include + +#include "blockassembler.h" +#include "chainparams.h" +#include "consensus/merkle.h" +#include "consensus/validation.h" +#include "pow.h" +#include "random.h" +#include "test/test_pivx.h" +#include "validation.h" +#include "validationinterface.h" + +struct RegtestingSetup : public TestingSetup { + RegtestingSetup() : TestingSetup() { + SelectParams(CBaseChainParams::REGTEST); + } +}; + +BOOST_FIXTURE_TEST_SUITE(validation_block_tests, RegtestingSetup) + +struct TestSubscriber : public CValidationInterface { + uint256 m_expected_tip; + + TestSubscriber(uint256 tip) : m_expected_tip(std::move(tip)) {} + + void UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload) + { + BOOST_CHECK_EQUAL(m_expected_tip, pindexNew->GetBlockHash()); + } + + void BlockConnected(const std::shared_ptr& block, const CBlockIndex* pindex, const std::vector& txnConflicted) + { + BOOST_CHECK_EQUAL(m_expected_tip, block->hashPrevBlock); + BOOST_CHECK_EQUAL(m_expected_tip, pindex->pprev->GetBlockHash()); + + m_expected_tip = block->GetHash(); + } + + void BlockDisconnected(const std::shared_ptr &block, const uint256& blockHash, int nBlockHeight, int64_t blockTime) + { + BOOST_CHECK_EQUAL(m_expected_tip, block->GetHash()); + + m_expected_tip = block->hashPrevBlock; + } +}; + +std::shared_ptr Block(const uint256& prev_hash) +{ + static int i = 0; + static uint64_t time = Params().GenesisBlock().nTime; + + CScript pubKey; + pubKey << i++ << OP_TRUE; + + auto ptemplate = BlockAssembler(Params(), false).CreateNewBlock(pubKey); + auto pblock = std::make_shared(ptemplate->block); + pblock->hashPrevBlock = prev_hash; + pblock->nTime = ++time; + + CMutableTransaction txCoinbase(*pblock->vtx[0]); + txCoinbase.vout.resize(1); + pblock->vtx[0] = MakeTransactionRef(std::move(txCoinbase)); + + return pblock; +} + +std::shared_ptr FinalizeBlock(std::shared_ptr pblock) +{ + pblock->hashMerkleRoot = BlockMerkleRoot(*pblock); + + while (!CheckProofOfWork(pblock->GetHash(), pblock->nBits)) { + ++(pblock->nNonce); + } + + return pblock; +} + +// construct a valid block +const std::shared_ptr GoodBlock(const uint256& prev_hash) +{ + return FinalizeBlock(Block(prev_hash)); +} + +// construct an invalid block (but with a valid header) +const std::shared_ptr BadBlock(const uint256& prev_hash) +{ + auto pblock = Block(prev_hash); + + CMutableTransaction coinbase_spend; + coinbase_spend.vin.emplace_back(CTxIn(COutPoint(pblock->vtx[0]->GetHash(), 0), CScript(), 0)); + coinbase_spend.vout.emplace_back(pblock->vtx[0]->vout[0]); + + CTransactionRef tx = MakeTransactionRef(coinbase_spend); + pblock->vtx.emplace_back(tx); + + auto ret = FinalizeBlock(pblock); + return ret; +} + +void BuildChain(const uint256& root, int height, const unsigned int invalid_rate, const unsigned int branch_rate, const unsigned int max_size, std::vector>& blocks) +{ + if (height <= 0 || blocks.size() >= max_size) return; + + bool gen_invalid = GetRand(100) < invalid_rate; + bool gen_fork = GetRand(100) < branch_rate; + + const std::shared_ptr pblock = gen_invalid ? BadBlock(root) : GoodBlock(root); + blocks.emplace_back(pblock); + if (!gen_invalid) { + BuildChain(pblock->GetHash(), height - 1, invalid_rate, branch_rate, max_size, blocks); + } + + if (gen_fork) { + blocks.emplace_back(GoodBlock(root)); + BuildChain(blocks.back()->GetHash(), height - 1, invalid_rate, branch_rate, max_size, blocks); + } +} + +BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering) +{ + // build a large-ish chain that's likely to have some forks + std::vector> blocks; + while (blocks.size() < 50) { + blocks.clear(); + BuildChain(Params().GenesisBlock().GetHash(), 100, 15, 10, 500, blocks); + } + + CValidationState state; + // Connect the genesis block and drain any outstanding events + BOOST_CHECK_MESSAGE(ProcessNewBlock(state, nullptr, std::make_shared(Params().GenesisBlock()), nullptr), "Error: genesis not connected"); + SyncWithValidationInterfaceQueue(); + + // subscribe to events (this subscriber will validate event ordering) + const CBlockIndex* initial_tip = WITH_LOCK(cs_main, return chainActive.Tip()); + TestSubscriber sub(initial_tip->GetBlockHash()); + RegisterValidationInterface(&sub); + + // create a bunch of threads that repeatedly process a block generated above at random + // this will create parallelism and randomness inside validation - the ValidationInterface + // will subscribe to events generated during block validation and assert on ordering invariance + boost::thread_group threads; + for (int i = 0; i < 10; i++) { + threads.create_thread([&blocks]() { + CValidationState state; + for (int i = 0; i < 1000; i++) { + auto block = blocks[GetRand(blocks.size() - 1)]; + ProcessNewBlock(state, nullptr, block, nullptr); + } + + // to make sure that eventually we process the full chain - do it here + for (const auto& block : blocks) { + if (block->vtx.size() == 1) { + bool processed = ProcessNewBlock(state, nullptr, block, nullptr); + // Future to do: "prevblk-not-found" here is the only valid reason to not check processed flag. + if (state.GetRejectReason() == "duplicate" || + state.GetRejectReason() == "prevblk-not-found" || + state.GetRejectReason() == "bad-prevblk") continue; + BOOST_ASSERT_MSG(processed, ("Error: " + state.GetRejectReason()).c_str()); + } + } + }); + } + + threads.join_all(); + while (GetMainSignals().CallbacksPending() > 0) { + MilliSleep(100); + } + + UnregisterValidationInterface(&sub); + + BOOST_CHECK_EQUAL(sub.m_expected_tip, WITH_LOCK(cs_main, return chainActive.Tip()->GetBlockHash())); +} + +BOOST_AUTO_TEST_SUITE_END() From b3a12b58b4f0fa804652e147af6106db36a4826f Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 1 Apr 2021 12:07:48 -0300 Subject: [PATCH 21/31] [RPC] `getwalletinfo`: Add last_processed_block return value. Github-Pull: #2283 Rebased-From: dc2d22a525aa8b6a4d50b11a34269a41e97eaf55 --- src/wallet/rpcwallet.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 6a150e2c32d92..ed615e2c5a650 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3721,12 +3721,13 @@ UniValue getwalletinfo(const JSONRPCRequest& request) " \"immature_balance\": xxxxxx, (numeric) the total immature balance of the wallet in PIV\n" " \"txcount\": xxxxxxx, (numeric) the total number of transactions in the wallet\n" " \"keypoololdest\": xxxxxx, (numeric) the timestamp (seconds since GMT epoch) of the oldest pre-generated key in the key pool\n" - " \"keypoolsize\": xxxx, (numeric) how many new keys are pre-generated (only counts external keys)\n" - " \"keypoolsize_hd_internal\": xxxx, (numeric) how many new keys are pre-generated for internal use (used for change outputs, only appears if the wallet is using this feature, otherwise external keys are used)\n" - " \"keypoolsize_hd_staking\": xxxx, (numeric) how many new keys are pre-generated for staking use (used for staking contracts, only appears if the wallet is using this feature)\n" + " \"keypoolsize\": xxxx, (numeric) how many new keys are pre-generated (only counts external keys)\n" + " \"keypoolsize_hd_internal\": xxxx, (numeric) how many new keys are pre-generated for internal use (used for change outputs, only appears if the wallet is using this feature, otherwise external keys are used)\n" + " \"keypoolsize_hd_staking\": xxxx, (numeric) how many new keys are pre-generated for staking use (used for staking contracts, only appears if the wallet is using this feature)\n" " \"unlocked_until\": ttt, (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked\n" " \"paytxfee\": x.xxxx (numeric) the transaction fee configuration, set in PIV/kB\n" - " \"hdseedid\": \"\" (string, optional) the Hash160 of the HD seed (only present when HD is enabled)\n" + " \"hdseedid\": \"\" (string, optional) the Hash160 of the HD seed (only present when HD is enabled)\n" + " \"last_processed_block\": xxxxx, (numeric) the last block processed block height\n" "}\n" "\nExamples:\n" + @@ -3768,6 +3769,7 @@ UniValue getwalletinfo(const JSONRPCRequest& request) if (pwalletMain->IsCrypted()) obj.pushKV("unlocked_until", nWalletUnlockTime); obj.pushKV("paytxfee", ValueFromAmount(payTxFee.GetFeePerK())); + obj.pushKV("last_processed_block", pwalletMain->GetLastBlockHeight()); return obj; } From 7f244b10d0f0e4f016e2539ab589bd32f437256d Mon Sep 17 00:00:00 2001 From: random-zebra Date: Thu, 8 Apr 2021 18:08:38 +0200 Subject: [PATCH 22/31] [Tests] Check last_processed_block in getwalletinfo Github-Pull: #2283 Rebased-From: a700fdfb0cf41eeea3f09b3f9b7fc9add9192921 --- test/functional/mining_pos_coldStaking.py | 2 ++ test/functional/mining_pos_reorg.py | 1 + test/functional/sapling_wallet_anchorfork.py | 1 + test/functional/wallet_basic.py | 11 ++++++++++- 4 files changed, 14 insertions(+), 1 deletion(-) diff --git a/test/functional/mining_pos_coldStaking.py b/test/functional/mining_pos_coldStaking.py index 07a6b2e0e2b3b..f5c5eb289acbb 100755 --- a/test/functional/mining_pos_coldStaking.py +++ b/test/functional/mining_pos_coldStaking.py @@ -421,6 +421,7 @@ def run_test(self): def checkBalances(self): w_info = self.nodes[0].getwalletinfo() + assert_equal(self.nodes[0].getblockcount(), w_info['last_processed_block']) self.log.info("OWNER - Delegated %f / Cold %f [%f / %f]" % ( float(w_info["delegated_balance"]), w_info["cold_staking_balance"], float(w_info["immature_delegated_balance"]), w_info["immature_cold_staking_balance"])) @@ -428,6 +429,7 @@ def checkBalances(self): assert_equal(float(w_info["immature_delegated_balance"]), self.expected_immature_balance) assert_equal(float(w_info["cold_staking_balance"]), 0) w_info = self.nodes[1].getwalletinfo() + assert_equal(self.nodes[1].getblockcount(), w_info['last_processed_block']) self.log.info("STAKER - Delegated %f / Cold %f [%f / %f]" % ( float(w_info["delegated_balance"]), w_info["cold_staking_balance"], float(w_info["immature_delegated_balance"]), w_info["immature_cold_staking_balance"])) diff --git a/test/functional/mining_pos_reorg.py b/test/functional/mining_pos_reorg.py index d59ee8d8006d8..35cb843504059 100755 --- a/test/functional/mining_pos_reorg.py +++ b/test/functional/mining_pos_reorg.py @@ -47,6 +47,7 @@ def disconnect_all(self): def get_tot_balance(self, nodeid): wi = self.nodes[nodeid].getwalletinfo() + assert_equal(self.nodes[nodeid].getblockcount(), wi['last_processed_block']) return wi['balance'] + wi['immature_balance'] def check_money_supply(self, expected_piv): diff --git a/test/functional/sapling_wallet_anchorfork.py b/test/functional/sapling_wallet_anchorfork.py index 92a4666885f34..40943257ab6e9 100755 --- a/test/functional/sapling_wallet_anchorfork.py +++ b/test/functional/sapling_wallet_anchorfork.py @@ -24,6 +24,7 @@ def run_test (self): self.sync_all() walletinfo = self.nodes[0].getwalletinfo() + assert_equal(self.nodes[0].getblockcount(), walletinfo['last_processed_block']) assert_equal(walletinfo['immature_balance'], 1000) assert_equal(walletinfo['balance'], 0) diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 2b8f7af43767f..e0c8634a855e0 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -32,6 +32,9 @@ def setup_network(self): def get_vsize(self, txn): return self.nodes[0].decoderawtransaction(txn)['size'] + def check_wallet_processed_blocks(self, nodeid, walletinfo): + assert_equal(self.nodes[nodeid].getblockcount(), walletinfo['last_processed_block']) + def run_test(self): # Check that there's no UTXO on none of the nodes assert_equal(len(self.nodes[0].listunspent()), 0) @@ -43,6 +46,7 @@ def run_test(self): self.nodes[0].generate(1) walletinfo = self.nodes[0].getwalletinfo() + self.check_wallet_processed_blocks(0, walletinfo) assert_equal(walletinfo['immature_balance'], 250) assert_equal(walletinfo['balance'], 0) @@ -54,13 +58,17 @@ def run_test(self): assert_equal(self.nodes[1].getbalance(), 250) assert_equal(self.nodes[2].getbalance(), 0) + walletinfo = self.nodes[0].getwalletinfo() + self.check_wallet_processed_blocks(0, walletinfo) + self.check_wallet_processed_blocks(1, self.nodes[1].getwalletinfo()) + self.check_wallet_processed_blocks(2, self.nodes[2].getwalletinfo()) + # Check that only first and second nodes have UTXOs utxos = self.nodes[0].listunspent() assert_equal(len(utxos), 1) assert_equal(len(self.nodes[1].listunspent()), 1) assert_equal(len(self.nodes[2].listunspent()), 0) - walletinfo = self.nodes[0].getwalletinfo() assert_equal(walletinfo['immature_balance'], 0) # Exercise locking of unspent outputs @@ -185,6 +193,7 @@ def run_test(self): assert_equal(balance_nodes, [self.nodes[i].getbalance() for i in range(3)]) # Exercise listsinceblock with the last two blocks + self.check_wallet_processed_blocks(0, self.nodes[0].getwalletinfo()) coinbase_tx_1 = self.nodes[0].listsinceblock(blocks[0]) assert_equal(coinbase_tx_1["lastblock"], blocks[1]) assert_equal(len(coinbase_tx_1["transactions"]), 1) From a78dfbe574bccd4e6f864d211fbdaeb7b64aa932 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 3 Apr 2021 10:16:47 -0300 Subject: [PATCH 23/31] Validation: Remove `CheckBlockSignature` now unneeded enableP2PKH flag. It was only needed for the activation period, now that it's fully enforced, can be removed. The activation time isn't needed, v5 enforcement checkpoint is ensuring that the chain cannot reorg to a time in which P2PKH stakes weren't enabled. Github-Pull: #2295 Rebased-From: 5aa36008f1d2ea8bf70bb263bbfc14a32f562fb8 --- src/blocksignature.cpp | 9 +-------- src/blocksignature.h | 2 +- src/test/main_tests.cpp | 27 +++++++-------------------- src/validation.cpp | 8 +++----- 4 files changed, 12 insertions(+), 34 deletions(-) diff --git a/src/blocksignature.cpp b/src/blocksignature.cpp index dedf764f83ffc..df698702803d4 100644 --- a/src/blocksignature.cpp +++ b/src/blocksignature.cpp @@ -40,7 +40,7 @@ bool SignBlock(CBlock& block, const CKeyStore& keystore) return SignBlockWithKey(block, key); } -bool CheckBlockSignature(const CBlock& block, const bool enableP2PKH) +bool CheckBlockSignature(const CBlock& block) { if (block.IsProofOfWork()) return block.vchBlockSig.empty(); @@ -64,13 +64,6 @@ bool CheckBlockSignature(const CBlock& block, const bool enableP2PKH) if (!Solver(txout.scriptPubKey, whichType, vSolutions)) return false; - if (!enableP2PKH) { - // Before v5 activation, P2PKH was always failing. - if (whichType == TX_PUBKEYHASH) { - return false; - } - } - if (whichType == TX_PUBKEY) { valtype& vchPubKey = vSolutions[0]; pubkey = CPubKey(vchPubKey); diff --git a/src/blocksignature.h b/src/blocksignature.h index feed6af20d5ac..ad399b85dbc0e 100644 --- a/src/blocksignature.h +++ b/src/blocksignature.h @@ -11,6 +11,6 @@ bool SignBlockWithKey(CBlock& block, const CKey& key); bool SignBlock(CBlock& block, const CKeyStore& keystore); -bool CheckBlockSignature(const CBlock& block, const bool enableP2PKH); +bool CheckBlockSignature(const CBlock& block); #endif //PIVX_BLOCKSIGNATURE_H diff --git a/src/test/main_tests.cpp b/src/test/main_tests.cpp index 00d6ef62ab574..6e5a9e97f8f0d 100644 --- a/src/test/main_tests.cpp +++ b/src/test/main_tests.cpp @@ -72,14 +72,9 @@ CBlock CreateDummyBlockWithSignature(CKey stakingKey, BlockSignatureType type, b return block; } -bool TestBlockSignaturePreEnforcementV5(const CBlock& block) +bool TestBlockSignature(const CBlock& block) { - return CheckBlockSignature(block, false); -} - -bool TestBlockSignaturePostEnforcementV5(const CBlock& block) -{ - return CheckBlockSignature(block, true); + return CheckBlockSignature(block); } BOOST_AUTO_TEST_CASE(block_signature_test) @@ -89,27 +84,19 @@ BOOST_AUTO_TEST_CASE(block_signature_test) stakingKey.MakeNewKey(true); bool useInputP2PK = i % 2 == 0; - // Test P2PK block signature pre enforcement. + // Test P2PK block signature CBlock block = CreateDummyBlockWithSignature(stakingKey, BlockSignatureType::P2PK, useInputP2PK); - BOOST_CHECK(TestBlockSignaturePreEnforcementV5(block)); - - // Test P2PK block signature post enforcement - block = CreateDummyBlockWithSignature(stakingKey, BlockSignatureType::P2PK, useInputP2PK); - BOOST_CHECK(TestBlockSignaturePostEnforcementV5(block)); - - // Test P2PKH block signature pre enforcement ---> must fail. - block = CreateDummyBlockWithSignature(stakingKey, BlockSignatureType::P2PKH, useInputP2PK); - BOOST_CHECK(!TestBlockSignaturePreEnforcementV5(block)); + BOOST_CHECK(TestBlockSignature(block)); - // Test P2PKH block signature post enforcement + // Test P2PKH block signature block = CreateDummyBlockWithSignature(stakingKey, BlockSignatureType::P2PKH, useInputP2PK); if (useInputP2PK) { // If it's using a P2PK scriptsig as input and a P2PKH output // The block doesn't contain the public key to verify the sig anywhere. // Must fail. - BOOST_CHECK(!TestBlockSignaturePostEnforcementV5(block)); + BOOST_CHECK(!TestBlockSignature(block)); } else { - BOOST_CHECK(TestBlockSignaturePostEnforcementV5(block)); + BOOST_CHECK(TestBlockSignature(block)); } } } diff --git a/src/validation.cpp b/src/validation.cpp index 59ff2cc0d1272..70c6f55b84fa7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3263,15 +3263,12 @@ bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const std::shared_pt // Preliminary checks int64_t nStartTime = GetTimeMillis(); const Consensus::Params& consensus = Params().GetConsensus(); + int newHeight = 0; // check block bool checked = CheckBlock(*pblock, state); - // For now, we need the tip to know whether p2pkh block signatures are accepted or not. - // After 5.0, this can be removed and replaced by the enforcement block time. - const int newHeight = chainActive.Height() + 1; - const bool enableP2PKH = consensus.NetworkUpgradeActive(newHeight, Consensus::UPGRADE_V5_0); - if (!CheckBlockSignature(*pblock, enableP2PKH)) + if (!CheckBlockSignature(*pblock)) return error("%s : bad proof-of-stake block signature", __func__); if (pblock->GetHash() != consensus.hashGenesisBlock && pfrom != NULL) { @@ -3298,6 +3295,7 @@ bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const std::shared_pt if (!ret) { return error("%s : AcceptBlock FAILED", __func__); } + newHeight = pindex->nHeight; } if (!ActivateBestChain(state, pblock, checked)) From 374f6c9efce9df0946b23c455cc3c6124f44f86e Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 3 Apr 2021 11:53:19 -0300 Subject: [PATCH 24/31] Refactor: move `CheckBlockSignature` function call inside `CheckBlock`. Github-Pull: #2295 Rebased-From: 47cae2825bc6aead138c75d151ce8b0ab21f58e8 --- src/blockassembler.cpp | 2 +- src/primitives/block.h | 2 +- src/validation.cpp | 15 +++++++++------ src/validation.h | 2 +- 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/blockassembler.cpp b/src/blockassembler.cpp index e4fab31464c8c..7e8da0663db5e 100644 --- a/src/blockassembler.cpp +++ b/src/blockassembler.cpp @@ -226,7 +226,7 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc if (chainActive.Tip() != pindexPrev) return nullptr; // new block came in, move on CValidationState state; - if (!TestBlockValidity(state, *pblock, pindexPrev, false, false)) { + if (!TestBlockValidity(state, *pblock, pindexPrev, false, false, false)) { throw std::runtime_error( strprintf("%s: TestBlockValidity failed: %s", __func__, FormatStateMessage(state))); } diff --git a/src/primitives/block.h b/src/primitives/block.h index ed86e531daf89..aee2abe93c925 100644 --- a/src/primitives/block.h +++ b/src/primitives/block.h @@ -94,7 +94,7 @@ class CBlock : public CBlockHeader std::vector vchBlockSig; // memory only - mutable bool fChecked; + mutable bool fChecked{false}; CBlock() { diff --git a/src/validation.cpp b/src/validation.cpp index 70c6f55b84fa7..2bd2dd53f42c1 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1376,7 +1376,7 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd { AssertLockHeld(cs_main); // Check it again in case a previous version let a bad block in - if (!fAlreadyChecked && !CheckBlock(block, state, !fJustCheck, !fJustCheck)) { + if (!fAlreadyChecked && !CheckBlock(block, state, !fJustCheck, !fJustCheck, !fJustCheck)) { if (state.CorruptionPossible()) { // We don't write down blocks to disk if they may have been // corrupted, so this should be impossible unless we're having hardware @@ -2776,6 +2776,12 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo return state.DoS(100, error("%s : out-of-bounds SigOpCount", __func__), REJECT_INVALID, "bad-blk-sigops", true); + // Check PoS signature. + if (fCheckSig && !CheckBlockSignature(block)) { + return state.DoS(100, error("%s : bad proof-of-stake block signature", __func__), + REJECT_INVALID, "bad-PoS-sig", true); + } + if (fCheckPOW && fCheckMerkleRoot && fCheckSig) block.fChecked = true; @@ -3268,9 +3274,6 @@ bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const std::shared_pt // check block bool checked = CheckBlock(*pblock, state); - if (!CheckBlockSignature(*pblock)) - return error("%s : bad proof-of-stake block signature", __func__); - if (pblock->GetHash() != consensus.hashGenesisBlock && pfrom != NULL) { //if we get this far, check if the prev block is our prev block, if not then request sync and return false BlockMap::iterator mi = mapBlockIndex.find(pblock->hashPrevBlock); @@ -3315,7 +3318,7 @@ bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const std::shared_pt return true; } -bool TestBlockValidity(CValidationState& state, const CBlock& block, CBlockIndex* const pindexPrev, bool fCheckPOW, bool fCheckMerkleRoot) +bool TestBlockValidity(CValidationState& state, const CBlock& block, CBlockIndex* const pindexPrev, bool fCheckPOW, bool fCheckMerkleRoot, bool fCheckBlockSig) { AssertLockHeld(cs_main); assert(pindexPrev); @@ -3332,7 +3335,7 @@ bool TestBlockValidity(CValidationState& state, const CBlock& block, CBlockIndex // NOTE: CheckBlockHeader is called by CheckBlock if (!ContextualCheckBlockHeader(block, state, pindexPrev)) return error("%s: ContextualCheckBlockHeader failed: %s", __func__, FormatStateMessage(state)); - if (!CheckBlock(block, state, fCheckPOW, fCheckMerkleRoot)) + if (!CheckBlock(block, state, fCheckPOW, fCheckMerkleRoot, fCheckBlockSig)) return error("%s: CheckBlock failed: %s", __func__, FormatStateMessage(state)); if (!ContextualCheckBlock(block, state, pindexPrev)) return error("%s: ContextualCheckBlock failed: %s", __func__, FormatStateMessage(state)); diff --git a/src/validation.h b/src/validation.h index 1bbe01aa687d4..ae262f9c6ba7e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -332,7 +332,7 @@ bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& sta bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindexPrev); /** Check a block is completely valid from start to finish (only works on top of our current best block, with cs_main held) */ -bool TestBlockValidity(CValidationState& state, const CBlock& block, CBlockIndex* pindexPrev, bool fCheckPOW = true, bool fCheckMerkleRoot = true); +bool TestBlockValidity(CValidationState& state, const CBlock& block, CBlockIndex* pindexPrev, bool fCheckPOW = true, bool fCheckMerkleRoot = true, bool fCheckBlockSig = true); /** Store block on disk. If dbp is provided, the file is known to already reside on disk */ bool AcceptBlock(const CBlock& block, CValidationState& state, CBlockIndex** pindex, CDiskBlockPos* dbp = NULL, bool fAlreadyCheckedBlock = false); From f53c82483b2faca0832f93d614ab0bc089db133b Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 3 Apr 2021 11:58:29 -0300 Subject: [PATCH 25/31] Refactor: remove redundant `fAlreadyCheckedBlock` argument from `AcceptBlock` `CBlock` has a member `fChecked` that has the exact same purpose as the `fAlreadyCheckedBlock` flag. Github-Pull: #2295 Rebased-From: 362a5982130ba22a5a514383590fd066f962d968 --- src/validation.cpp | 6 +++--- src/validation.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 2bd2dd53f42c1..1d45119a0f13d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3016,7 +3016,7 @@ bool AcceptBlockHeader(const CBlock& block, CValidationState& state, CBlockIndex return true; } -bool AcceptBlock(const CBlock& block, CValidationState& state, CBlockIndex** ppindex, CDiskBlockPos* dbp, bool fAlreadyCheckedBlock) +bool AcceptBlock(const CBlock& block, CValidationState& state, CBlockIndex** ppindex, CDiskBlockPos* dbp) { AssertLockHeld(cs_main); @@ -3049,7 +3049,7 @@ bool AcceptBlock(const CBlock& block, CValidationState& state, CBlockIndex** ppi return true; } - if ((!fAlreadyCheckedBlock && !CheckBlock(block, state)) || !ContextualCheckBlock(block, state, pindex->pprev)) { + if (!CheckBlock(block, state) || !ContextualCheckBlock(block, state, pindex->pprev)) { if (state.IsInvalid() && !state.CorruptionPossible()) { pindex->nStatus |= BLOCK_FAILED_VALID; setDirtyBlockIndex.insert(pindex); @@ -3292,7 +3292,7 @@ bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const std::shared_pt // Store to disk CBlockIndex* pindex = nullptr; - bool ret = AcceptBlock(*pblock, state, &pindex, dbp, checked); + bool ret = AcceptBlock(*pblock, state, &pindex, dbp); if (fAccepted) *fAccepted = ret; CheckBlockIndex(); if (!ret) { diff --git a/src/validation.h b/src/validation.h index ae262f9c6ba7e..5eaab72648a6d 100644 --- a/src/validation.h +++ b/src/validation.h @@ -335,7 +335,7 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIn bool TestBlockValidity(CValidationState& state, const CBlock& block, CBlockIndex* pindexPrev, bool fCheckPOW = true, bool fCheckMerkleRoot = true, bool fCheckBlockSig = true); /** Store block on disk. If dbp is provided, the file is known to already reside on disk */ -bool AcceptBlock(const CBlock& block, CValidationState& state, CBlockIndex** pindex, CDiskBlockPos* dbp = NULL, bool fAlreadyCheckedBlock = false); +bool AcceptBlock(const CBlock& block, CValidationState& state, CBlockIndex** pindex, CDiskBlockPos* dbp = NULL); bool AcceptBlockHeader(const CBlock& block, CValidationState& state, CBlockIndex** ppindex = nullptr, CBlockIndex* pindexPrev = nullptr); From 1f38b907687db37b556c0d24899d6d4268b65b48 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 3 Apr 2021 12:21:59 -0300 Subject: [PATCH 26/31] Removal of the `fAlreadyChecked` flag from the entire `ActivateBestChain` flow. Github-Pull: #2295 Rebased-From: 6355774c2e66bd6cf7df3d404588b2386b8b2be9 --- src/validation.cpp | 23 +++++++++-------------- src/validation.h | 2 +- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 1d45119a0f13d..11f38c38504e8 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1372,11 +1372,11 @@ static int64_t nTimeTotal = 0; /** Apply the effects of this block (with given index) on the UTXO set represented by coins. * Validity checks that depend on the UTXO set are also done; ConnectBlock() * can fail if those validity checks fail (among other reasons). */ -static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& view, bool fJustCheck = false, bool fAlreadyChecked = false) +static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& view, bool fJustCheck = false) { AssertLockHeld(cs_main); // Check it again in case a previous version let a bad block in - if (!fAlreadyChecked && !CheckBlock(block, state, !fJustCheck, !fJustCheck, !fJustCheck)) { + if (!CheckBlock(block, state, !fJustCheck, !fJustCheck, !fJustCheck)) { if (state.CorruptionPossible()) { // We don't write down blocks to disk if they may have been // corrupted, so this should be impossible unless we're having hardware @@ -1997,13 +1997,10 @@ class ConnectTrace { * * The block is added to connectTrace if connection succeeds. */ -bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr& pblock, bool fAlreadyChecked, ConnectTrace& connectTrace) +bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr& pblock, ConnectTrace& connectTrace) { assert(pindexNew->pprev == chainActive.Tip()); - if (pblock == NULL) - fAlreadyChecked = false; - // Read block from disk. int64_t nTime1 = GetTimeMicros(); std::shared_ptr pthisBlock; @@ -2024,7 +2021,7 @@ bool static ConnectTip(CValidationState& state, CBlockIndex* pindexNew, const st LogPrint(BCLog::BENCH, " - Load block from disk: %.2fms [%.2fs]\n", (nTime2 - nTime1) * 0.001, nTimeReadFromDisk * 0.000001); { CCoinsViewCache view(pcoinsTip); - bool rv = ConnectBlock(blockConnecting, state, pindexNew, view, false, fAlreadyChecked); + bool rv = ConnectBlock(blockConnecting, state, pindexNew, view, false); GetMainSignals().BlockChecked(blockConnecting, state); if (!rv) { if (state.IsInvalid()) @@ -2144,11 +2141,9 @@ static void PruneBlockIndexCandidates() * Try to make some progress towards making pindexMostWork the active block. * pblock is either NULL or a pointer to a CBlock corresponding to pindexMostWork. */ -static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr& pblock, bool fAlreadyChecked, bool& fInvalidFound, ConnectTrace& connectTrace) +static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) { AssertLockHeld(cs_main); - if (pblock == NULL) - fAlreadyChecked = false; const CBlockIndex* pindexOldTip = chainActive.Tip(); const CBlockIndex* pindexFork = chainActive.FindFork(pindexMostWork); @@ -2179,7 +2174,7 @@ static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMo // Connect new blocks. for (CBlockIndex* pindexConnect : reverse_iterate(vpindexToConnect)) { - if (!ConnectTip(state, pindexConnect, (pindexConnect == pindexMostWork) ? pblock : std::shared_ptr(), fAlreadyChecked, connectTrace)) { + if (!ConnectTip(state, pindexConnect, (pindexConnect == pindexMostWork) ? pblock : std::shared_ptr(), connectTrace)) { if (state.IsInvalid()) { // The block violates a consensus rule. if (!state.CorruptionPossible()) @@ -2224,7 +2219,7 @@ static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMo * that is already loaded (to avoid loading it again from disk). */ -bool ActivateBestChain(CValidationState& state, std::shared_ptr pblock, bool fAlreadyChecked) +bool ActivateBestChain(CValidationState& state, std::shared_ptr pblock) { // Note that while we're often called here from ProcessNewBlock, this is // far from a guarantee. Things in the P2P/RPC will often end up calling @@ -2270,7 +2265,7 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb bool fInvalidFound = false; std::shared_ptr nullBlockPtr; - if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fAlreadyChecked, fInvalidFound, connectTrace)) + if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace)) return false; blocks_connected = true; @@ -3301,7 +3296,7 @@ bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const std::shared_pt newHeight = pindex->nHeight; } - if (!ActivateBestChain(state, pblock, checked)) + if (!ActivateBestChain(state, pblock)) return error("%s : ActivateBestChain failed", __func__); if (!fLiteMode) { diff --git a/src/validation.h b/src/validation.h index 5eaab72648a6d..790c501aa03eb 100644 --- a/src/validation.h +++ b/src/validation.h @@ -207,7 +207,7 @@ double ConvertBitsToDouble(unsigned int nBits); int64_t GetMasternodePayment(); /** Find the best known block, and make it the tip of the block chain */ -bool ActivateBestChain(CValidationState& state, std::shared_ptr pblock = std::shared_ptr(), bool fAlreadyChecked = false); +bool ActivateBestChain(CValidationState& state, std::shared_ptr pblock = std::shared_ptr()); CAmount GetBlockValue(int nHeight); /** Create a new block index entry for a given block hash */ From c44b431886419f4a5bfb981c3ca39ae6b6a83b91 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 3 Apr 2021 13:38:53 -0300 Subject: [PATCH 27/31] validation: missing cs_main lock for `CheckBlock` in `ProcessNewBlock` Github-Pull: #2295 Rebased-From: ce1781e42a7293bd030e644ad5c7e53fb824b10f --- src/validation.cpp | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 11f38c38504e8..74f7dd8ca0320 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3266,25 +3266,23 @@ bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const std::shared_pt const Consensus::Params& consensus = Params().GetConsensus(); int newHeight = 0; - // check block - bool checked = CheckBlock(*pblock, state); - - if (pblock->GetHash() != consensus.hashGenesisBlock && pfrom != NULL) { - //if we get this far, check if the prev block is our prev block, if not then request sync and return false - BlockMap::iterator mi = mapBlockIndex.find(pblock->hashPrevBlock); - if (mi == mapBlockIndex.end()) { - g_connman->PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::GETBLOCKS, chainActive.GetLocator(), UINT256_ZERO)); - return false; - } - } - { + // CheckBlock requires cs_main lock LOCK(cs_main); - - if (!checked) { + if (!CheckBlock(*pblock, state)) { return error ("%s : CheckBlock FAILED for block %s, %s", __func__, pblock->GetHash().GetHex(), FormatStateMessage(state)); } + // Request sync if needed + if (pblock->GetHash() != consensus.hashGenesisBlock && !pfrom) { + // if we get this far, check if the prev block is our prev block, if not then request sync and return false + BlockMap::iterator mi = mapBlockIndex.find(pblock->hashPrevBlock); + if (mi == mapBlockIndex.end()) { + g_connman->PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::GETBLOCKS, chainActive.GetLocator(), UINT256_ZERO)); + return false; + } + } + // Store to disk CBlockIndex* pindex = nullptr; bool ret = AcceptBlock(*pblock, state, &pindex, dbp); From 64bd400b96ac8bd5367740b1717f5abe2ac377b9 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 5 Apr 2021 00:33:56 -0300 Subject: [PATCH 28/31] validation: Remove redundant request sync from ProcessNewBlock(). ProcessNewBlock is called by ProcessMessage (NetMsgType::BLOCK command) which is doing the exact same check: if the node does not have the previous block, request sync up to it. Github-Pull: #2295 Rebased-From: 959936f23321bb48d1bc175b59c1cfcc81339cda --- src/validation.cpp | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 74f7dd8ca0320..ee6b90377b8a2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3273,16 +3273,6 @@ bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const std::shared_pt return error ("%s : CheckBlock FAILED for block %s, %s", __func__, pblock->GetHash().GetHex(), FormatStateMessage(state)); } - // Request sync if needed - if (pblock->GetHash() != consensus.hashGenesisBlock && !pfrom) { - // if we get this far, check if the prev block is our prev block, if not then request sync and return false - BlockMap::iterator mi = mapBlockIndex.find(pblock->hashPrevBlock); - if (mi == mapBlockIndex.end()) { - g_connman->PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::GETBLOCKS, chainActive.GetLocator(), UINT256_ZERO)); - return false; - } - } - // Store to disk CBlockIndex* pindex = nullptr; bool ret = AcceptBlock(*pblock, state, &pindex, dbp); From 0d2555edeb5ffb4161650bc1f1800f3ff75966b9 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 5 Apr 2021 10:28:28 -0300 Subject: [PATCH 29/31] net_processing: missing cs_main lock for chainActive.GetLocator() call Github-Pull: #2295 Rebased-From: 21646d8b2357da30a6d9839bab8ba0bb7b95abf7 --- src/net_processing.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 907cca1cab74d..49c898edf2b61 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1599,16 +1599,17 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR CInv inv(MSG_BLOCK, hashBlock); LogPrint(BCLog::NET, "received block %s peer=%d\n", inv.hash.ToString(), pfrom->id); - //sometimes we will be sent their most recent block and its not the one we want, in that case tell where we are + // sometimes we will be sent their most recent block and its not the one we want, in that case tell where we are if (!mapBlockIndex.count(pblock->hashPrevBlock)) { + CBlockLocator locator = WITH_LOCK(cs_main, return chainActive.GetLocator();); if (find(pfrom->vBlockRequested.begin(), pfrom->vBlockRequested.end(), hashBlock) != pfrom->vBlockRequested.end()) { - //we already asked for this block, so lets work backwards and ask for the previous block - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::GETBLOCKS, chainActive.GetLocator(), pblock->hashPrevBlock)); - pfrom->vBlockRequested.push_back(pblock->hashPrevBlock); + // we already asked for this block, so lets work backwards and ask for the previous block + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::GETBLOCKS, locator, pblock->hashPrevBlock)); + pfrom->vBlockRequested.emplace_back(pblock->hashPrevBlock); } else { - //ask to sync to this block - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::GETBLOCKS, chainActive.GetLocator(), hashBlock)); - pfrom->vBlockRequested.push_back(hashBlock); + // ask to sync to this block + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::GETBLOCKS, locator, hashBlock)); + pfrom->vBlockRequested.emplace_back(hashBlock); } } else { pfrom->AddInventoryKnown(inv); From 1fc145c1003148a09a826c24f0fe066a4abf958e Mon Sep 17 00:00:00 2001 From: MishaPozdnikin Date: Thu, 25 Feb 2021 17:32:06 +0200 Subject: [PATCH 30/31] Automatically set the lowest possible Custom Fee when user type in fee that is too low. Github-Pull: #2215 Rebased-From: 60bc46821891ba2b161001e552eff8b970422db1 --- src/qt/pivx/sendcustomfeedialog.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qt/pivx/sendcustomfeedialog.cpp b/src/qt/pivx/sendcustomfeedialog.cpp index f70c896fcb4c8..b0650cccec196 100644 --- a/src/qt/pivx/sendcustomfeedialog.cpp +++ b/src/qt/pivx/sendcustomfeedialog.cpp @@ -127,6 +127,9 @@ void SendCustomFeeDialog::accept() inform(tr("Fee too high. Must be below: %1").arg( BitcoinUnits::formatWithUnit(walletModel->getOptionsModel()->getDisplayUnit(), insaneFee))); } else if (customFee < CWallet::GetRequiredFee(1000)) { + CAmount nFee; + walletModel->getWalletCustomFee(nFee); + ui->lineEditCustomFee->setText(BitcoinUnits::format(walletModel->getOptionsModel()->getDisplayUnit(), nFee)); inform(tr("Fee too low. Must be at least: %1").arg( BitcoinUnits::formatWithUnit(walletModel->getOptionsModel()->getDisplayUnit(), CWallet::GetRequiredFee(1000)))); } else { From 50a5e841c93fcfbd02ad288a936f19b1a500dcc7 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 9 Apr 2021 12:16:48 -0300 Subject: [PATCH 31/31] GUI: if the custom fee is disabled, use the minimum required fee and not the stored value. --- src/qt/pivx/sendcustomfeedialog.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/qt/pivx/sendcustomfeedialog.cpp b/src/qt/pivx/sendcustomfeedialog.cpp index b0650cccec196..9712b8d224dfa 100644 --- a/src/qt/pivx/sendcustomfeedialog.cpp +++ b/src/qt/pivx/sendcustomfeedialog.cpp @@ -127,8 +127,12 @@ void SendCustomFeeDialog::accept() inform(tr("Fee too high. Must be below: %1").arg( BitcoinUnits::formatWithUnit(walletModel->getOptionsModel()->getDisplayUnit(), insaneFee))); } else if (customFee < CWallet::GetRequiredFee(1000)) { - CAmount nFee; - walletModel->getWalletCustomFee(nFee); + CAmount nFee = 0; + if (walletModel->hasWalletCustomFee()) { + walletModel->getWalletCustomFee(nFee); + } else { + nFee = CWallet::GetRequiredFee(1000); + } ui->lineEditCustomFee->setText(BitcoinUnits::format(walletModel->getOptionsModel()->getDisplayUnit(), nFee)); inform(tr("Fee too low. Must be at least: %1").arg( BitcoinUnits::formatWithUnit(walletModel->getOptionsModel()->getDisplayUnit(), CWallet::GetRequiredFee(1000))));