From d9de6b64fc65eaddfbb1127a850b277ef9f4c733 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 31 Mar 2022 15:46:41 -0600 Subject: [PATCH 1/7] Adds a test demonstrating an Orchard wallet initialization bug. If a new Orchard wallet is created after the first Orchard spend post NU5 activation, it causes an assertion failure because the root of the wallet's empty note commitment tree does not match the global note commitment tree root. --- qa/pull-tester/rpc-tests.py | 1 + qa/rpc-tests/wallet_orchard_init.py | 118 ++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+) create mode 100755 qa/rpc-tests/wallet_orchard_init.py diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py index 1260f148a..c51c27567 100755 --- a/qa/pull-tester/rpc-tests.py +++ b/qa/pull-tester/rpc-tests.py @@ -76,6 +76,7 @@ BASE_SCRIPTS= [ 'wallet_import_export.py', 'wallet_isfromme.py', 'wallet_orchard_change.py', + 'wallet_orchard_init.py', 'wallet_orchard_persistence.py', 'wallet_nullifiers.py', 'wallet_sapling.py', diff --git a/qa/rpc-tests/wallet_orchard_init.py b/qa/rpc-tests/wallet_orchard_init.py new file mode 100755 index 000000000..d8ad830a9 --- /dev/null +++ b/qa/rpc-tests/wallet_orchard_init.py @@ -0,0 +1,118 @@ +#!/usr/bin/env python3 +# Copyright (c) 2022 The Zcash developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or https://www.opensource.org/licenses/mit-license.php . + +import os +import os.path + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + NU5_BRANCH_ID, + assert_equal, + get_coinbase_address, + nuparams, + start_nodes, + stop_nodes, + wait_bitcoinds, + wait_and_assert_operationid_status, +) + +# Test wallet behaviour with the Orchard protocol +class OrchardWalletInitTest(BitcoinTestFramework): + def __init__(self): + super().__init__() + self.num_nodes = 4 + + def setup_nodes(self): + return start_nodes(self.num_nodes, self.options.tmpdir, [[ + nuparams(NU5_BRANCH_ID, 205), + '-regtestwalletsetbestchaineveryblock' + ]] * self.num_nodes) + + def run_test(self): + # Sanity-check the test harness + assert_equal(self.nodes[0].getblockcount(), 200) + + # Get a new Orchard account on node 0 + acct0 = self.nodes[0].z_getnewaccount()['account'] + ua0 = self.nodes[0].z_getaddressforaccount(acct0, ['orchard'])['address'] + + # Activate NU5 + self.nodes[0].generate(5) + self.sync_all() + + # Get a recipient address + acct1 = self.nodes[1].z_getnewaccount()['account'] + ua1 = self.nodes[1].z_getaddressforaccount(acct1, ['orchard'])['address'] + + # Send a transaction to node 1 so that it has an Orchard note to spend. + recipients = [{"address": ua1, "amount": 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) + + self.sync_all() + self.nodes[0].generate(1) + self.sync_all() + + # Check the value sent to ua1 was received + assert_equal( + {'pools': {'orchard': {'valueZat': 10_0000_0000}}, 'minimum_confirmations': 1}, + self.nodes[1].z_getbalanceforaccount(acct1)) + + # Create an Orchard spend, so that the note commitment tree root gets altered. + recipients = [{"address": ua0, "amount": 1}] + myopid = self.nodes[1].z_sendmany(ua1, recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[1], myopid) + + self.sync_all() + self.nodes[0].generate(1) + self.sync_all() + + # Verify the balance on both nodes + assert_equal( + {'pools': {'orchard': {'valueZat': 9_0000_0000}}, 'minimum_confirmations': 1}, + self.nodes[1].z_getbalanceforaccount(acct1)) + + assert_equal( + {'pools': {'orchard': {'valueZat': 1_0000_0000}}, 'minimum_confirmations': 1}, + self.nodes[0].z_getbalanceforaccount(acct0)) + + # Shut down the network and delete node 0's wallet + stop_nodes(self.nodes) + wait_bitcoinds() + + tmpdir = self.options.tmpdir + os.remove(os.path.join(tmpdir, "node0", "regtest", "wallet.dat")) + + # Restart the network split; the split here is note necessary to reproduce the + # error but it is sufficient, and we will later use the split to check the + # corresponding rewind. + self.setup_network(True) + + assert_equal( + {'pools': {'orchard': {'valueZat': 9_0000_0000}}, 'minimum_confirmations': 1}, + self.nodes[1].z_getbalanceforaccount(acct1)) + + # Get a new account with an Orchard UA on node 0 + acct0new = self.nodes[0].z_getnewaccount()['account'] + ua0new = self.nodes[0].z_getaddressforaccount(acct0, ['orchard'])['address'] + + # Send a transaction to the Orchard account. When we mine this transaction, + # the bug causes the state of note commitment tree in the wallet to not match + # the state of the global note commitment tree. + recipients = [{"address": ua0new, "amount": 1}] + myopid = self.nodes[1].z_sendmany(ua1, recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[1], myopid) + + self.sync_all() + self.nodes[0].generate(1) + self.sync_all() + + assert_equal( + {'pools': {'orchard': {'valueZat': 1_0000_0000}}, 'minimum_confirmations': 1}, + self.nodes[0].z_getbalanceforaccount(acct0new)) + +if __name__ == '__main__': + OrchardWalletInitTest().main() + From 4afc6a37c9eb88a2ae941824bb9c72bf66fa176e Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 31 Mar 2022 15:15:42 -0600 Subject: [PATCH 2/7] Refactor ChainTip to take a struct of Merkle trees instead of a pair. This makes addition of the Orchard Merkle frontier easier in the future. --- src/validationinterface.cpp | 17 ++--- src/validationinterface.h | 9 ++- src/wallet/gtest/test_wallet.cpp | 115 ++++++++++++++----------------- src/wallet/wallet.cpp | 30 ++++---- src/wallet/wallet.h | 8 +-- src/zcbenchmarks.cpp | 14 ++-- 6 files changed, 92 insertions(+), 101 deletions(-) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 61c9d7f5a..5fed78540 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -70,12 +70,12 @@ void SyncWithWallets(const CTransaction &tx, const CBlock *pblock, const int nHe struct CachedBlockData { CBlockIndex *pindex; - std::pair oldTrees; + MerkleFrontiers oldTrees; std::list txConflicted; CachedBlockData( CBlockIndex *pindex, - std::pair oldTrees, + MerkleFrontiers oldTrees, std::list txConflicted): pindex(pindex), oldTrees(oldTrees), txConflicted(txConflicted) {} }; @@ -135,27 +135,28 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) // Iterate backwards over the connected blocks we need to notify. while (pindex && pindex != pindexFork) { + MerkleFrontiers oldFrontiers; // Get the Sprout commitment tree as of the start of this block. - SproutMerkleTree oldSproutTree; - assert(pcoinsTip->GetSproutAnchorAt(pindex->hashSproutAnchor, oldSproutTree)); + assert(pcoinsTip->GetSproutAnchorAt(pindex->hashSproutAnchor, oldFrontiers.sprout)); // Get the Sapling commitment tree as of the start of this block. // We can get this from the `hashFinalSaplingRoot` of the last block // However, this is only reliable if the last block was on or after // the Sapling activation height. Otherwise, the last anchor was the // empty root. - SaplingMerkleTree oldSaplingTree; if (chainParams.GetConsensus().NetworkUpgradeActive( pindex->pprev->nHeight, Consensus::UPGRADE_SAPLING)) { assert(pcoinsTip->GetSaplingAnchorAt( - pindex->pprev->hashFinalSaplingRoot, oldSaplingTree)); + pindex->pprev->hashFinalSaplingRoot, oldFrontiers.sapling)); } else { - assert(pcoinsTip->GetSaplingAnchorAt(SaplingMerkleTree::empty_root(), oldSaplingTree)); + assert(pcoinsTip->GetSaplingAnchorAt(SaplingMerkleTree::empty_root(), oldFrontiers.sapling)); } + // TODO: Add the Orchard frontier to oldFrontiers + blockStack.emplace_back( pindex, - std::make_pair(oldSproutTree, oldSaplingTree), + oldFrontiers, recentlyConflicted.first.at(pindex)); pindex = pindex->pprev; diff --git a/src/validationinterface.h b/src/validationinterface.h index 4dce6df3c..fbba2860a 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -23,6 +23,11 @@ class CValidationInterface; class CValidationState; class uint256; +struct MerkleFrontiers { + SproutMerkleTree sprout; + SaplingMerkleTree sapling; +}; + // These functions dispatch to one or all registered wallets /** Register a wallet to receive updates from core */ @@ -37,7 +42,7 @@ protected: virtual void UpdatedBlockTip(const CBlockIndex *pindex) {} virtual void SyncTransaction(const CTransaction &tx, const CBlock *pblock, const int nHeight) {} virtual void EraseFromWallet(const uint256 &hash) {} - virtual void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, std::optional> added) {} + virtual void ChainTip(const CBlockIndex *pindex, const CBlock *pblock, std::optional added) {} virtual void UpdatedTransaction(const uint256 &hash) {} virtual void Inventory(const uint256 &hash) {} virtual void ResendWalletTransactions(int64_t nBestBlockTime) {} @@ -59,7 +64,7 @@ struct CMainSignals { /** Notifies listeners of an updated transaction without new data (for now: a coinbase potentially becoming visible). */ boost::signals2::signal UpdatedTransaction; /** Notifies listeners of a change to the tip of the active block chain. */ - boost::signals2::signal>)> ChainTip; + boost::signals2::signal)> ChainTip; /** Notifies listeners about an inventory item being seen on the network. */ boost::signals2::signal Inventory; /** Tells listeners to broadcast their data. */ diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index 87c8dee7c..0a6fbb6a7 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -60,11 +60,10 @@ public: void IncrementNoteWitnesses(const Consensus::Params& consensus, const CBlockIndex* pindex, const CBlock* pblock, - SproutMerkleTree& sproutTree, - SaplingMerkleTree& saplingTree, + MerkleFrontiers& frontiers, bool performOrchardWalletUpdates) { CWallet::IncrementNoteWitnesses( - consensus, pindex, pblock, sproutTree, saplingTree, performOrchardWalletUpdates); + consensus, pindex, pblock, frontiers, performOrchardWalletUpdates); } @@ -96,8 +95,7 @@ std::pair CreateValidBlock(TestWallet& wallet, const libzcash::SproutSpendingKey& sk, const CBlockIndex& index, CBlock& block, - SproutMerkleTree& sproutTree, - SaplingMerkleTree& saplingTree) { + MerkleFrontiers& frontiers) { auto wtx = GetValidSproutReceive(sk, 50, true); auto note = GetSproutNote(sk, wtx, 0, 1); auto nullifier = note.nullifier(sk); @@ -111,7 +109,7 @@ std::pair CreateValidBlock(TestWallet& wallet, wallet.LoadWalletTx(wtx); block.vtx.push_back(wtx); - wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index, &block, sproutTree, saplingTree, true); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index, &block, frontiers, true); return std::make_pair(jsoutpt, saplingNotes[0]); } @@ -705,10 +703,11 @@ TEST(WalletTests, GetConflictedSaplingNotes) { // Generate note A libzcash::SaplingNote note(pk, 50000, zip_212_enabled[ver]); auto cm = note.cmu().value(); - SaplingMerkleTree saplingTree; - saplingTree.append(cm); - auto anchor = saplingTree.root(); - auto witness = saplingTree.witness(); + + MerkleFrontiers frontiers; + frontiers.sapling.append(cm); + auto anchor = frontiers.sapling.root(); + auto witness = frontiers.sapling.witness(); // Generate tx to create output note B auto builder = TransactionBuilder(consensusParams, 1, std::nullopt); @@ -719,7 +718,6 @@ TEST(WalletTests, GetConflictedSaplingNotes) { // Fake-mine the transaction EXPECT_EQ(-1, chainActive.Height()); - SproutMerkleTree sproutTree; CBlock block; block.vtx.push_back(wtx); block.hashMerkleRoot = block.BuildMerkleTree(); @@ -738,7 +736,7 @@ TEST(WalletTests, GetConflictedSaplingNotes) { wallet.LoadWalletTx(wtx); // Simulate receiving new block and ChainTip signal - wallet.IncrementNoteWitnesses(Params().GetConsensus(),&fakeIndex, &block, sproutTree, saplingTree, true); + wallet.IncrementNoteWitnesses(Params().GetConsensus(),&fakeIndex, &block, frontiers, true); wallet.UpdateSaplingNullifierNoteMapForBlock(&block); // Retrieve the updated wtx from wallet @@ -764,7 +762,7 @@ TEST(WalletTests, GetConflictedSaplingNotes) { ASSERT_EQ(static_cast(maybe_nf), true); auto nullifier2 = maybe_nf.value(); - anchor = saplingTree.root(); + anchor = frontiers.sapling.root(); // Create transaction to spend note B auto builder2 = TransactionBuilder(consensusParams, 2, std::nullopt); @@ -844,8 +842,7 @@ TEST(WalletTests, GetConflictedOrchardNotes) { CWalletTx wtx {&wallet, tx}; // Fake-mine the transaction - SproutMerkleTree sproutTree; - SaplingMerkleTree saplingTree; + MerkleFrontiers frontiers; OrchardMerkleFrontier orchardTree; orchardTree.AppendBundle(wtx.GetOrchardBundle()); @@ -870,7 +867,7 @@ TEST(WalletTests, GetConflictedOrchardNotes) { wallet.LoadWalletTx(wtx); // Simulate receiving new block and ChainTip signal - wallet.IncrementNoteWitnesses(Params().GetConsensus(),&fakeIndex, &block, sproutTree, saplingTree, true); + wallet.IncrementNoteWitnesses(Params().GetConsensus(),&fakeIndex, &block, frontiers, true); // Fetch the Orchard note so we can spend it. std::vector sproutEntries; @@ -1103,12 +1100,13 @@ TEST(WalletTests, NavigateFromSaplingNullifierToNote) { ASSERT_TRUE(nf); uint256 nullifier = nf.value(); + MerkleFrontiers frontiers = { .sapling = testNote.tree }; + // Verify dummy note is unspent EXPECT_FALSE(wallet.IsSaplingSpent(nullifier)); // Fake-mine the transaction EXPECT_EQ(-1, chainActive.Height()); - SproutMerkleTree sproutTree; CBlock block; block.vtx.push_back(wtx); block.hashMerkleRoot = block.BuildMerkleTree(); @@ -1138,7 +1136,7 @@ TEST(WalletTests, NavigateFromSaplingNullifierToNote) { } // Simulate receiving new block and ChainTip signal - wallet.IncrementNoteWitnesses(Params().GetConsensus(), &fakeIndex, &block, sproutTree, testNote.tree, true); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &fakeIndex, &block, frontiers, true); wallet.UpdateSaplingNullifierNoteMapForBlock(&block); // Retrieve the updated wtx from wallet @@ -1221,10 +1219,10 @@ TEST(WalletTests, SpentSaplingNoteIsFromMe) { // Generate Sapling note A libzcash::SaplingNote note(pk, 50000, zip_212_enabled[ver]); auto cm = note.cmu().value(); - SaplingMerkleTree saplingTree; - saplingTree.append(cm); - auto anchor = saplingTree.root(); - auto witness = saplingTree.witness(); + MerkleFrontiers frontiers; + frontiers.sapling.append(cm); + auto anchor = frontiers.sapling.root(); + auto witness = frontiers.sapling.witness(); // Generate transaction, which sends funds to note B auto builder = TransactionBuilder(consensusParams, 1, std::nullopt); @@ -1238,7 +1236,6 @@ TEST(WalletTests, SpentSaplingNoteIsFromMe) { // Fake-mine the transaction EXPECT_EQ(-1, chainActive.Height()); - SproutMerkleTree sproutTree; CBlock block; block.vtx.push_back(wtx); block.hashMerkleRoot = block.BuildMerkleTree(); @@ -1258,7 +1255,7 @@ TEST(WalletTests, SpentSaplingNoteIsFromMe) { // Simulate receiving new block and ChainTip signal. // This triggers calculation of nullifiers for notes belonging to this wallet // in the output descriptions of wtx. - wallet.IncrementNoteWitnesses(Params().GetConsensus(), &fakeIndex, &block, sproutTree, saplingTree, true); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &fakeIndex, &block, frontiers, true); wallet.UpdateSaplingNullifierNoteMapForBlock(&block); // Retrieve the updated wtx from wallet @@ -1296,7 +1293,7 @@ TEST(WalletTests, SpentSaplingNoteIsFromMe) { // NOTE: Not updating the anchor results in a core dump. Shouldn't builder just return error? // *** Error in `./zcash-gtest': double free or corruption (out): 0x00007ffd8755d990 *** - anchor = saplingTree.root(); + anchor = frontiers.sapling.root(); // Create transaction to spend note B auto builder2 = TransactionBuilder(consensusParams, 2, std::nullopt); @@ -1393,9 +1390,8 @@ TEST(WalletTests, CachedWitnessesEmptyChain) { CBlock block; block.vtx.push_back(wtx); CBlockIndex index(block); - SproutMerkleTree sproutTree; - SaplingMerkleTree saplingTree; - wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index, &block, sproutTree, saplingTree, true); + MerkleFrontiers frontiers; + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index, &block, frontiers, true); ::GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); @@ -1415,8 +1411,7 @@ TEST(WalletTests, CachedWitnessesChainTip) { std::pair anchors1; CBlock block1; - SproutMerkleTree sproutTree; - SaplingMerkleTree saplingTree; + MerkleFrontiers frontiers; auto sk = libzcash::SproutSpendingKey::random(); wallet.AddSproutSpendingKey(sk); @@ -1425,7 +1420,7 @@ TEST(WalletTests, CachedWitnessesChainTip) { // First block (case tested in _empty_chain) CBlockIndex index1(block1); index1.nHeight = 1; - auto outpts = CreateValidBlock(wallet, sk, index1, block1, sproutTree, saplingTree); + auto outpts = CreateValidBlock(wallet, sk, index1, block1, frontiers); // Called to fetch anchor std::vector sproutNotes {outpts.first}; @@ -1466,9 +1461,8 @@ TEST(WalletTests, CachedWitnessesChainTip) { block2.vtx.push_back(wtx); CBlockIndex index2(block2); index2.nHeight = 2; - SproutMerkleTree sproutTree2 {sproutTree}; - SaplingMerkleTree saplingTree2 {saplingTree}; - wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index2, &block2, sproutTree2, saplingTree2, true); + MerkleFrontiers frontiers2 = { .sprout = frontiers.sprout, .sapling = frontiers.sapling }; + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index2, &block2, frontiers2, true); auto anchors2 = GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); EXPECT_NE(anchors2.first, anchors2.second); @@ -1489,7 +1483,7 @@ TEST(WalletTests, CachedWitnessesChainTip) { EXPECT_NE(anchors1.second, anchors3.second); // Re-incrementing with the same block should give the same result - wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index2, &block2, sproutTree, saplingTree, true); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index2, &block2, frontiers, true); auto anchors4 = GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); EXPECT_NE(anchors4.first, anchors4.second); @@ -1499,7 +1493,7 @@ TEST(WalletTests, CachedWitnessesChainTip) { EXPECT_EQ(anchors2.second, anchors4.second); // Incrementing with the same block again should not change the cache - wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index2, &block2, sproutTree, saplingTree, true); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index2, &block2, frontiers, true); std::vector> sproutWitnesses5; std::vector> saplingWitnesses5; @@ -1518,8 +1512,7 @@ TEST(WalletTests, CachedWitnessesDecrementFirst) { TestWallet wallet(Params()); LOCK(wallet.cs_wallet); - SproutMerkleTree sproutTree; - SaplingMerkleTree saplingTree; + MerkleFrontiers frontiers; auto sk = libzcash::SproutSpendingKey::random(); wallet.AddSproutSpendingKey(sk); @@ -1529,7 +1522,7 @@ TEST(WalletTests, CachedWitnessesDecrementFirst) { CBlock block1; CBlockIndex index1(block1); index1.nHeight = 1; - CreateValidBlock(wallet, sk, index1, block1, sproutTree, saplingTree); + CreateValidBlock(wallet, sk, index1, block1, frontiers); } std::pair anchors2; @@ -1539,7 +1532,7 @@ TEST(WalletTests, CachedWitnessesDecrementFirst) { { // Second block (case tested in _chain_tip) index2.nHeight = 2; - auto outpts = CreateValidBlock(wallet, sk, index2, block2, sproutTree, saplingTree); + auto outpts = CreateValidBlock(wallet, sk, index2, block2, frontiers); // Called to fetch anchor std::vector sproutNotes {outpts.first}; @@ -1585,7 +1578,7 @@ TEST(WalletTests, CachedWitnessesDecrementFirst) { EXPECT_NE(anchors2.second, anchors4.second); // Re-incrementing with the same block should give the same result - wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index2, &block2, sproutTree, saplingTree, true); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index2, &block2, frontiers, true); auto anchors5 = GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); @@ -1607,10 +1600,8 @@ TEST(WalletTests, CachedWitnessesCleanIndex) { std::vector saplingNotes; std::vector sproutAnchors; std::vector saplingAnchors; - SproutMerkleTree sproutTree; - SproutMerkleTree sproutRiTree = sproutTree; - SaplingMerkleTree saplingTree; - SaplingMerkleTree saplingRiTree = saplingTree; + MerkleFrontiers frontiers; + MerkleFrontiers riFrontiers = { .sprout = frontiers.sprout, .sapling = frontiers.sapling }; std::vector> sproutWitnesses; std::vector> saplingWitnesses; @@ -1623,11 +1614,11 @@ TEST(WalletTests, CachedWitnessesCleanIndex) { indices.resize(numBlocks); for (size_t i = 0; i < numBlocks; i++) { indices[i].nHeight = i; - auto oldSproutRoot = sproutTree.root(); - auto oldSaplingRoot = saplingTree.root(); - auto outpts = CreateValidBlock(wallet, sk, indices[i], blocks[i], sproutTree, saplingTree); - EXPECT_NE(oldSproutRoot, sproutTree.root()); - EXPECT_NE(oldSaplingRoot, saplingTree.root()); + auto oldSproutRoot = frontiers.sprout.root(); + auto oldSaplingRoot = frontiers.sapling.root(); + auto outpts = CreateValidBlock(wallet, sk, indices[i], blocks[i], frontiers); + EXPECT_NE(oldSproutRoot, frontiers.sprout.root()); + EXPECT_NE(oldSaplingRoot, frontiers.sapling.root()); sproutNotes.push_back(outpts.first); saplingNotes.push_back(outpts.second); @@ -1643,9 +1634,8 @@ TEST(WalletTests, CachedWitnessesCleanIndex) { // Now pretend we are reindexing: the chain is cleared, and each block is // used to increment witnesses again. for (size_t i = 0; i < numBlocks; i++) { - SproutMerkleTree sproutRiPrevTree {sproutRiTree}; - SaplingMerkleTree saplingRiPrevTree {saplingRiTree}; - wallet.IncrementNoteWitnesses(Params().GetConsensus(), &(indices[i]), &(blocks[i]), sproutRiTree, saplingRiTree, true); + MerkleFrontiers riPrevFrontiers{riFrontiers}; + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &(indices[i]), &(blocks[i]), riFrontiers, true); auto anchors = GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); for (size_t j = 0; j < numBlocks; j++) { @@ -1672,7 +1662,7 @@ TEST(WalletTests, CachedWitnessesCleanIndex) { } { - wallet.IncrementNoteWitnesses(Params().GetConsensus(), &(indices[i]), &(blocks[i]), sproutRiPrevTree, saplingRiPrevTree, true); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &(indices[i]), &(blocks[i]), riPrevFrontiers, true); auto anchors = GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); for (size_t j = 0; j < numBlocks; j++) { EXPECT_TRUE((bool) sproutWitnesses[j]); @@ -2059,6 +2049,8 @@ TEST(WalletTests, UpdatedSaplingNoteData) { builder.AddSaplingOutput(extfvk.fvk.ovk, pa2, 25000, {}); auto tx = builder.Build().GetTxOrThrow(); + MerkleFrontiers frontiers = { .sapling = testNote.tree }; + // Wallet contains extfvk1 but not extfvk2 CWalletTx wtx {&wallet, tx}; ASSERT_TRUE(wallet.AddSaplingZKey(sk)); @@ -2067,7 +2059,6 @@ TEST(WalletTests, UpdatedSaplingNoteData) { // Fake-mine the transaction EXPECT_EQ(-1, chainActive.Height()); - SproutMerkleTree sproutTree; CBlock block; block.vtx.push_back(wtx); block.hashMerkleRoot = block.BuildMerkleTree(); @@ -2086,7 +2077,7 @@ TEST(WalletTests, UpdatedSaplingNoteData) { wallet.LoadWalletTx(wtx); // Simulate receiving new block and ChainTip signal - wallet.IncrementNoteWitnesses(Params().GetConsensus(), &fakeIndex, &block, sproutTree, testNote.tree, true); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &fakeIndex, &block, frontiers, true); wallet.UpdateSaplingNullifierNoteMapForBlock(&block); // Retrieve the updated wtx from wallet @@ -2104,7 +2095,7 @@ TEST(WalletTests, UpdatedSaplingNoteData) { // The payment note has not been witnessed yet, so let's fake the witness. SaplingOutPoint sop0(wtx2.GetHash(), 0); SaplingOutPoint sop1(wtx2.GetHash(), 1); - wtx2.mapSaplingNoteData[sop0].witnesses.push_front(testNote.tree.witness()); + wtx2.mapSaplingNoteData[sop0].witnesses.push_front(frontiers.sapling.witness()); wtx2.mapSaplingNoteData[sop0].witnessHeight = 0; // The txs are different as wtx is aware of just the change output, @@ -2132,7 +2123,7 @@ TEST(WalletTests, UpdatedSaplingNoteData) { EXPECT_EQ(wtx.mapSaplingNoteData[sop0].witnesses.front(), wtx2.mapSaplingNoteData[sop0].witnesses.front()); // wtx2 never had its change output witnessed even though it has been in wtx EXPECT_EQ(0, wtx2.mapSaplingNoteData[sop1].witnesses.size()); - EXPECT_EQ(wtx.mapSaplingNoteData[sop1].witnesses.front(), testNote.tree.witness()); + EXPECT_EQ(wtx.mapSaplingNoteData[sop1].witnesses.front(), frontiers.sapling.witness()); // Tear down chainActive.SetTip(NULL); @@ -2215,7 +2206,7 @@ TEST(WalletTests, MarkAffectedSaplingTransactionsDirty) { // Fake-mine the transaction EXPECT_EQ(-1, chainActive.Height()); - SaplingMerkleTree saplingTree; + MerkleFrontiers frontiers; SproutMerkleTree sproutTree; CBlock block; block.vtx.push_back(wtx); @@ -2235,7 +2226,7 @@ TEST(WalletTests, MarkAffectedSaplingTransactionsDirty) { wallet.LoadWalletTx(wtx); // Simulate receiving new block and ChainTip signal - wallet.IncrementNoteWitnesses(Params().GetConsensus(), &fakeIndex, &block, sproutTree, saplingTree, true); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &fakeIndex, &block, frontiers, true); wallet.UpdateSaplingNullifierNoteMapForBlock(&block); // Retrieve the updated wtx from wallet @@ -2248,8 +2239,8 @@ TEST(WalletTests, MarkAffectedSaplingTransactionsDirty) { auto maybe_note = maybe_pt.value().note(ivk); ASSERT_EQ(static_cast(maybe_note), true); auto note = maybe_note.value(); - auto anchor = saplingTree.root(); - auto witness = saplingTree.witness(); + auto anchor = frontiers.sapling.root(); + auto witness = frontiers.sapling.witness(); // Create a Sapling-only transaction // 0.0004 z-ZEC in, 0.00025 z-ZEC out, default fee, 0.00005 z-ZEC change diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c80e19cc4..9b1a87129 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1392,15 +1392,14 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, void CWallet::ChainTipAdded(const CBlockIndex *pindex, const CBlock *pblock, - SproutMerkleTree sproutTree, - SaplingMerkleTree saplingTree, + MerkleFrontiers frontiers, bool performOrchardWalletUpdates) { const auto chainParams = Params(); IncrementNoteWitnesses( chainParams.GetConsensus(), pindex, pblock, - sproutTree, saplingTree, performOrchardWalletUpdates); + frontiers, performOrchardWalletUpdates); UpdateSaplingNullifierNoteMapForBlock(pblock); // SetBestChain() can be expensive for large wallets, so do only @@ -1432,11 +1431,11 @@ void CWallet::ChainTip(const CBlockIndex *pindex, const CBlock *pblock, // If this is None, it indicates a rollback and we will decrement the // witnesses / rewind the tree - std::optional> added) + std::optional added) { const auto& consensus = Params().GetConsensus(); if (added.has_value()) { - ChainTipAdded(pindex, pblock, added->first, added->second, true); + ChainTipAdded(pindex, pblock, added.value(), true); // Prevent migration transactions from being created when node is syncing after launch, // and also when node wakes up from suspension/hibernation and incoming blocks are old. if (!IsInitialBlockDownload(consensus) && @@ -2600,8 +2599,7 @@ void CWallet::IncrementNoteWitnesses( const Consensus::Params& consensus, const CBlockIndex* pindex, const CBlock* pblockIn, - SproutMerkleTree& sproutTree, - SaplingMerkleTree& saplingTree, + MerkleFrontiers& frontiers, bool performOrchardWalletUpdates) { LOCK(cs_wallet); @@ -2633,7 +2631,7 @@ void CWallet::IncrementNoteWitnesses( const JSDescription& jsdesc = tx.vJoinSplit[i]; for (uint8_t j = 0; j < jsdesc.commitments.size(); j++) { const uint256& note_commitment = jsdesc.commitments[j]; - sproutTree.append(note_commitment); + frontiers.sprout.append(note_commitment); // Increment existing witnesses for (std::pair& wtxItem : mapWallet) { @@ -2643,14 +2641,14 @@ void CWallet::IncrementNoteWitnesses( // If this is our note, witness it if (txIsOurs) { JSOutPoint jsoutpt {hash, i, j}; - ::WitnessNoteIfMine(mapWallet[hash].mapSproutNoteData, pindex->nHeight, nWitnessCacheSize, jsoutpt, sproutTree.witness()); + ::WitnessNoteIfMine(mapWallet[hash].mapSproutNoteData, pindex->nHeight, nWitnessCacheSize, jsoutpt, frontiers.sprout.witness()); } } } // Sapling for (uint32_t i = 0; i < tx.vShieldedOutput.size(); i++) { const uint256& note_commitment = tx.vShieldedOutput[i].cmu; - saplingTree.append(note_commitment); + frontiers.sapling.append(note_commitment); // Increment existing witnesses for (std::pair& wtxItem : mapWallet) { @@ -2660,13 +2658,14 @@ void CWallet::IncrementNoteWitnesses( // If this is our note, witness it if (txIsOurs) { SaplingOutPoint outPoint {hash, i}; - ::WitnessNoteIfMine(mapWallet[hash].mapSaplingNoteData, pindex->nHeight, nWitnessCacheSize, outPoint, saplingTree.witness()); + ::WitnessNoteIfMine(mapWallet[hash].mapSaplingNoteData, pindex->nHeight, nWitnessCacheSize, outPoint, frontiers.sapling.witness()); } } } // If we're at or beyond NU5 activation, update the Orchard note commitment tree. if (performOrchardWalletUpdates && consensus.NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_NU5)) { + assert(orchardWallet.AppendNoteCommitments(pindex->nHeight, *pblock)); assert(pindex->hashFinalOrchardRoot == orchardWallet.GetLatestAnchor()); } @@ -4389,18 +4388,17 @@ int CWallet::ScanForWalletTransactions( } } - SproutMerkleTree sproutTree; - SaplingMerkleTree saplingTree; + MerkleFrontiers frontiers; // This should never fail: we should always be able to get the tree // state on the path to the tip of our chain - assert(pcoinsTip->GetSproutAnchorAt(pindex->hashSproutAnchor, sproutTree)); + assert(pcoinsTip->GetSproutAnchorAt(pindex->hashSproutAnchor, frontiers.sprout)); if (pindex->pprev) { if (consensus.NetworkUpgradeActive(pindex->pprev->nHeight, Consensus::UPGRADE_SAPLING)) { - assert(pcoinsTip->GetSaplingAnchorAt(pindex->pprev->hashFinalSaplingRoot, saplingTree)); + assert(pcoinsTip->GetSaplingAnchorAt(pindex->pprev->hashFinalSaplingRoot, frontiers.sapling)); } } // Increment note witness caches - ChainTipAdded(pindex, &block, sproutTree, saplingTree, performOrchardWalletUpdates); + ChainTipAdded(pindex, &block, frontiers, performOrchardWalletUpdates); pindex = chainActive.Next(pindex); if (GetTime() >= nNow + 60) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index e38161a67..d6745aed1 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1086,8 +1086,7 @@ protected: const Consensus::Params& consensus, const CBlockIndex* pindex, const CBlock* pblock, - SproutMerkleTree& sproutTree, - SaplingMerkleTree& saplingTree, + MerkleFrontiers& frontiers, bool performOrchardWalletUpdates ); /** @@ -1158,8 +1157,7 @@ private: void ChainTipAdded( const CBlockIndex *pindex, const CBlock *pblock, - SproutMerkleTree sproutTree, - SaplingMerkleTree saplingTree, + MerkleFrontiers frontiers, bool performOrchardWalletUpdates); /* Add a transparent secret key to the wallet. Internal use only. */ @@ -1806,7 +1804,7 @@ public: void ChainTip( const CBlockIndex *pindex, const CBlock *pblock, - std::optional> added); + std::optional added); void RunSaplingMigration(int blockHeight); void AddPendingSaplingMigrationTx(const CTransaction& tx); /** Saves witness caches and best block locator to disk. */ diff --git a/src/zcbenchmarks.cpp b/src/zcbenchmarks.cpp index fb3009eca..65dfec8ad 100644 --- a/src/zcbenchmarks.cpp +++ b/src/zcbenchmarks.cpp @@ -335,8 +335,7 @@ double benchmark_increment_sprout_note_witnesses(size_t nTxs) const Consensus::Params& consensusParams = Params().GetConsensus(); CWallet wallet(Params()); - SproutMerkleTree sproutTree; - SaplingMerkleTree saplingTree; + MerkleFrontiers frontiers; auto sproutSpendingKey = libzcash::SproutSpendingKey::random(); wallet.AddSproutSpendingKey(sproutSpendingKey); @@ -353,7 +352,7 @@ double benchmark_increment_sprout_note_witnesses(size_t nTxs) index1.nHeight = 1; // Increment to get transactions witnessed - wallet.ChainTip(&index1, &block1, std::make_pair(sproutTree, saplingTree)); + wallet.ChainTip(&index1, &block1, frontiers); // Second block CBlock block2; @@ -369,7 +368,7 @@ double benchmark_increment_sprout_note_witnesses(size_t nTxs) struct timeval tv_start; timer_start(tv_start); - wallet.ChainTip(&index2, &block2, std::make_pair(sproutTree, saplingTree)); + wallet.ChainTip(&index2, &block2, frontiers); return timer_stop(tv_start); } @@ -397,8 +396,7 @@ double benchmark_increment_sapling_note_witnesses(size_t nTxs) const Consensus::Params& consensusParams = Params().GetConsensus(); CWallet wallet(Params()); - SproutMerkleTree sproutTree; - SaplingMerkleTree saplingTree; + MerkleFrontiers frontiers; auto saplingSpendingKey = GetTestMasterSaplingSpendingKey(); wallet.AddSaplingSpendingKey(saplingSpendingKey); @@ -415,7 +413,7 @@ double benchmark_increment_sapling_note_witnesses(size_t nTxs) index1.nHeight = 1; // Increment to get transactions witnessed - wallet.ChainTip(&index1, &block1, std::make_pair(sproutTree, saplingTree)); + wallet.ChainTip(&index1, &block1, frontiers); // Second block CBlock block2; @@ -431,7 +429,7 @@ double benchmark_increment_sapling_note_witnesses(size_t nTxs) struct timeval tv_start; timer_start(tv_start); - wallet.ChainTip(&index2, &block2, std::make_pair(sproutTree, saplingTree)); + wallet.ChainTip(&index2, &block2, frontiers); return timer_stop(tv_start); } From 6fbcba641dd9104a0cfd2d26da638544ebd7fc44 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 31 Mar 2022 15:15:42 -0600 Subject: [PATCH 3/7] Fix a bug in initialization of the Orchard wallet after NU5 activation. When initializing a new Orchard wallet after NU5 activation, it is not valid to start from the empty note commitment tree; instead, the note commitment tree needs to be initialized from the state of the global Orchard Merkle frontier. In addition, this change necessitates a change to how rewinds work, such that in a rollback scenario with a newly initialized wallet that does not have sufficient checkpoints to fully satisfy a requested rewind, the rewind is allowed to proceed so long as it does not invalidate any persisted witness data. --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/rust/include/rust/orchard/wallet.h | 21 +++-- src/rust/src/wallet.rs | 117 +++++++++++++++++-------- src/validationinterface.cpp | 14 ++- src/validationinterface.h | 1 + src/wallet/orchard.h | 15 +++- src/wallet/wallet.cpp | 18 ++-- src/zcash/IncrementalMerkleTree.hpp | 4 + 9 files changed, 141 insertions(+), 53 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2cf0ecbf3..5b8aadd83 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -855,7 +855,7 @@ dependencies = [ [[package]] name = "incrementalmerkletree" version = "0.3.0-beta.1" -source = "git+https://github.com/nuttycom/incrementalmerkletree?rev=027a98e53ad59699662d258ac22cd2095ae02aea#027a98e53ad59699662d258ac22cd2095ae02aea" +source = "git+https://github.com/nuttycom/incrementalmerkletree?rev=f19cf9c381d70547d170216f844b62c86befb6c3#f19cf9c381d70547d170216f844b62c86befb6c3" dependencies = [ "serde", ] diff --git a/Cargo.toml b/Cargo.toml index 0e0b3bf60..a0e6bbcb1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -87,7 +87,7 @@ codegen-units = 1 [patch.crates-io] hdwallet = { git = "https://github.com/nuttycom/hdwallet", rev = "576683b9f2865f1118c309017ff36e01f84420c9" } -incrementalmerkletree = { git = "https://github.com/nuttycom/incrementalmerkletree", rev = "027a98e53ad59699662d258ac22cd2095ae02aea" } +incrementalmerkletree = { git = "https://github.com/nuttycom/incrementalmerkletree", rev = "f19cf9c381d70547d170216f844b62c86befb6c3" } zcash_address = { git = "https://github.com/zcash/librustzcash.git", rev = "9c1ed86c5aa8ae3b6d6dcc1478f2d6ba1264488f" } zcash_encoding = { git = "https://github.com/zcash/librustzcash.git", rev = "9c1ed86c5aa8ae3b6d6dcc1478f2d6ba1264488f" } zcash_history = { git = "https://github.com/zcash/librustzcash.git", rev = "9c1ed86c5aa8ae3b6d6dcc1478f2d6ba1264488f" } diff --git a/src/rust/include/rust/orchard/wallet.h b/src/rust/include/rust/orchard/wallet.h index a65962878..dae875dab 100644 --- a/src/rust/include/rust/orchard/wallet.h +++ b/src/rust/include/rust/orchard/wallet.h @@ -5,6 +5,7 @@ #ifndef ZCASH_RUST_INCLUDE_RUST_ORCHARD_WALLET_H #define ZCASH_RUST_INCLUDE_RUST_ORCHARD_WALLET_H +#include "rust/orchard/incremental_merkle_tree.h" #include "rust/orchard/keys.h" #include "rust/builder.h" @@ -65,19 +66,27 @@ bool orchard_wallet_get_last_checkpoint( * * The `blockHeight` argument provides the height to which the witness tree should be * rewound, such that after the rewind this height corresponds to the latest block - * appended to the tree. The number of blocks that were removed from the witness - * tree in the rewind process is returned via `blocksRewoundRet`. + * appended to the tree. * - * Returns `true` if the rewind is successful, in which case the number of blocks that were - * removed from the witness tree in the rewind process is returned via `blocksRewoundRet`; - * this returns `false` and leaves `blocksRewoundRet` unmodified. + * Returns `true` if the rewind is successful, in which case `resultHeight` will contain + * the height to which the tree has been rewound; otherwise, this returns `false` and + * leaves `resultHeight` unmodified. */ bool orchard_wallet_rewind( OrchardWalletPtr* wallet, uint32_t blockHeight, - uint32_t* blocksRewoundRet + uint32_t* resultHeight ); +/** + * Initialize the wallet's note commitment tree to the empty tree starting from the + * specified Merkle frontier. This will return `false` and leave the wallet unmodified if + * it would cause any checkpoint or witness state to be invalidated. + */ +bool orchard_wallet_init_from_frontier( + OrchardWalletPtr* wallet, + const OrchardMerkleFrontierPtr* frontier); + /** * A C struct used to transfer action_idx/IVK pairs back from Rust across the FFI * boundary. This must have the same in-memory representation as the `FFIActionIVK` type diff --git a/src/rust/src/wallet.rs b/src/rust/src/wallet.rs index 765390e53..858ff4997 100644 --- a/src/rust/src/wallet.rs +++ b/src/rust/src/wallet.rs @@ -1,5 +1,5 @@ use byteorder::{LittleEndian, ReadBytesExt, WriteBytesExt}; -use incrementalmerkletree::{bridgetree::BridgeTree, Frontier, Position, Tree}; +use incrementalmerkletree::{bridgetree, bridgetree::BridgeTree, Frontier, Position, Tree}; use libc::c_uchar; use std::collections::{BTreeMap, BTreeSet}; use std::convert::TryInto; @@ -266,47 +266,45 @@ impl Wallet { self.last_checkpoint } - /// Rewinds the note commitment tree to the given height, removes notes and - /// spentness information for transactions mined in the removed blocks, and returns - /// the number of blocks by which the tree has been rewound if successful. Returns - /// `RewindError` if not enough checkpoints exist to execute the full rewind - /// requested. If the requested height is greater than or equal to the height of the - /// latest checkpoint, this returns `Ok(0)` and leaves the wallet unmodified. - pub fn rewind(&mut self, to_height: BlockHeight) -> Result { + /// Rewinds the note commitment tree to the given height, removes notes and spentness + /// information for transactions mined in the removed blocks, and returns the height to which + /// the tree has been rewound if successful. Returns `RewindError` if not enough checkpoints + /// exist to execute the full rewind requested and the wallet has witness information that + /// would be invalidated by the rewind. If the requested height is greater than or equal to the + /// height of the latest checkpoint, this returns a successful result containing the height of + /// the last checkpoint. + /// + /// In the case that no checkpoints exist but the note commitment tree also records no witness + /// information, we allow the wallet to continue to rewind, under the assumption that the state + /// of the note commitment tree will be overwritten prior to the next append. + pub fn rewind(&mut self, to_height: BlockHeight) -> Result { if let Some(checkpoint_height) = self.last_checkpoint { if to_height >= checkpoint_height { - return Ok(0); + return Ok(checkpoint_height); } let blocks_to_rewind = ::from(checkpoint_height) - ::from(to_height); - - // if the rewind length exceeds the number of checkpoints we have stored, - // return failure. let checkpoint_count = self.witness_tree.checkpoints().len(); - if blocks_to_rewind as usize > checkpoint_count { - return Err(RewindError::InsufficientCheckpoints(checkpoint_count)); - } - for _ in 0..blocks_to_rewind { - // we've already checked that we have enough checkpoints to fully - // rewind by the requested amount. - assert!(self.witness_tree.rewind()); + // If the rewind fails, we have no more checkpoints. This is fine in the + // case that we have a recently-initialized tree, so long as we have no + // witnessed indices. In the case that we have any witnessed notes, we + // have hit the maximum rewind limit, and this is an error. + if !self.witness_tree.rewind() { + assert!(self.witness_tree.checkpoints().is_empty()); + if !self.witness_tree.witnessed_indices().is_empty() { + return Err(RewindError::InsufficientCheckpoints(checkpoint_count)); + } + } } - // reset our last observed height to ensure that notes added in the future are - // from a new block - let last_observed = LastObserved { - block_height: checkpoint_height - blocks_to_rewind, - block_tx_idx: None, - }; - // retain notes that correspond to transactions that are not "un-mined" after // the rewind let to_retain: BTreeSet<_> = self .wallet_note_positions .iter() .filter_map(|(txid, n)| { - if n.tx_height <= last_observed.block_height { + if n.tx_height <= to_height { Some(*txid) } else { None @@ -315,21 +313,39 @@ impl Wallet { .collect(); self.mined_notes.retain(|_, v| to_retain.contains(&v.txid)); + // nullifier and received note data are retained, because these values are stable // once we've observed a note for the first time. The block height at which we // observed the note is removed along with the note positions, because the // transaction will no longer have been observed as having been mined. self.wallet_note_positions .retain(|txid, _| to_retain.contains(txid)); - self.last_observed = Some(last_observed); + + // reset our last observed height to ensure that notes added in the future are + // from a new block + self.last_observed = Some(LastObserved { + block_height: to_height, + block_tx_idx: None, + }); + self.last_checkpoint = if checkpoint_count > blocks_to_rewind as usize { - Some(checkpoint_height - blocks_to_rewind) + Some(to_height) } else { - // checkpoint_count == blocks_to_rewind + // checkpoint_count <= blocks_to_rewind None }; - Ok(blocks_to_rewind) + Ok(to_height) + } else if self.witness_tree.witnessed_indices().is_empty() { + // If we have no witnessed notes, it's okay to keep "rewinding" even though + // we have no checkpoints. We then allow last_observed to assume the height + // to which we have reset the tree state. + self.last_observed = Some(LastObserved { + block_height: to_height, + block_tx_idx: None, + }); + + Ok(to_height) } else { Err(RewindError::InsufficientCheckpoints(0)) } @@ -728,14 +744,14 @@ pub extern "C" fn orchard_wallet_get_last_checkpoint( pub extern "C" fn orchard_wallet_rewind( wallet: *mut Wallet, to_height: BlockHeight, - blocks_rewound: *mut u32, + result_height: *mut BlockHeight, ) -> bool { let wallet = unsafe { wallet.as_mut() }.expect("Wallet pointer may not be null"); - let blocks_rewound = - unsafe { blocks_rewound.as_mut() }.expect("Return value pointer may not be null."); + let result_height = + unsafe { result_height.as_mut() }.expect("Return value pointer may not be null."); match wallet.rewind(to_height) { - Ok(rewound) => { - *blocks_rewound = rewound; + Ok(result) => { + *result_height = result; true } Err(e) => { @@ -1273,3 +1289,32 @@ pub extern "C" fn orchard_wallet_load_note_commitment_tree( } } } + +#[no_mangle] +pub extern "C" fn orchard_wallet_init_from_frontier( + wallet: *mut Wallet, + frontier: *const bridgetree::Frontier, +) -> bool { + let wallet = unsafe { wallet.as_mut() }.expect("Wallet pointer may not be null."); + let frontier = unsafe { frontier.as_ref() }.expect("Wallet pointer may not be null."); + + if wallet.witness_tree.checkpoints().is_empty() + && wallet.witness_tree.witnessed_indices().is_empty() + { + wallet.witness_tree = frontier.value().map_or_else( + || BridgeTree::new(MAX_CHECKPOINTS), + |nonempty_frontier| { + BridgeTree::from_frontier(MAX_CHECKPOINTS, nonempty_frontier.clone()) + }, + ); + true + } else { + // if we have any checkpoints in the tree, or if we have any witnessed notes, + // don't allow reinitialization + error!( + "Invalid attempt to reinitialize note commitment tree: {} checkpoints present.", + wallet.witness_tree.checkpoints().len() + ); + false + } +} diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 5fed78540..9cd0b15fb 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -152,7 +152,19 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) assert(pcoinsTip->GetSaplingAnchorAt(SaplingMerkleTree::empty_root(), oldFrontiers.sapling)); } - // TODO: Add the Orchard frontier to oldFrontiers + // Get the Orchard Merkle frontier as of the start of this block. + // We can get this from the `hashFinalOrchardRoot` of the last block + // However, this is only reliable if the last block was on or after + // the Orchard activation height. Otherwise, the last anchor was the + // empty root. + if (chainParams.GetConsensus().NetworkUpgradeActive( + pindex->pprev->nHeight, Consensus::UPGRADE_NU5)) { + assert(pcoinsTip->GetOrchardAnchorAt( + pindex->pprev->hashFinalOrchardRoot, oldFrontiers.orchard)); + } else { + assert(pcoinsTip->GetOrchardAnchorAt( + OrchardMerkleFrontier::empty_root(), oldFrontiers.orchard)); + } blockStack.emplace_back( pindex, diff --git a/src/validationinterface.h b/src/validationinterface.h index fbba2860a..ca5757c97 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -26,6 +26,7 @@ class uint256; struct MerkleFrontiers { SproutMerkleTree sprout; SaplingMerkleTree sapling; + OrchardMerkleFrontier orchard; }; // These functions dispatch to one or all registered wallets diff --git a/src/wallet/orchard.h b/src/wallet/orchard.h index d6aa43ce2..a846a95c3 100644 --- a/src/wallet/orchard.h +++ b/src/wallet/orchard.h @@ -13,6 +13,7 @@ #include "rust/orchard/keys.h" #include "rust/orchard/wallet.h" #include "zcash/address/orchard.hpp" +#include "zcash/IncrementalMerkleTree.hpp" class OrchardWallet; class OrchardWalletNoteCommitmentTreeWriter; @@ -218,6 +219,16 @@ public: return orchard_wallet_reset(inner.get()); } + /** + * Overwrite the first bridge of the Orchard note commitment tree to have the + * provided frontier as its latest state. This will fail with an assertion error + * if any checkpoints exist in the tree. + */ + void InitNoteCommitmentTree(const OrchardMerkleFrontier& frontier) { + assert(!GetLastCheckpointHeight().has_value()); + assert(orchard_wallet_init_from_frontier(inner.get(), frontier.inner.get())); + } + /** * Checkpoint the note commitment tree. This returns `false` and leaves the note * commitment tree unmodified if the block height specified is not the successor @@ -245,9 +256,9 @@ public: * previously identified as having been spent by transactions in the * latest block. */ - bool Rewind(int nBlockHeight, uint32_t& blocksRewoundRet) { + bool Rewind(int nBlockHeight, uint32_t& uResultHeight) { assert(nBlockHeight >= 0); - return orchard_wallet_rewind(inner.get(), (uint32_t) nBlockHeight, &blocksRewoundRet); + return orchard_wallet_rewind(inner.get(), (uint32_t) nBlockHeight, &uResultHeight); } static void PushOrchardActionIVK(void* rec, RawOrchardActionIVK actionIVK) { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9b1a87129..80187b222 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2609,6 +2609,9 @@ void CWallet::IncrementNoteWitnesses( } if (performOrchardWalletUpdates && consensus.NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_NU5)) { + if (!orchardWallet.GetLastCheckpointHeight().has_value()) { + orchardWallet.InitNoteCommitmentTree(frontiers.orchard); + } assert(orchardWallet.CheckpointNoteCommitmentTree(pindex->nHeight)); } @@ -2665,7 +2668,6 @@ void CWallet::IncrementNoteWitnesses( // If we're at or beyond NU5 activation, update the Orchard note commitment tree. if (performOrchardWalletUpdates && consensus.NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_NU5)) { - assert(orchardWallet.AppendNoteCommitments(pindex->nHeight, *pblock)); assert(pindex->hashFinalOrchardRoot == orchardWallet.GetLatestAnchor()); } @@ -2736,10 +2738,10 @@ void CWallet::DecrementNoteWitnesses(const Consensus::Params& consensus, const C if (consensus.NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_NU5)) { // pindex->nHeight is the height of the block being removed, so we rewind // to the previous block height - uint32_t blocksRewound{0}; + uint32_t uResultHeight{0}; assert(pindex->nHeight >= 1); - assert(orchardWallet.Rewind(pindex->nHeight - 1, blocksRewound)); - assert(blocksRewound == 1); + assert(orchardWallet.Rewind(pindex->nHeight - 1, uResultHeight)); + assert(uResultHeight == pindex->nHeight - 1); if (consensus.NetworkUpgradeActive(pindex->nHeight - 1, Consensus::UPGRADE_NU5)) { assert(pindex->pprev->hashFinalOrchardRoot == orchardWallet.GetLatestAnchor()); } @@ -4346,13 +4348,14 @@ int CWallet::ScanForWalletTransactions( // since we do not support Orchard key import. if (isInitScan) { int rewindHeight = std::max(nu5_height.value(), pindex->nHeight - 1); - uint32_t blocksRewound{0}; LogPrintf( "CWallet::ScanForWalletTransactions(): Rewinding Orchard wallet to height %d; current is %d", rewindHeight, orchardWallet.GetLastCheckpointHeight().value()); - if (orchardWallet.Rewind(rewindHeight, blocksRewound)) { + uint32_t uResultHeight{0}; + if (orchardWallet.Rewind(rewindHeight, uResultHeight)) { // rewind was successful or a no-op, so perform Orchard wallet updates + assert(uResultHeight == rewindHeight); performOrchardWalletUpdates = true; } else { // Orchard witnesses will not be able to be correctly updated, because we @@ -4396,6 +4399,9 @@ int CWallet::ScanForWalletTransactions( if (consensus.NetworkUpgradeActive(pindex->pprev->nHeight, Consensus::UPGRADE_SAPLING)) { assert(pcoinsTip->GetSaplingAnchorAt(pindex->pprev->hashFinalSaplingRoot, frontiers.sapling)); } + if (consensus.NetworkUpgradeActive(pindex->pprev->nHeight, Consensus::UPGRADE_NU5)) { + assert(pcoinsTip->GetOrchardAnchorAt(pindex->pprev->hashFinalOrchardRoot, frontiers.orchard)); + } } // Increment note witness caches ChainTipAdded(pindex, &block, frontiers, performOrchardWalletUpdates); diff --git a/src/zcash/IncrementalMerkleTree.hpp b/src/zcash/IncrementalMerkleTree.hpp index 5c6d51899..e8fa7e912 100644 --- a/src/zcash/IncrementalMerkleTree.hpp +++ b/src/zcash/IncrementalMerkleTree.hpp @@ -259,12 +259,16 @@ typedef libzcash::IncrementalMerkleTree SaplingWitness; typedef libzcash::IncrementalWitness SaplingTestingWitness; +class OrchardWallet; + class OrchardMerkleFrontier { private: /// An incremental Sinsemilla tree; this pointer may never be null. /// Memory is allocated by Rust. std::unique_ptr inner; + + friend class OrchardWallet; public: OrchardMerkleFrontier() : inner(orchard_merkle_frontier_empty(), orchard_merkle_frontier_free) {} From 344aef435dd3f49a6eaa9383465d4678f6a9defd Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 31 Mar 2022 16:53:51 -0600 Subject: [PATCH 4/7] Improve error output from OrchardWallet::get_spend_info --- src/rust/src/wallet.rs | 106 +++++++++++++++++++++++++---------------- 1 file changed, 64 insertions(+), 42 deletions(-) diff --git a/src/rust/src/wallet.rs b/src/rust/src/wallet.rs index 858ff4997..526d07327 100644 --- a/src/rust/src/wallet.rs +++ b/src/rust/src/wallet.rs @@ -42,14 +42,14 @@ pub struct LastObserved { } /// A pointer to a particular action in an Orchard transaction output. -#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] pub struct OutPoint { txid: TxId, action_idx: usize, } /// A pointer to a previous output being spent in an Orchard action. -#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] pub struct InPoint { txid: TxId, action_idx: usize, @@ -195,6 +195,14 @@ pub enum BundleLoadError { InvalidActionIndex(usize), } +#[derive(Debug, Clone)] +pub enum SpendRetrievalError { + DecryptedNoteNotFound(OutPoint), + NoIvkForRecipient(Address), + FvkNotFound(IncomingViewingKey), + NoteNotPositioned(OutPoint), +} + /// A struct used to return metadata about how a bundle was determined /// to be involved with the wallet. #[derive(Debug, Clone)] @@ -652,42 +660,55 @@ impl Wallet { /// /// Returns `None` if the `OutPoint` is not known to the wallet, or the Orchard bundle /// containing the note has not been passed to `Wallet::append_bundle_commitments`. - pub fn get_spend_info(&self, outpoint: OutPoint) -> Option { + pub fn get_spend_info( + &self, + outpoint: OutPoint, + ) -> Result { // TODO: Take `confirmations` parameter and obtain the Merkle path to the root at // that checkpoint, not the latest root. - self.wallet_received_notes + let dnote = self + .wallet_received_notes .get(&outpoint.txid) .and_then(|tx_notes| tx_notes.decrypted_notes.get(&outpoint.action_idx)) - .and_then(|dnote| { + .ok_or(SpendRetrievalError::DecryptedNoteNotFound(outpoint))?; + + let fvk = self + .key_store + .ivk_for_address(&dnote.note.recipient()) + .ok_or_else(|| SpendRetrievalError::NoIvkForRecipient(dnote.note.recipient())) + .and_then(|ivk| { self.key_store - .ivk_for_address(&dnote.note.recipient()) - .and_then(|ivk| self.key_store.viewing_keys.get(ivk)) - .zip( - self.wallet_note_positions - .get(&outpoint.txid) - .and_then(|tx_notes| tx_notes.note_positions.get(&outpoint.action_idx)), - ) - .map(|(fvk, position)| { - assert_eq!( - self.witness_tree - .get_witnessed_leaf(*position) - .expect("tree has witnessed the leaf for this note."), - &MerkleHashOrchard::from_cmx(&dnote.note.commitment().into()), - ); - let path = self - .witness_tree - .authentication_path(*position) - .expect("wallet always has paths to positioned notes"); - OrchardSpendInfo::from_parts( - fvk.clone(), - dnote.note, - MerklePath::from_parts( - u64::from(*position).try_into().unwrap(), - path.try_into().unwrap(), - ), - ) - }) - }) + .viewing_keys + .get(ivk) + .ok_or_else(|| SpendRetrievalError::FvkNotFound(ivk.clone())) + })?; + + let position = self + .wallet_note_positions + .get(&outpoint.txid) + .and_then(|tx_notes| tx_notes.note_positions.get(&outpoint.action_idx)) + .ok_or(SpendRetrievalError::NoteNotPositioned(outpoint))?; + + assert_eq!( + self.witness_tree + .get_witnessed_leaf(*position) + .expect("tree has witnessed the leaf for this note."), + &MerkleHashOrchard::from_cmx(&dnote.note.commitment().into()), + ); + + let path = self + .witness_tree + .authentication_path(*position) + .expect("wallet always has paths to positioned notes"); + + Ok(OrchardSpendInfo::from_parts( + fvk.clone(), + dnote.note, + MerklePath::from_parts( + u64::from(*position).try_into().unwrap(), + path.try_into().unwrap(), + ), + )) } } @@ -1158,15 +1179,16 @@ pub extern "C" fn orchard_wallet_get_spend_info( let outpoint = OutPoint { txid, action_idx }; - if let Some(ret) = wallet.get_spend_info(outpoint) { - Box::into_raw(Box::new(ret)) - } else { - tracing::error!( - "Requested note in action {} of transaction {} wasn't in the wallet", - outpoint.action_idx, - outpoint.txid - ); - ptr::null_mut() + match wallet.get_spend_info(outpoint) { + Ok(ret) => Box::into_raw(Box::new(ret)), + Err(e) => { + tracing::error!( + "Error obtaining spend info for outpoint {:?}: {:?}", + outpoint, + e + ); + ptr::null_mut() + } } } From 5b7370c55ecee08e13ae0aea30b300f8a92099c4 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 4 Apr 2022 12:50:51 -0600 Subject: [PATCH 5/7] Update test to verify rewind behavior. --- qa/rpc-tests/wallet_orchard_init.py | 50 +++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/qa/rpc-tests/wallet_orchard_init.py b/qa/rpc-tests/wallet_orchard_init.py index d8ad830a9..b5db9f4e1 100755 --- a/qa/rpc-tests/wallet_orchard_init.py +++ b/qa/rpc-tests/wallet_orchard_init.py @@ -6,6 +6,8 @@ import os import os.path +from decimal import Decimal + from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( NU5_BRANCH_ID, @@ -78,6 +80,15 @@ class OrchardWalletInitTest(BitcoinTestFramework): {'pools': {'orchard': {'valueZat': 1_0000_0000}}, 'minimum_confirmations': 1}, self.nodes[0].z_getbalanceforaccount(acct0)) + # Split the network. We'll advance the state of nodes 0/1 so that after + # we re-join the network, we need to roll back more than one block after + # the wallet is restored, into a chain state that the wallet never observed. + self.split_network() + + self.sync_all() + self.nodes[0].generate(2) + self.sync_all() + # Shut down the network and delete node 0's wallet stop_nodes(self.nodes) wait_bitcoinds() @@ -85,8 +96,8 @@ class OrchardWalletInitTest(BitcoinTestFramework): tmpdir = self.options.tmpdir os.remove(os.path.join(tmpdir, "node0", "regtest", "wallet.dat")) - # Restart the network split; the split here is note necessary to reproduce the - # error but it is sufficient, and we will later use the split to check the + # Restart the network, still split; the split here is note necessary to reproduce + # the error but it is sufficient, and we will later use the split to check the # corresponding rewind. self.setup_network(True) @@ -103,7 +114,7 @@ class OrchardWalletInitTest(BitcoinTestFramework): # the state of the global note commitment tree. recipients = [{"address": ua0new, "amount": 1}] myopid = self.nodes[1].z_sendmany(ua1, recipients, 1, 0) - wait_and_assert_operationid_status(self.nodes[1], myopid) + rollback_tx = wait_and_assert_operationid_status(self.nodes[1], myopid) self.sync_all() self.nodes[0].generate(1) @@ -113,6 +124,39 @@ class OrchardWalletInitTest(BitcoinTestFramework): {'pools': {'orchard': {'valueZat': 1_0000_0000}}, 'minimum_confirmations': 1}, self.nodes[0].z_getbalanceforaccount(acct0new)) + # Node 2 has no progress since the network was first split, so this should roll + # everything back to the original fork point. + self.nodes[2].generate(10) + + # Re-join the network + self.join_network() + + assert_equal(set([rollback_tx]), set(self.nodes[1].getrawmempool())) + + # Resend un-mined transactions and sync the network + self.nodes[1].resendwallettransactions() + self.sync_all() + self.nodes[0].generate(1) + self.sync_all() + + assert_equal( + {'pools': {'orchard': {'valueZat': 1_0000_0000}}, 'minimum_confirmations': 1}, + self.nodes[0].z_getbalanceforaccount(acct0new)) + + # Spend from the note that was just received + recipients = [{"address": ua1, "amount": Decimal('0.3')}] + myopid = self.nodes[0].z_sendmany(ua0new, recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[0], myopid) + + self.sync_all() + self.nodes[0].generate(1) + self.sync_all() + + assert_equal( + {'pools': {'orchard': {'valueZat': 8_3000_0000}}, 'minimum_confirmations': 1}, + self.nodes[1].z_getbalanceforaccount(acct1)) + + if __name__ == '__main__': OrchardWalletInitTest().main() From afb503503d8996de060ce409a9d2dd17b05c7005 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 4 Apr 2022 13:05:46 -0600 Subject: [PATCH 6/7] Omit check of Orchard commitment root after rewind past first checkpoint. If we no longer have any checkpoints in the Orchard wallet, we must skip the check against the prior note commitment tree root because we know that the Orchard wallet's state may be for a chain that is being reorg'ed away. This is safe because we know that we will have removed all information from the wallet that we need to perform spends from that state, and we also know that when we start rolling forward along the new chain that we will overwrite the initial state of the Orchard note commitment tree. --- src/wallet/wallet.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 80187b222..8f3751e81 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2742,7 +2742,11 @@ void CWallet::DecrementNoteWitnesses(const Consensus::Params& consensus, const C assert(pindex->nHeight >= 1); assert(orchardWallet.Rewind(pindex->nHeight - 1, uResultHeight)); assert(uResultHeight == pindex->nHeight - 1); - if (consensus.NetworkUpgradeActive(pindex->nHeight - 1, Consensus::UPGRADE_NU5)) { + // If we have no checkpoints after the rewind, then the latest anchor of the + // wallet's Orchard note commitment tree will be in an indeterminate state and it + // will be overwritten in the next `IncrementNoteWitnesses` call, so we can skip + // the check against `hashFinalOrchardRoot`. + if (orchardWallet.GetLastCheckpointHeight().has_value()) { assert(pindex->pprev->hashFinalOrchardRoot == orchardWallet.GetLatestAnchor()); } } From 8e4dd1e964d2a4a61d850909b9a00c160ff793b3 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 4 Apr 2022 15:31:19 -0600 Subject: [PATCH 7/7] Only check nWitnessCacheSize on rewind if we've ever witnessed a Sprout or Sapling note. This allows "rollback" for empty Sapling wallets injected into nonempty chain state that is then rolled back. --- src/wallet/wallet.cpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8f3751e81..fd5263e1f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2726,13 +2726,21 @@ void DecrementNoteWitnesses(NoteDataMap& noteDataMap, int indexHeight, int64_t n void CWallet::DecrementNoteWitnesses(const Consensus::Params& consensus, const CBlockIndex* pindex) { LOCK(cs_wallet); + bool hasSprout = false; + bool hasSapling = false; for (std::pair& wtxItem : mapWallet) { + hasSprout |= !wtxItem.second.mapSproutNoteData.empty(); ::DecrementNoteWitnesses(wtxItem.second.mapSproutNoteData, pindex->nHeight, nWitnessCacheSize); + hasSapling |= !wtxItem.second.mapSaplingNoteData.empty(); ::DecrementNoteWitnesses(wtxItem.second.mapSaplingNoteData, pindex->nHeight, nWitnessCacheSize); } - nWitnessCacheSize -= 1; - // TODO: If nWitnessCache is zero, we need to regenerate the caches (#1302) - assert(nWitnessCacheSize > 0); + if (nWitnessCacheSize > 0) { + nWitnessCacheSize -= 1; + } + // TODO: If nWitnessCache is zero, we need to regenerate the caches (#1302); + // however, if we have never observed Sprout or Sapling notes, this is okay + // because then the witness cache size can remain at 0. + assert(!(hasSprout || hasSapling) || nWitnessCacheSize > 0); // ORCHARD: rewind to the last checkpoint. if (consensus.NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_NU5)) {