Auto merge of #4573 - LarryRuane:issue4301-CopyPreviousWitnesses, r=daira

Flush witness data to disk only when it's consistent

Closes #4301. Running this PR's code will not repair a data directory that has been affected by this problem; that requires starting zcashd with the `-rescan` or `-reindex` options.

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

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())

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()

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<int> 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());
     }

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)
     {

src/wallet/wallet.h

@@ -766,6 +766,7 @@ private:
 
     int64_t nNextResend;
     int64_t nLastResend;
+    int64_t nLastSetChain;
     bool fBroadcastTransactions;
 
     template <class T>
@@ -926,6 +927,7 @@ public:
         nOrderPosNext = 0;
         nNextResend = 0;
         nLastResend = 0;
+        nLastSetChain = 0;
         nTimeFirstKey = 0;
         fBroadcastTransactions = false;
         nWitnessCacheSize = 0;