From 51d62c652daed01770f1788458163945583c7b59 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 30 Jan 2023 23:41:31 +0000 Subject: [PATCH 1/3] qa: Add RPC test reproducing the Orchard reindex issue Co-authored-by: Daira Hopwood Co-authored-by: Kris Nuttycombe --- qa/pull-tester/rpc-tests.py | 1 + qa/rpc-tests/wallet_orchard_reindex.py | 95 ++++++++++++++++++++++++++ 2 files changed, 96 insertions(+) create mode 100755 qa/rpc-tests/wallet_orchard_reindex.py diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py index 3a703fd69..0852c4810 100755 --- a/qa/pull-tester/rpc-tests.py +++ b/qa/pull-tester/rpc-tests.py @@ -79,6 +79,7 @@ BASE_SCRIPTS= [ 'wallet_orchard_change.py', 'wallet_orchard_init.py', 'wallet_orchard_persistence.py', + 'wallet_orchard_reindex.py', 'wallet_nullifiers.py', 'wallet_sapling.py', 'wallet_sendmany_any_taddr.py', diff --git a/qa/rpc-tests/wallet_orchard_reindex.py b/qa/rpc-tests/wallet_orchard_reindex.py new file mode 100755 index 000000000..c1a3a9686 --- /dev/null +++ b/qa/rpc-tests/wallet_orchard_reindex.py @@ -0,0 +1,95 @@ +#!/usr/bin/env python3 +# Copyright (c) 2023 The Zcash developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or https://www.opensource.org/licenses/mit-license.php . + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.mininode import COIN +from test_framework.util import ( + NU5_BRANCH_ID, + assert_equal, + get_coinbase_address, + nuparams, + start_node, + start_nodes, + stop_nodes, + wait_and_assert_operationid_status, + wait_bitcoinds, +) + +from decimal import Decimal +import time + +BASE_ARGS = [ + nuparams(NU5_BRANCH_ID, 210), + '-regtestwalletsetbestchaineveryblock', +] + +# Test wallet behaviour when reindexing with Orchard state. +class WalletOrchardReindexTest(BitcoinTestFramework): + def setup_nodes(self): + return start_nodes(self.num_nodes, self.options.tmpdir, extra_args=[BASE_ARGS] * self.num_nodes) + + def run_test(self): + # Check height is before NU5. + assert_equal(self.nodes[0].getblockcount(), 200) + + # Mine blocks to activate NU5. + self.sync_all() + self.nodes[2].generate(10) + self.sync_all() + + # Get a new Orchard-only unified address + acct = self.nodes[0].z_getnewaccount()['account'] + addrRes = self.nodes[0].z_getaddressforaccount(acct, ['orchard']) + assert_equal(acct, addrRes['account']) + assert_equal(addrRes['receiver_types'], ['orchard']) + ua = addrRes['address'] + + # Create a transaction with an Orchard output to advance the Orchard + # commitment tree. + recipients = [{'address': ua, 'amount': Decimal('10')}] + myopid = self.nodes[0].z_sendmany( + get_coinbase_address(self.nodes[0]), + recipients, 1, 0, 'AllowRevealedSenders') + wait_and_assert_operationid_status(self.nodes[0], myopid) + + # Mine the transaction. + self.sync_all() + self.nodes[2].generate(1) + self.sync_all() + + # Confirm that we see the Orchard note in the wallet. + assert_equal( + {'pools': {'orchard': {'valueZat': Decimal('10') * COIN}}, 'minimum_confirmations': 1}, + self.nodes[0].z_getbalanceforaccount(acct)) + + # Mine blocks to ensure that the wallet's tip is far enough beyond NU5 + # activation that it won't rescan blocks before then on startup. + self.sync_all() + self.nodes[2].generate(9) + self.sync_all() + + # Restart the node with -reindex. + blockcount = self.nodes[0].getblockcount() + stop_nodes(self.nodes) + wait_bitcoinds() + self.num_nodes = 1 + self.nodes = [start_node(0, self.options.tmpdir, BASE_ARGS + ['-reindex'])] + + # Confirm that the reindexed node does not crash. + # Before https://github.com/zcash/zcash/issues/6004 was fixed, the node + # would crash here inside `IncrementNoteWitnesses`, unless the node was + # restarted during the `ActivateBestChain` phase of reindexing (which we + # don't do in this test). + while self.nodes[0].getblockcount() < blockcount: + time.sleep(0.1) + assert_equal(self.nodes[0].getblockcount(), blockcount) + + # Confirm that we still see the Orchard note in the wallet. + assert_equal( + {'pools': {'orchard': {'valueZat': Decimal('10') * COIN}}, 'minimum_confirmations': 1}, + self.nodes[0].z_getbalanceforaccount(acct)) + +if __name__ == '__main__': + WalletOrchardReindexTest().main() From 0d1e1ad438f491291bcabf234057105e1ec06a33 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 31 Jan 2023 00:05:30 +0000 Subject: [PATCH 2/3] Fix return type of `orchard_wallet_reset` Part of zcash/zcash#6386. --- src/rust/include/rust/orchard/wallet.h | 2 +- src/wallet/orchard.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rust/include/rust/orchard/wallet.h b/src/rust/include/rust/orchard/wallet.h index 43dfa17c0..8735f038b 100644 --- a/src/rust/include/rust/orchard/wallet.h +++ b/src/rust/include/rust/orchard/wallet.h @@ -39,7 +39,7 @@ void orchard_wallet_free(OrchardWalletPtr* wallet); * in place with the expectation that they will be overwritten and/or updated in * the rescan process. */ -bool orchard_wallet_reset(OrchardWalletPtr* wallet); +void orchard_wallet_reset(OrchardWalletPtr* wallet); /** * Checkpoint the note commitment tree. This returns `false` and leaves the note diff --git a/src/wallet/orchard.h b/src/wallet/orchard.h index 517acdf10..d3836c287 100644 --- a/src/wallet/orchard.h +++ b/src/wallet/orchard.h @@ -215,8 +215,8 @@ public: * in place with the expectation that they will be overwritten and/or updated in the * rescan process. */ - bool Reset() { - return orchard_wallet_reset(inner.get()); + void Reset() { + orchard_wallet_reset(inner.get()); } /** From 204950191cf0fc42033005ef637d8df0b95812ce Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 31 Jan 2023 00:06:13 +0000 Subject: [PATCH 3/3] Reset Orchard wallet state in `CWallet::ClearNoteWitnessCache` The general invariant in the wallet is that `CWallet::ChainTip` is only called with sequential (either connecting or disconnecting) blocks. The one exception to this is when starting `zcashd` with `-reindex`, which creates a discontinuity: the node jumps back to the genesis block, and `ThreadNotifyWallets` will similarly start notifying the wallet of the entire chain again. In Bitcoin Core, this behaviour was fine: there was no persistent cached state that couldn't just be overwritten during the re-notification. For Zcash however, wallets need to additionally maintain witnesses for notes that are spendable, and these witnesses can generally only be amended by sequential blocks. For Sprout and Sapling, the discontinuity was handled by checking if a reindex was occurring during `CWallet::InitLoadWallet`, and clearing the witness caches in `CWallet::ClearNoteWitnessCache` if so. The witnesses would then be rebuilt as the reindexed chain was re-connected during `ActivateBestChain`. The Orchard wallet stores its witnesses in a different structure on the Rust side, so it wasn't being cleared at the same time. This meant that when a full reindex was performed in one go, the sequentiality invariant would be broken once `ThreadNotifyWallets` reached NU5 activation, and the node would crash with a failed assertion (the issue at hand). However, reindexing Zcash takes a long time, and has been historically buggy for various reasons (e.g. crashing due to OOM). And due to a quirk of how the `-rescan` behaviour is implemented, if a reindexing node is restarted during its `ActivateBestChain` phase, on restart the node will almost always trigger a rescan due to the wallet chain locator not containing any hashes that match the on-start chain tip. And the first thing that the rescan logic does is check whether the start of the rescan is before NU5 activation, and reset the Orchard wallet if so. We now reset the Orchard wallet unconditionally at the same time as we clear the Sprout and Sapling witness caches. This additionally clears spentness information that the Orchard wallet is storing, but that is rebuilt during the reindex. Closes zcash/zcash#5736. Closes zcash/zcash#6004. --- src/wallet/wallet.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4ba05390a..1c251f0d9 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2550,6 +2550,11 @@ void CWallet::ClearNoteWitnessCache() } } nWitnessCacheSize = 0; + + // This resets spentness information in addition to the Orchard note witness + // caches, which is fine because it will be recovered during the reindex or + // rescan that called `ClearNoteWitnessCache()`. + orchardWallet.Reset(); } template