diff --git a/qa/pull-tester/rpc-tests.sh b/qa/pull-tester/rpc-tests.sh index 226d871e8..7adeda3dd 100755 --- a/qa/pull-tester/rpc-tests.sh +++ b/qa/pull-tester/rpc-tests.sh @@ -106,6 +106,7 @@ testScriptsExt=( 'invalidblockrequest.py' # 'forknotify.py' 'p2p-acceptblock.py' + 'wallet_db_flush.py' ); if [ "x$ENABLE_ZMQ" = "x1" ]; then diff --git a/qa/rpc-tests/wallet_anchorfork.py b/qa/rpc-tests/wallet_anchorfork.py index f02ab542a..86c56e72d 100755 --- a/qa/rpc-tests/wallet_anchorfork.py +++ b/qa/rpc-tests/wallet_anchorfork.py @@ -6,7 +6,9 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal, initialize_chain_clean, \ start_nodes, stop_nodes, connect_nodes_bi, \ - wait_and_assert_operationid_status, wait_bitcoinds, get_coinbase_address + wait_and_assert_operationid_status, wait_bitcoinds, get_coinbase_address, \ + sync_blocks, sync_mempools + from decimal import Decimal class WalletAnchorForkTest (BitcoinTestFramework): @@ -83,6 +85,8 @@ class WalletAnchorForkTest (BitcoinTestFramework): txid2 = self.nodes[1].sendrawtransaction(rawhex) assert_equal(txid, txid2) self.nodes[1].generate(1) + sync_blocks(self.nodes[1:]) + sync_mempools(self.nodes[1:]) # Check that Partition B is one block ahead and that they have different tips assert_equal(self.nodes[0].getblockcount() + 1, self.nodes[1].getblockcount()) diff --git a/qa/rpc-tests/wallet_db_flush.py b/qa/rpc-tests/wallet_db_flush.py new file mode 100755 index 000000000..56fd83e9e --- /dev/null +++ b/qa/rpc-tests/wallet_db_flush.py @@ -0,0 +1,85 @@ +#!/usr/bin/env python3 +# Copyright (c) 2020 The Zcash developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or https://www.opensource.org/licenses/mit-license.php . +# +# This test reproduces https://github.com/zcash/zcash/issues/4301 +# It takes an hour to run! + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + bitcoind_processes, + initialize_chain_clean, + start_node, +) +import time + +class WalletDBFlush (BitcoinTestFramework): + + def setup_chain(self): + print("Initializing test directory "+self.options.tmpdir) + initialize_chain_clean(self.options.tmpdir, 1) + + def start_node_with(self, index, extra_args=[]): + args = [ + "-nuparams=2bb40e60:1", # Blossom + "-nuparams=f5b9230b:2", # Heartwood + "-nurejectoldversions=false", + ] + return start_node(index, self.options.tmpdir, args + extra_args) + + def setup_network(self, split=False): + self.nodes = [] + self.nodes.append(self.start_node_with(0)) + self.is_network_split=False + self.sync_all() + + def run_test (self): + print("PLEASE NOTE: This test takes an hour to run!") + + # This test requires shielded funds in the local wallet so + # there is witness data, and the easiest way to get shielded + # funds is to mine (since Heartwood, mining reward can go to + # a zaddr), so first create a Sapling address to mine to. + zaddr = self.nodes[0].z_getnewaddress('sapling') + self.nodes[0].generate(2) + + self.nodes[0].stop() + bitcoind_processes[0].wait() + + print("Start mining to address ", zaddr) + self.nodes[0] = self.start_node_with(0, [ + "-mineraddress=%s" % zaddr, + ]) + self.nodes[0].generate(1) + self.sync_all() + self.nodes[0].stop() + bitcoind_processes[0].wait() + + # If you replace main.cpp:3129 DATABASE_WRITE_INTERVAL with + # 60 (seconds), then sleeptime here can be 80, and this test + # will fail (pre-PR) much faster. + sleeptime = 3620 # just over one hour (DATABASE_WRITE_INTERVAL) + + print("Restart, sleep {}, mine (pre-PR will flush bad wallet state)".format(sleeptime)) + self.nodes[0] = self.start_node_with(0, [ + "-mineraddress=%s" % zaddr, + ]) + assert_equal(self.nodes[0].z_getbalance(zaddr, 0), 5) + time.sleep(sleeptime) + self.nodes[0].generate(1) + self.sync_all() + self.nodes[0].stop() + bitcoind_processes[0].wait() + + print("Restart, generate, expect assert in CopyPreviousWitnesses") + self.nodes[0] = self.start_node_with(0, [ + "-mineraddress=%s" % zaddr, + ]) + self.nodes[0].generate(1) + self.sync_all() + self.nodes[0].stop() + +if __name__ == '__main__': + WalletDBFlush().main() diff --git a/src/main.cpp b/src/main.cpp index bad3fd3a5..8d08fa397 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3084,7 +3084,6 @@ bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) { LOCK2(cs_main, cs_LastBlockFile); static int64_t nLastWrite = 0; static int64_t nLastFlush = 0; - static int64_t nLastSetChain = 0; std::set setFilesToPrune; bool fFlushForPrune = false; try { @@ -3107,9 +3106,6 @@ bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) { if (nLastFlush == 0) { nLastFlush = nNow; } - if (nLastSetChain == 0) { - nLastSetChain = nNow; - } size_t cacheSize = pcoinsTip->DynamicMemoryUsage(); // The cache is large and close to the limit, but we have time now (not in the middle of a block processing). bool fCacheLarge = mode == FLUSH_STATE_PERIODIC && cacheSize * (10.0/9) > nCoinCacheUsage; @@ -3165,11 +3161,7 @@ bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) { return AbortNode(state, "Failed to write to coin database"); nLastFlush = nNow; } - if ((mode == FLUSH_STATE_ALWAYS || mode == FLUSH_STATE_PERIODIC) && nNow > nLastSetChain + (int64_t)DATABASE_WRITE_INTERVAL * 1000000) { - // Update best block in wallet (so we can detect restored wallets). - GetMainSignals().SetBestChain(chainActive.GetLocator()); - nLastSetChain = nNow; - } + // Don't flush the wallet witness cache (SetBestChain()) here, see #4301 } catch (const std::runtime_error& e) { return AbortNode(state, std::string("System error while flushing: ") + e.what()); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6e20ae6d4..6237c051c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -585,6 +585,19 @@ void CWallet::ChainTipAdded(const CBlockIndex *pindex, { IncrementNoteWitnesses(pindex, pblock, sproutTree, saplingTree); UpdateSaplingNullifierNoteMapForBlock(pblock); + + // SetBestChain() can be expensive for large wallets, so do this + // at most once per hour; the wallet state will be brought up to + // date during rescanning on startup. + int64_t nNow = GetTimeMicros(); + if (nLastSetChain == 0) { + // Don't flush during startup. + nLastSetChain = nNow; + } + if (nLastSetChain + (int64_t)DATABASE_WRITE_INTERVAL * 1000000 < nNow) { + nLastSetChain = nNow; + SetBestChain(chainActive.GetLocator()); + } } void CWallet::ChainTip(const CBlockIndex *pindex, @@ -4723,20 +4736,14 @@ bool CWallet::InitLoadWallet(bool clearWitnessCaches) RegisterValidationInterface(walletInstance); - CBlockIndex *pindexRescan = chainActive.Tip(); - if (clearWitnessCaches || GetBoolArg("-rescan", false)) - { + CBlockIndex *pindexRescan = chainActive.Genesis(); + if (clearWitnessCaches || GetBoolArg("-rescan", false)) { walletInstance->ClearNoteWitnessCache(); - pindexRescan = chainActive.Genesis(); - } - else - { + } else { CWalletDB walletdb(walletFile); CBlockLocator locator; if (walletdb.ReadBestBlock(locator)) pindexRescan = FindForkInGlobalIndex(chainActive, locator); - else - pindexRescan = chainActive.Genesis(); } if (chainActive.Tip() && chainActive.Tip() != pindexRescan) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c41116119..9bf7001ae 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -766,6 +766,7 @@ private: int64_t nNextResend; int64_t nLastResend; + int64_t nLastSetChain; bool fBroadcastTransactions; template @@ -926,6 +927,7 @@ public: nOrderPosNext = 0; nNextResend = 0; nLastResend = 0; + nLastSetChain = 0; nTimeFirstKey = 0; fBroadcastTransactions = false; nWitnessCacheSize = 0;