From 5a6ef3467eb1a4248634446e0f995de5f4088a26 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 24 Feb 2022 13:35:45 -0700 Subject: [PATCH 01/16] Update MSRV for lints. --- .github/workflows/lints.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/lints.yml b/.github/workflows/lints.yml index d2347eac3..7a74929bf 100644 --- a/.github/workflows/lints.yml +++ b/.github/workflows/lints.yml @@ -70,19 +70,19 @@ jobs: if: always() rust-clippy: - name: Clippy (1.54.0) + name: Clippy (1.56.1) runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 - uses: actions-rs/toolchain@v1 with: - toolchain: 1.54.0 + toolchain: 1.56.1 components: clippy override: true - name: Run clippy uses: actions-rs/clippy-check@v1 with: - name: Clippy (1.54.0) + name: Clippy (1.56.1) token: ${{ secrets.GITHUB_TOKEN }} args: --all-features --all-targets -- -D warnings @@ -93,7 +93,7 @@ jobs: - uses: actions/checkout@v2 - uses: actions-rs/toolchain@v1 with: - toolchain: 1.54.0 + toolchain: 1.56.1 override: true - run: rustup component add rustfmt - uses: actions-rs/cargo@v1 From 237384cf2bd8fab2224968588a7aa1ac741424ee Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 24 Feb 2022 07:37:08 -0700 Subject: [PATCH 02/16] Update incrementalmerkletree version --- .cargo/config.offline | 9 +++++++-- Cargo.lock | 21 ++++++++++----------- Cargo.toml | 13 +++++++------ 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/.cargo/config.offline b/.cargo/config.offline index bf526dc97..d98d2c9b3 100644 --- a/.cargo/config.offline +++ b/.cargo/config.offline @@ -3,12 +3,17 @@ replace-with = "vendored-sources" [source."https://github.com/zcash/librustzcash.git"] git = "https://github.com/zcash/librustzcash.git" -rev = "3d935a94e75786a67c3ea4992d7c372af203086f" +rev = "cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9" replace-with = "vendored-sources" [source."https://github.com/zcash/orchard.git"] git = "https://github.com/zcash/orchard.git" -rev = "3b8d07f7b64b2329622089ac9698e4cce97e2f14" +rev = "8449fd133c002a349869c09c529a4d214b20c0c0" +replace-with = "vendored-sources" + +[source."https://github.com/zcash/incrementalmerkletree.git"] +git = "https://github.com/zcash/incrementalmerkletree.git" +rev = "5707d6ac096c2b7a345166e08c378e0a5d64e6d8" replace-with = "vendored-sources" [source."https://github.com/nuttycom/hdwallet.git"] diff --git a/Cargo.lock b/Cargo.lock index 56892ec02..7ab6d7224 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -501,7 +501,7 @@ checksum = "c34f04666d835ff5d62e058c3995147c06f42fe86ff053337632bca83e42702d" [[package]] name = "equihash" version = "0.1.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=3d935a94e75786a67c3ea4992d7c372af203086f#3d935a94e75786a67c3ea4992d7c372af203086f" +source = "git+https://github.com/zcash/librustzcash.git?rev=cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9#cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9" dependencies = [ "blake2b_simd 1.0.0", "byteorder", @@ -510,7 +510,7 @@ dependencies = [ [[package]] name = "f4jumble" version = "0.0.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=3d935a94e75786a67c3ea4992d7c372af203086f#3d935a94e75786a67c3ea4992d7c372af203086f" +source = "git+https://github.com/zcash/librustzcash.git?rev=cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9#cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9" dependencies = [ "blake2b_simd 1.0.0", ] @@ -749,8 +749,7 @@ dependencies = [ [[package]] name = "incrementalmerkletree" version = "0.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "186fd3ab92aeac865d4b80b410de9a7b341d31ba8281373caed0b6d17b2b5e96" +source = "git+https://github.com/zcash/incrementalmerkletree.git?rev=5707d6ac096c2b7a345166e08c378e0a5d64e6d8#5707d6ac096c2b7a345166e08c378e0a5d64e6d8" dependencies = [ "serde", ] @@ -1090,7 +1089,7 @@ checksum = "624a8340c38c1b80fd549087862da4ba43e08858af025b236e509b6649fc13d5" [[package]] name = "orchard" version = "0.1.0-beta.1" -source = "git+https://github.com/zcash/orchard.git?rev=3b8d07f7b64b2329622089ac9698e4cce97e2f14#3b8d07f7b64b2329622089ac9698e4cce97e2f14" +source = "git+https://github.com/zcash/orchard.git?rev=8449fd133c002a349869c09c529a4d214b20c0c0#8449fd133c002a349869c09c529a4d214b20c0c0" dependencies = [ "aes", "arrayvec 0.7.2", @@ -1916,7 +1915,7 @@ dependencies = [ [[package]] name = "zcash_address" version = "0.0.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=3d935a94e75786a67c3ea4992d7c372af203086f#3d935a94e75786a67c3ea4992d7c372af203086f" +source = "git+https://github.com/zcash/librustzcash.git?rev=cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9#cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9" dependencies = [ "bech32", "bs58", @@ -1927,7 +1926,7 @@ dependencies = [ [[package]] name = "zcash_encoding" version = "0.0.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=3d935a94e75786a67c3ea4992d7c372af203086f#3d935a94e75786a67c3ea4992d7c372af203086f" +source = "git+https://github.com/zcash/librustzcash.git?rev=cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9#cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9" dependencies = [ "byteorder", "nonempty", @@ -1936,7 +1935,7 @@ dependencies = [ [[package]] name = "zcash_history" version = "0.2.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=3d935a94e75786a67c3ea4992d7c372af203086f#3d935a94e75786a67c3ea4992d7c372af203086f" +source = "git+https://github.com/zcash/librustzcash.git?rev=cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9#cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9" dependencies = [ "bigint", "blake2b_simd 1.0.0", @@ -1946,7 +1945,7 @@ dependencies = [ [[package]] name = "zcash_note_encryption" version = "0.1.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=3d935a94e75786a67c3ea4992d7c372af203086f#3d935a94e75786a67c3ea4992d7c372af203086f" +source = "git+https://github.com/zcash/librustzcash.git?rev=cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9#cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9" dependencies = [ "chacha20", "chacha20poly1305", @@ -1957,7 +1956,7 @@ dependencies = [ [[package]] name = "zcash_primitives" version = "0.5.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=3d935a94e75786a67c3ea4992d7c372af203086f#3d935a94e75786a67c3ea4992d7c372af203086f" +source = "git+https://github.com/zcash/librustzcash.git?rev=cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9#cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9" dependencies = [ "aes", "bip0039", @@ -1993,7 +1992,7 @@ dependencies = [ [[package]] name = "zcash_proofs" version = "0.5.0" -source = "git+https://github.com/zcash/librustzcash.git?rev=3d935a94e75786a67c3ea4992d7c372af203086f#3d935a94e75786a67c3ea4992d7c372af203086f" +source = "git+https://github.com/zcash/librustzcash.git?rev=cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9#cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9" dependencies = [ "bellman", "blake2b_simd 1.0.0", diff --git a/Cargo.toml b/Cargo.toml index ecd6ee944..ee858c36b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -72,9 +72,10 @@ codegen-units = 1 [patch.crates-io] hdwallet = { git = "https://github.com/nuttycom/hdwallet", rev = "576683b9f2865f1118c309017ff36e01f84420c9" } -orchard = { git = "https://github.com/zcash/orchard.git", rev = "3b8d07f7b64b2329622089ac9698e4cce97e2f14" } -zcash_address = { git = "https://github.com/zcash/librustzcash.git", rev = "3d935a94e75786a67c3ea4992d7c372af203086f" } -zcash_history = { git = "https://github.com/zcash/librustzcash.git", rev = "3d935a94e75786a67c3ea4992d7c372af203086f" } -zcash_note_encryption = { git = "https://github.com/zcash/librustzcash.git", rev = "3d935a94e75786a67c3ea4992d7c372af203086f" } -zcash_primitives = { git = "https://github.com/zcash/librustzcash.git", rev = "3d935a94e75786a67c3ea4992d7c372af203086f" } -zcash_proofs = { git = "https://github.com/zcash/librustzcash.git", rev = "3d935a94e75786a67c3ea4992d7c372af203086f" } +incrementalmerkletree = { git = "https://github.com/zcash/incrementalmerkletree.git", rev = "5707d6ac096c2b7a345166e08c378e0a5d64e6d8" } +orchard = { git = "https://github.com/zcash/orchard.git", rev = "8449fd133c002a349869c09c529a4d214b20c0c0" } +zcash_address = { git = "https://github.com/zcash/librustzcash.git", rev = "cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9" } +zcash_history = { git = "https://github.com/zcash/librustzcash.git", rev = "cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9" } +zcash_note_encryption = { git = "https://github.com/zcash/librustzcash.git", rev = "cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9" } +zcash_primitives = { git = "https://github.com/zcash/librustzcash.git", rev = "cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9" } +zcash_proofs = { git = "https://github.com/zcash/librustzcash.git", rev = "cfb49cfd52e1f5c43a27de1de1156d9504ca9eb9" } From 8bc4c2aad836fdd5d7f40447fa1b6b5961ad37b0 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 1 Mar 2022 13:53:50 -0700 Subject: [PATCH 03/16] Split LoadWalletTx from AddToWallet The `CWallet::AddToWallet` method had completely divergent behavior depending upon the value of the fFromLoadWallet flag, and `pwalletdb` was unused when this flag was set to `true`, so this is better represented as two distinct methods on CWallet. --- src/wallet/gtest/test_wallet.cpp | 78 ++++++++++++++-------------- src/wallet/test/rpc_wallet_tests.cpp | 8 +-- src/wallet/wallet.cpp | 36 ++++++------- src/wallet/wallet.h | 3 +- src/wallet/walletdb.cpp | 2 +- src/zcbenchmarks.cpp | 8 +-- 6 files changed, 68 insertions(+), 67 deletions(-) diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index a745c5cef..5be1820e8 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -102,7 +102,7 @@ std::pair CreateValidBlock(TestWallet& wallet, noteData[jsoutpt] = nd; wtx.SetSproutNoteData(noteData); auto saplingNotes = SetSaplingNoteData(wtx); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); block.vtx.push_back(wtx); wallet.IncrementNoteWitnesses(&index, &block, sproutTree, saplingTree); @@ -205,7 +205,7 @@ TEST(WalletTests, FindUnspentSproutNotes) { noteData[jsoutpt] = nd; wtx.SetSproutNoteData(noteData); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); EXPECT_FALSE(wallet.IsSproutSpent(nullifier)); // We currently have an unspent and unconfirmed note in the wallet (depth of -1) @@ -235,7 +235,7 @@ TEST(WalletTests, FindUnspentSproutNotes) { EXPECT_EQ(0, chainActive.Height()); wtx.SetMerkleBranch(block); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); EXPECT_FALSE(wallet.IsSproutSpent(nullifier)); @@ -259,7 +259,7 @@ TEST(WalletTests, FindUnspentSproutNotes) { // Let's spend the note. auto wtx2 = GetValidSproutSpend(sk, note, 5); - wallet.AddToWallet(wtx2, true, NULL); + wallet.LoadWalletTx(wtx2); EXPECT_FALSE(wallet.IsSproutSpent(nullifier)); // Fake-mine a spend transaction @@ -277,7 +277,7 @@ TEST(WalletTests, FindUnspentSproutNotes) { EXPECT_EQ(1, chainActive.Height()); wtx2.SetMerkleBranch(block2); - wallet.AddToWallet(wtx2, true, NULL); + wallet.LoadWalletTx(wtx2); EXPECT_TRUE(wallet.IsSproutSpent(nullifier)); // The note has been spent. By default, GetFilteredNotes() ignores spent notes. @@ -319,7 +319,7 @@ TEST(WalletTests, FindUnspentSproutNotes) { noteData[jsoutpt] = nd; wtx.SetSproutNoteData(noteData); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); EXPECT_FALSE(wallet.IsSproutSpent(nullifier)); wtx3 = wtx; @@ -340,7 +340,7 @@ TEST(WalletTests, FindUnspentSproutNotes) { EXPECT_EQ(2, chainActive.Height()); wtx3.SetMerkleBranch(block3); - wallet.AddToWallet(wtx3, true, NULL); + wallet.LoadWalletTx(wtx3); // We now have an unspent note which has one confirmation, in addition to our spent note. wallet.GetFilteredNotes(sproutEntries, saplingEntries, orchardEntries, std::nullopt, 1); @@ -654,15 +654,15 @@ TEST(WalletTests, GetConflictedSproutNotes) { // No conflicts for no spends EXPECT_EQ(0, wallet.GetConflicts(hash2).size()); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); EXPECT_EQ(0, wallet.GetConflicts(hash2).size()); // No conflicts for one spend - wallet.AddToWallet(wtx2, true, NULL); + wallet.LoadWalletTx(wtx2); EXPECT_EQ(0, wallet.GetConflicts(hash2).size()); // Conflicts for two spends - wallet.AddToWallet(wtx3, true, NULL); + wallet.LoadWalletTx(wtx3); auto c3 = wallet.GetConflicts(hash2); EXPECT_EQ(2, c3.size()); EXPECT_EQ(std::set({hash2, hash3}), c3); @@ -723,7 +723,7 @@ TEST(WalletTests, GetConflictedSaplingNotes) { ASSERT_TRUE(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); wtx.SetMerkleBranch(block); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); // Simulate receiving new block and ChainTip signal wallet.IncrementNoteWitnesses(&fakeIndex, &block, sproutTree, saplingTree); @@ -777,11 +777,11 @@ TEST(WalletTests, GetConflictedSaplingNotes) { EXPECT_EQ(0, wallet.GetConflicts(hash3).size()); // No conflicts for one spend - wallet.AddToWallet(wtx2, true, NULL); + wallet.LoadWalletTx(wtx2); EXPECT_EQ(0, wallet.GetConflicts(hash2).size()); // Conflicts for two spends - wallet.AddToWallet(wtx3, true, NULL); + wallet.LoadWalletTx(wtx3); auto c3 = wallet.GetConflicts(hash2); EXPECT_EQ(2, c3.size()); EXPECT_EQ(std::set({hash2, hash3}), c3); @@ -808,11 +808,11 @@ TEST(WalletTests, SproutNullifierIsSpent) { EXPECT_FALSE(wallet.IsSproutSpent(nullifier)); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); EXPECT_FALSE(wallet.IsSproutSpent(nullifier)); auto wtx2 = GetValidSproutSpend(sk, note, 5); - wallet.AddToWallet(wtx2, true, NULL); + wallet.LoadWalletTx(wtx2); EXPECT_FALSE(wallet.IsSproutSpent(nullifier)); // Fake-mine the transaction @@ -828,7 +828,7 @@ TEST(WalletTests, SproutNullifierIsSpent) { EXPECT_EQ(0, chainActive.Height()); wtx2.SetMerkleBranch(block); - wallet.AddToWallet(wtx2, true, NULL); + wallet.LoadWalletTx(wtx2); EXPECT_TRUE(wallet.IsSproutSpent(nullifier)); // Tear down @@ -880,7 +880,7 @@ TEST(WalletTests, SaplingNullifierIsSpent) { EXPECT_EQ(0, chainActive.Height()); wtx.SetMerkleBranch(block); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); // Verify note has been spent EXPECT_TRUE(wallet.IsSaplingSpent(nullifier)); @@ -914,7 +914,7 @@ TEST(WalletTests, NavigateFromSproutNullifierToNote) { EXPECT_EQ(0, wallet.mapSproutNullifiersToNotes.count(nullifier)); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); EXPECT_EQ(1, wallet.mapSproutNullifiersToNotes.count(nullifier)); EXPECT_EQ(wtx.GetHash(), wallet.mapSproutNullifiersToNotes[nullifier].hash); EXPECT_EQ(0, wallet.mapSproutNullifiersToNotes[nullifier].js); @@ -970,7 +970,7 @@ TEST(WalletTests, NavigateFromSaplingNullifierToNote) { auto saplingNoteData = wallet.FindMySaplingNotes(wtx, chainActive.Height()).first; ASSERT_TRUE(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); // Verify dummy note is now spent, as AddToWallet invokes AddToSpends() EXPECT_TRUE(wallet.IsSaplingSpent(nullifier)); @@ -1038,7 +1038,7 @@ TEST(WalletTests, SpentSproutNoteIsFromMe) { EXPECT_FALSE(wallet.IsFromMe(wtx)); EXPECT_FALSE(wallet.IsFromMe(wtx2)); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); EXPECT_FALSE(wallet.IsFromMe(wtx)); EXPECT_TRUE(wallet.IsFromMe(wtx2)); } @@ -1097,7 +1097,7 @@ TEST(WalletTests, SpentSaplingNoteIsFromMe) { ASSERT_TRUE(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); wtx.SetMerkleBranch(block); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); // Simulate receiving new block and ChainTip signal. // This triggers calculation of nullifiers for notes belonging to this wallet @@ -1174,9 +1174,9 @@ TEST(WalletTests, SpentSaplingNoteIsFromMe) { ASSERT_TRUE(saplingNoteData2.size() > 0); wtx2.SetSaplingNoteData(saplingNoteData2); wtx2.SetMerkleBranch(block2); - wallet.AddToWallet(wtx2, true, NULL); + wallet.LoadWalletTx(wtx2); - // Verify note B is spent. AddToWallet invokes AddToSpends which updates mapTxSaplingNullifiers + // Verify note B is spent. LoadWalletTx invokes AddToSpends which updates mapTxSaplingNullifiers EXPECT_TRUE(wallet.IsSaplingSpent(nullifier2)); // Verify note B belongs to wallet. @@ -1226,7 +1226,7 @@ TEST(WalletTests, CachedWitnessesEmptyChain) { EXPECT_FALSE((bool) sproutWitnesses[1]); EXPECT_FALSE((bool) saplingWitnesses[0]); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); ::GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); @@ -1293,7 +1293,7 @@ TEST(WalletTests, CachedWitnessesChainTip) { sproutNoteData[jsoutpt] = nd; wtx.SetSproutNoteData(sproutNoteData); std::vector saplingNotes = SetSaplingNoteData(wtx); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); std::vector sproutNotes {jsoutpt}; std::vector> sproutWitnesses; @@ -1405,7 +1405,7 @@ TEST(WalletTests, CachedWitnessesDecrementFirst) { noteData[jsoutpt] = nd; wtx.SetSproutNoteData(noteData); std::vector saplingNotes = SetSaplingNoteData(wtx); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); std::vector sproutNotes {jsoutpt}; std::vector> sproutWitnesses; @@ -1562,7 +1562,7 @@ TEST(WalletTests, ClearNoteWitnessCache) { wtx.mapSaplingNoteData[saplingNotes[0]].witnessHeight = 1; wallet.nWitnessCacheSize = 2; - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); // For Sprout, we have two outputs in the one JSDescription, only one of // which is in the wallet. @@ -1618,7 +1618,7 @@ TEST(WalletTests, WriteWitnessCache) { SproutNoteData nd {sk.address(), nullifier}; noteData[jsoutpt] = nd; wtx.SetSproutNoteData(noteData); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); // TxnBegin fails EXPECT_CALL(walletdb, TxnBegin()) @@ -1708,13 +1708,13 @@ TEST(WalletTests, SetBestChainIgnoresTxsWithoutShieldedData) { t.vout[0].nValue = 90*CENT; t.vout[0].scriptPubKey = scriptPubKey; CWalletTx wtxTransparent {nullptr, t}; - wallet.AddToWallet(wtxTransparent, true, nullptr); + wallet.LoadWalletTx(wtxTransparent); // Generate a Sprout transaction that is ours auto wtxSprout = GetValidSproutReceive(sk, 10, true); auto noteMap = wallet.FindMySproutNotes(wtxSprout); wtxSprout.SetSproutNoteData(noteMap); - wallet.AddToWallet(wtxSprout, true, nullptr); + wallet.LoadWalletTx(wtxSprout); // Generate a Sprout transaction that only involves our transparent address auto sk2 = libzcash::SproutSpendingKey::random(); @@ -1725,7 +1725,7 @@ TEST(WalletTests, SetBestChainIgnoresTxsWithoutShieldedData) { mtx.vout[0].scriptPubKey = scriptPubKey; mtx.vout[0].nValue = CENT; CWalletTx wtxSproutTransparent {nullptr, mtx}; - wallet.AddToWallet(wtxSproutTransparent, true, nullptr); + wallet.LoadWalletTx(wtxSproutTransparent); // Generate a fake Sapling transaction CMutableTransaction mtxSapling; @@ -1736,7 +1736,7 @@ TEST(WalletTests, SetBestChainIgnoresTxsWithoutShieldedData) { zcash_test_harness_random_jubjub_point(mtxSapling.vShieldedOutput[0].cv.begin()); CWalletTx wtxSapling {nullptr, mtxSapling}; SetSaplingNoteData(wtxSapling); - wallet.AddToWallet(wtxSapling, true, nullptr); + wallet.LoadWalletTx(wtxSapling); // Generate a fake Sapling transaction that would only involve our transparent addresses CMutableTransaction mtxSaplingTransparent; @@ -1746,7 +1746,7 @@ TEST(WalletTests, SetBestChainIgnoresTxsWithoutShieldedData) { mtxSaplingTransparent.vShieldedOutput.resize(1); zcash_test_harness_random_jubjub_point(mtxSaplingTransparent.vShieldedOutput[0].cv.begin()); CWalletTx wtxSaplingTransparent {nullptr, mtxSaplingTransparent}; - wallet.AddToWallet(wtxSaplingTransparent, true, nullptr); + wallet.LoadWalletTx(wtxSaplingTransparent); EXPECT_CALL(walletdb, TxnBegin()) .WillOnce(Return(true)); @@ -1793,7 +1793,7 @@ TEST(WalletTests, UpdateSproutNullifierNoteMap) { noteData[jsoutpt] = nd; wtx.SetSproutNoteData(noteData); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); EXPECT_EQ(0, wallet.mapSproutNullifiersToNotes.count(nullifier)); EXPECT_FALSE(wallet.UpdateNullifierNoteMap()); @@ -1907,7 +1907,7 @@ TEST(WalletTests, UpdatedSaplingNoteData) { ASSERT_TRUE(saplingNoteData.size() == 1); // wallet only has key for change output wtx.SetSaplingNoteData(saplingNoteData); wtx.SetMerkleBranch(block); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); // Simulate receiving new block and ChainTip signal wallet.IncrementNoteWitnesses(&fakeIndex, &block, sproutTree, testNote.tree); @@ -1986,7 +1986,7 @@ TEST(WalletTests, MarkAffectedSproutTransactionsDirty) { noteData[jsoutpt] = nd; wtx.SetSproutNoteData(noteData); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); wallet.MarkAffectedTransactionsDirty(wtx); // After getting a cached value, the first tx should be clean @@ -1994,7 +1994,7 @@ TEST(WalletTests, MarkAffectedSproutTransactionsDirty) { EXPECT_TRUE(wallet.mapWallet[hash].fDebitCached); // After adding the note spend, the first tx should be dirty - wallet.AddToWallet(wtx2, true, NULL); + wallet.LoadWalletTx(wtx2); wallet.MarkAffectedTransactionsDirty(wtx2); EXPECT_FALSE(wallet.mapWallet[hash].fDebitCached); } @@ -2054,7 +2054,7 @@ TEST(WalletTests, MarkAffectedSaplingTransactionsDirty) { ASSERT_TRUE(saplingNoteData.size() > 0); wtx.SetSaplingNoteData(saplingNoteData); wtx.SetMerkleBranch(block); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); // Simulate receiving new block and ChainTip signal wallet.IncrementNoteWitnesses(&fakeIndex, &block, sproutTree, saplingTree); @@ -2097,7 +2097,7 @@ TEST(WalletTests, MarkAffectedSaplingTransactionsDirty) { EXPECT_TRUE(wallet.mapWallet[hash].fDebitCached); // After adding the note spend, the first tx should be dirty - wallet.AddToWallet(wtx2, true, NULL); + wallet.LoadWalletTx(wtx2); wallet.MarkAffectedTransactionsDirty(wtx2); EXPECT_FALSE(wallet.mapWallet[hash].fDebitCached); diff --git a/src/wallet/test/rpc_wallet_tests.cpp b/src/wallet/test/rpc_wallet_tests.cpp index 5531ccd78..b5e181ccf 100644 --- a/src/wallet/test/rpc_wallet_tests.cpp +++ b/src/wallet/test/rpc_wallet_tests.cpp @@ -1336,7 +1336,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_taddr_to_sapling) CScript scriptPubKey = CScript() << OP_DUP << OP_HASH160 << ToByteVector(taddr) << OP_EQUALVERIFY << OP_CHECKSIG; mtx.vout.push_back(CTxOut(5 * COIN, scriptPubKey)); CWalletTx wtx(pwalletMain, mtx); - pwalletMain->AddToWallet(wtx, true, NULL); + pwalletMain->LoadWalletTx(wtx); // Fake-mine the transaction BOOST_CHECK_EQUAL(0, chainActive.Height()); @@ -1352,7 +1352,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_taddr_to_sapling) BOOST_CHECK(chainActive.Contains(&fakeIndex)); BOOST_CHECK_EQUAL(1, chainActive.Height()); wtx.SetMerkleBranch(block); - pwalletMain->AddToWallet(wtx, true, NULL); + pwalletMain->LoadWalletTx(wtx); // Context that z_sendmany requires auto builder = TransactionBuilder(consensusParams, nextBlockHeight, std::nullopt, pwalletMain); @@ -1990,7 +1990,7 @@ void TestWTxStatus(const Consensus::Params consensusParams, const int delta) { CScript scriptPubKey = CScript() << OP_DUP << OP_HASH160 << ToByteVector(taddr) << OP_EQUALVERIFY << OP_CHECKSIG; mtx.vout.push_back(CTxOut(5 * COIN, scriptPubKey)); CWalletTx wtx(pwalletMain, mtx); - pwalletMain->AddToWallet(wtx, true, NULL); + pwalletMain->LoadWalletTx(wtx); return wtx; }; @@ -2011,7 +2011,7 @@ void TestWTxStatus(const Consensus::Params consensusParams, const int delta) { if (has_trx) { wtx.SetMerkleBranch(block); - pwalletMain->AddToWallet(wtx, true, NULL); + pwalletMain->LoadWalletTx(wtx); } hashes.push_back(blockHash); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 92c5fcc04..9fb627c5c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2674,21 +2674,21 @@ void CWallet::UpdateSaplingNullifierNoteMapForBlock(const CBlock *pblock) { } } -bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletDB* pwalletdb) -{ +void CWallet::LoadWalletTx(const CWalletTx& wtxIn) { uint256 hash = wtxIn.GetHash(); + mapWallet[hash] = wtxIn; + CWalletTx& wtx = mapWallet[hash]; + wtx.BindWallet(this); + wtxOrdered.insert(make_pair(wtx.nOrderPos, &wtx)); + UpdateNullifierNoteMapWithTx(mapWallet[hash]); + AddToSpends(hash); +} + +bool CWallet::AddToWallet(const CWalletTx& wtxIn, CWalletDB* pwalletdb) +{ + { // additional scope left in place for backport whitespace compatibility + uint256 hash = wtxIn.GetHash(); - if (fFromLoadWallet) - { - mapWallet[hash] = wtxIn; - CWalletTx& wtx = mapWallet[hash]; - wtx.BindWallet(this); - wtxOrdered.insert(make_pair(wtx.nOrderPos, &wtx)); - UpdateNullifierNoteMapWithTx(mapWallet[hash]); - AddToSpends(hash); - } - else - { LOCK(cs_wallet); // Inserts only if not already there, returns tx inserted or tx found pair::iterator, bool> ret = mapWallet.insert(make_pair(hash, wtxIn)); @@ -2790,7 +2790,6 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD boost::replace_all(strCmd, "%s", wtxIn.GetHash().GetHex()); boost::thread t(runCommand, strCmd); // thread runs free } - } return true; } @@ -2846,7 +2845,7 @@ bool CWallet::UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx) */ bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pblock, const int nHeight, bool fUpdate) { - { + { // extra scope left in place for backport whitespace compatibility AssertLockHeld(cs_wallet); // Check whether the transaction is already known by the wallet. @@ -2891,10 +2890,10 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pbl // startup through our SetBestChain-mechanism CWalletDB walletdb(strWalletFile, "r+", false); - return AddToWallet(wtx, false, &walletdb); + return AddToWallet(wtx, &walletdb); } + return false; } - return false; } void CWallet::SyncTransaction(const CTransaction& tx, const CBlock* pblock, const int nHeight) @@ -5055,7 +5054,7 @@ bool CWallet::CommitTransaction(CWalletTx& wtxNew, std::optional setCoins; @@ -5932,6 +5931,7 @@ bool CWallet::InitLoadWallet(const CChainParams& params, bool clearWitnessCaches if (walletdb.ReadBestBlock(locator)) pindexRescan = FindForkInGlobalIndex(chainActive, locator); } + if (chainActive.Tip() && chainActive.Tip() != pindexRescan) { // We can't rescan beyond non-pruned blocks, stop and throw an error. diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c4e656c2f..8c563337f 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1561,7 +1561,8 @@ public: void UpdateNullifierNoteMapWithTx(const CWalletTx& wtx); void UpdateSaplingNullifierNoteMapWithTx(CWalletTx& wtx); void UpdateSaplingNullifierNoteMapForBlock(const CBlock* pblock); - bool AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletDB* pwalletdb); + void LoadWalletTx(const CWalletTx& wtxIn); + bool AddToWallet(const CWalletTx& wtxIn, CWalletDB* pwalletdb); void SyncTransaction(const CTransaction& tx, const CBlock* pblock, const int nHeight); bool AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pblock, const int nHeight, bool fUpdate); void EraseFromWallet(const uint256 &hash); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index a907fa2c4..9af11edaa 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -413,7 +413,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, if (wtx.nOrderPos == -1) wss.fAnyUnordered = true; - pwallet->AddToWallet(wtx, true, NULL); + pwallet->LoadWalletTx(wtx); } else if (strType == "watchs") { diff --git a/src/zcbenchmarks.cpp b/src/zcbenchmarks.cpp index 9ff6f0a23..4f2b658b8 100644 --- a/src/zcbenchmarks.cpp +++ b/src/zcbenchmarks.cpp @@ -342,7 +342,7 @@ double benchmark_increment_sprout_note_witnesses(size_t nTxs) CBlock block1; for (int i = 0; i < nTxs; ++i) { auto wtx = CreateSproutTxWithNoteData(sproutSpendingKey); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); block1.vtx.push_back(wtx); } @@ -357,7 +357,7 @@ double benchmark_increment_sprout_note_witnesses(size_t nTxs) block2.hashPrevBlock = block1.GetHash(); { auto sproutTx = CreateSproutTxWithNoteData(sproutSpendingKey); - wallet.AddToWallet(sproutTx, true, NULL); + wallet.LoadWalletTx(sproutTx); block2.vtx.push_back(sproutTx); } @@ -404,7 +404,7 @@ double benchmark_increment_sapling_note_witnesses(size_t nTxs) CBlock block1; for (int i = 0; i < nTxs; ++i) { auto wtx = CreateSaplingTxWithNoteData(consensusParams, wallet, saplingSpendingKey); - wallet.AddToWallet(wtx, true, NULL); + wallet.LoadWalletTx(wtx); block1.vtx.push_back(wtx); } @@ -419,7 +419,7 @@ double benchmark_increment_sapling_note_witnesses(size_t nTxs) block2.hashPrevBlock = block1.GetHash(); { auto saplingTx = CreateSaplingTxWithNoteData(consensusParams, wallet, saplingSpendingKey); - wallet.AddToWallet(saplingTx, true, NULL); + wallet.LoadWalletTx(saplingTx); block1.vtx.push_back(saplingTx); } From 5c049b6b13790e6f898c0f73326af110a3c26432 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 28 Feb 2022 13:06:35 -0700 Subject: [PATCH 04/16] Fix missing locks for GenerateNewUnifiedSpendingKey tests. --- src/wallet/gtest/test_wallet.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index 5be1820e8..71e0ba986 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -2203,6 +2203,9 @@ TEST(WalletTests, GenerateUnifiedAddress) { expected = WalletUAGenerationError::NoSuchAccount; EXPECT_EQ(uaResult, expected); + // lock required by GenerateNewUnifiedSpendingKey + LOCK(wallet.cs_wallet); + // Create an account, then generate an address for that account. auto ufvkpair = wallet.GenerateNewUnifiedSpendingKey(); auto ufvk = ufvkpair.first; @@ -2255,6 +2258,9 @@ TEST(WalletTests, GenerateUnifiedSpendingKeyAddsOrchardAddresses) { TestWallet wallet(Params()); wallet.GenerateNewSeed(); + // lock required by GenerateNewUnifiedSpendingKey + LOCK(wallet.cs_wallet); + // Create an account. auto ufvkpair = wallet.GenerateNewUnifiedSpendingKey(); auto ufvk = ufvkpair.first; From b78c2732abb3ace3b6874c35f6a25eb165e8ffe8 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 23 Feb 2022 12:08:08 -0700 Subject: [PATCH 05/16] Record when notes are detected as being spent in the Orchard wallet. --- src/consensus/params.cpp | 9 + src/consensus/params.h | 5 + src/rust/include/rust/orchard/wallet.h | 19 +- src/rust/src/wallet.rs | 309 ++++++++++++++++++---- src/wallet/asyncrpcoperation_sendmany.cpp | 6 +- src/wallet/gtest/test_orchard_wallet.cpp | 2 + src/wallet/gtest/test_wallet.cpp | 49 ++-- src/wallet/orchard.h | 28 +- src/wallet/rpcdump.cpp | 33 ++- src/wallet/wallet.cpp | 120 +++++++-- src/wallet/wallet.h | 37 ++- 11 files changed, 492 insertions(+), 125 deletions(-) diff --git a/src/consensus/params.cpp b/src/consensus/params.cpp index a8797b042..7d348da4a 100644 --- a/src/consensus/params.cpp +++ b/src/consensus/params.cpp @@ -12,6 +12,15 @@ #include "util/match.h" namespace Consensus { + std::optional Params::GetActivationHeight(Consensus::UpgradeIndex idx) const { + auto nActivationHeight = vUpgrades[idx].nActivationHeight; + if (nActivationHeight == Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT) { + return std::nullopt; + } else { + return nActivationHeight; + } + } + bool Params::NetworkUpgradeActive(int nHeight, Consensus::UpgradeIndex idx) const { return NetworkUpgradeState(nHeight, *this, idx) == UPGRADE_ACTIVE; } diff --git a/src/consensus/params.h b/src/consensus/params.h index 3411768d5..a24f7520c 100644 --- a/src/consensus/params.h +++ b/src/consensus/params.h @@ -225,6 +225,11 @@ static const unsigned int PRE_BLOSSOM_REGTEST_HALVING_INTERVAL = 144; * Parameters that influence chain consensus. */ struct Params { + /** + * Returns the activation height for the specified network upgrade, if any. + */ + std::optional GetActivationHeight(Consensus::UpgradeIndex idx) const; + /** * Returns true if the given network upgrade is active as of the given block * height. Caller must check that the height is >= 0 (and handle unknown diff --git a/src/rust/include/rust/orchard/wallet.h b/src/rust/include/rust/orchard/wallet.h index 521e7c9be..16cfc7f3f 100644 --- a/src/rust/include/rust/orchard/wallet.h +++ b/src/rust/include/rust/orchard/wallet.h @@ -34,20 +34,33 @@ void orchard_wallet_free(OrchardWalletPtr* wallet); * Adds a checkpoint to the wallet's note commitment tree to enable * a future rewind. */ -void orchard_wallet_checkpoint( - OrchardWalletPtr* wallet +bool orchard_wallet_checkpoint( + OrchardWalletPtr* wallet, + uint32_t blockHeight ); +/** + * Returns whether or not the wallet has any checkpointed state to + * which it can rewind. + */ +bool orchard_wallet_is_checkpointed(const OrchardWalletPtr* wallet); + /** * Rewinds to the most recently added checkpoint. */ bool orchard_wallet_rewind( - OrchardWalletPtr* wallet + OrchardWalletPtr* wallet, + uint32_t blockHeight, + uint32_t* blocksRewoundRet ); /** * Searches the provided bundle for notes that are visible to the specified * wallet's incoming viewing keys, and adds those notes to the wallet. + * + * The provided bundle must be a component of the transaction from which + * `txid` was derived, and the specified block height must be the height + * at which this transaction was mined. */ void orchard_wallet_add_notes_from_bundle( OrchardWalletPtr* wallet, diff --git a/src/rust/src/wallet.rs b/src/rust/src/wallet.rs index 2ff15f336..dc0c89a4b 100644 --- a/src/rust/src/wallet.rs +++ b/src/rust/src/wallet.rs @@ -1,6 +1,6 @@ use incrementalmerkletree::{bridgetree::BridgeTree, Frontier, Tree}; use libc::c_uchar; -use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet}; use tracing::error; use zcash_primitives::{ @@ -11,6 +11,7 @@ use zcash_primitives::{ use orchard::{ bundle::Authorized, keys::{FullViewingKey, IncomingViewingKey, SpendingKey}, + note::Nullifier, tree::MerkleHashOrchard, Address, Bundle, Note, }; @@ -25,11 +26,11 @@ pub const MAX_CHECKPOINTS: usize = 100; /// have been added to the wallet's note commitment tree. #[derive(Debug, Clone)] pub struct LastObserved { - height: BlockHeight, - block_tx_idx: usize, + block_height: BlockHeight, + block_tx_idx: Option, } -#[derive(Clone, PartialEq, Eq, Hash)] +#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] pub struct OutPoint { txid: TxId, action_idx: usize, @@ -41,7 +42,15 @@ pub struct DecryptedNote { memo: [u8; 512], } -struct TxNotes { +/// A data structure tracking the note data that was decrypted from a single transaction, +/// along with the height at which that transaction was discovered in the chain. +#[derive(Debug, Clone)] +pub struct TxNotes { + /// The block height containing the transaction from which these + /// notes were decrypted. + tx_height: Option, + /// A map from the index of the Orchard action from which this note + /// was decrypted to the decrypted note value. decrypted_notes: BTreeMap, } @@ -110,18 +119,30 @@ impl KeyStore { pub struct Wallet { /// The in-memory index of keys and addresses known to the wallet. key_store: KeyStore, - /// The in-memory index from txid to notes from the associated transaction - /// that have been decrypted with the IVKs known to this wallet. - wallet_tx_notes: HashMap, - /// The incremental merkle tree used to track note commitments - /// and witnesses for notes belonging to the wallet. + /// The in-memory index from txid to notes from the associated transaction that have + /// been decrypted with the IVKs known to this wallet. + wallet_received_notes: BTreeMap, + /// The in-memory index from nullifier to the outpoint of the note from which that + /// nullifier was derived. + nullifiers: BTreeMap, + /// The incremental merkle tree used to track note commitments and witnesses for notes + /// belonging to the wallet. witness_tree: BridgeTree, - /// The block height and transaction index of the note most recently added to `witness_tree` + /// The block height at which the last checkpoint was created, if any. + last_checkpoint: Option, + /// The block height and transaction index of the note most recently added to + /// `witness_tree` last_observed: Option, /// Notes marked as locked (currently reserved for pending transactions) - locked_notes: HashSet, - /// Notes marked as spent - spent_notes: HashSet, + locked_notes: BTreeSet, + /// Notes marked as spent as a consequence of their nullifiers having been + /// observed in bundle action inputs. The keys of this map are the notes + /// that have been spent, and the values are the outpoints identifying + /// the actions in which they are spent. + spent_notes: BTreeMap, + /// For each nullifier which appears more than once in transactions that this + /// wallet has observed, the set of outpoints corresponding to those nullifiers. + conflicts: BTreeMap>, } #[derive(Debug, Clone)] @@ -130,38 +151,169 @@ pub enum WalletError { NoteCommitmentTreeFull, } +#[derive(Debug, Clone)] +pub enum RewindError { + /// The witness tree does not contain enough checkpoints to + /// rewind to the requested height. The number of blocks that + /// it is possible to rewind is returned as the payload of + /// this error. + InsufficientCheckpoints(usize), +} + impl Wallet { pub fn empty() -> Self { Wallet { key_store: KeyStore::empty(), - wallet_tx_notes: HashMap::new(), + wallet_received_notes: BTreeMap::new(), + nullifiers: BTreeMap::new(), witness_tree: BridgeTree::new(MAX_CHECKPOINTS), + last_checkpoint: None, last_observed: None, - locked_notes: HashSet::new(), - spent_notes: HashSet::new(), + locked_notes: BTreeSet::new(), + spent_notes: BTreeMap::new(), + conflicts: BTreeMap::new(), } } - pub fn checkpoint_witness_tree(&mut self) { - self.witness_tree.checkpoint(); + /// Reset the state of the wallet to be suitable for rescan from the NU5 activation + /// height. This removes all witness and spentness information from the wallet. The + /// keystore is unmodified and decrypted note, nullifier, and conflict data are left + /// in place with the expectation that they will be overwritten and/or updated in + /// the rescan process. + pub fn reset(&mut self) { + for tx_notes in self.wallet_received_notes.values_mut() { + tx_notes.tx_height = None; + } + self.witness_tree = BridgeTree::new(MAX_CHECKPOINTS); + self.last_checkpoint = None; + self.last_observed = None; + self.locked_notes = BTreeSet::new(); + self.spent_notes = BTreeMap::new(); } - pub fn rewind_witness_tree(&mut self) -> bool { - self.witness_tree.rewind() + /// Checkpoints the note commitment tree. This returns `false` and leaves the note + /// commitment tree unmodified if the block height does not immediately succeed + /// the last checkpointed block height (unless the note commitment tree is empty, + /// in which case it unconditionally succeeds). This must be called exactly once + /// per block. + pub fn checkpoint(&mut self, block_height: BlockHeight) -> bool { + // checkpoints must be in order of sequential block height and every + // block must be checkpointed + if self + .last_checkpoint + .iter() + .any(|last_height| block_height != *last_height + 1) + { + return false; + } + + self.witness_tree.checkpoint(); + self.last_checkpoint = Some(block_height); + true + } + + /// Returns whether or not a checkpoint has been created. If no checkpoint exists, + /// the wallet has not yet observed any blocks. + pub fn is_checkpointed(&self) -> bool { + self.last_checkpoint.is_some() + } + + /// 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 { + if let Some(checkpoint_height) = self.last_checkpoint { + if to_height >= checkpoint_height { + return Ok(0); + } + + 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()); + } + + // 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_received_notes + .iter() + .filter_map(|(txid, n)| { + if n.tx_height + .map_or(true, |h| h <= last_observed.block_height) + { + Some(*txid) + } else { + None + } + }) + .collect(); + + self.spent_notes.retain(|_, v| to_retain.contains(&v.txid)); + self.nullifiers.retain(|_, v| to_retain.contains(&v.txid)); + for (txid, n) in self.wallet_received_notes.iter_mut() { + // Erase block height information for any received notes + // that have been un-mined by the rewind. + if !to_retain.contains(txid) { + n.tx_height = None; + } + } + self.last_observed = Some(last_observed); + + Ok(blocks_to_rewind) + } else { + Err(RewindError::InsufficientCheckpoints(0)) + } } /// Add note data for those notes that are decryptable with one of this wallet's - /// incoming viewing keys to the wallet, and return the indices of the actions - /// that we were able to decrypt. + /// incoming viewing keys to the wallet, and return the indices of the actions that we + /// were able to decrypt. pub fn add_notes_from_bundle( &mut self, txid: &TxId, bundle: &Bundle, ) -> Vec { let mut tx_notes = TxNotes { + tx_height: None, decrypted_notes: BTreeMap::new(), }; + // Check for spends of our notes by matching against the nullifiers + // we're tracking, and when we detect one, associate the current + // txid as spending the note. + for (action_idx, action) in bundle.actions().iter().enumerate() { + let nf = action.nullifier(); + if let Some(outpoint) = self.nullifiers.get(nf) { + self.spent_notes.insert( + *outpoint, + OutPoint { + txid: *txid, + action_idx, + }, + ); + } + } + let keys = self .key_store .viewing_keys @@ -170,27 +322,52 @@ impl Wallet { .collect::>(); let mut result = vec![]; for (action_idx, ivk, note, recipient, memo) in bundle.decrypt_outputs_for_keys(&keys) { - // Mark the witness tree with the fact that we want to be able to compute - // a witness for this note. - let note_data = DecryptedNote { note, memo }; + let outpoint = OutPoint { + txid: *txid, + action_idx, + }; + // Generate the nullifier for the received note and add it to the nullifiers map so + // that we can detect when the note is later spent. + if let Some(fvk) = self.key_store.viewing_keys.get(&ivk) { + let nf = note.nullifier(fvk); + // if we already have an entry for this nullifier that does not correspond + // to the current outpoint, then add the existing outpoint to the + // conflicts map. + if let Some(op) = self.nullifiers.get(&nf) { + if op != &outpoint { + let conflicts = self.conflicts.entry(nf).or_insert_with(BTreeSet::new); + conflicts.insert(*op); + conflicts.insert(outpoint); + }; + } + + self.nullifiers.insert(nf, outpoint); + } + + // add the decrypted note data to the wallet + let note_data = DecryptedNote { note, memo }; tx_notes.decrypted_notes.insert(action_idx, note_data); + + // add the association between the address and the IVK used + // to decrypt the note self.key_store.add_raw_address(recipient, ivk); result.push(action_idx); } if !tx_notes.decrypted_notes.is_empty() { - self.wallet_tx_notes.insert(*txid, tx_notes); + self.wallet_received_notes.insert(*txid, tx_notes); } result } - /// Add note commitments for the Orchard components of a transaction to the note commitment - /// tree, and mark the tree at the notes decryptable by this wallet so that in the future - /// we can produce authentication paths to those notes. + /// Add note commitments for the Orchard components of a transaction to the note + /// commitment tree, and mark the tree at the notes decryptable by this wallet so that + /// in the future we can produce authentication paths to those notes. /// - /// * `block_height` - Height of the block containing the transaction that provided this bundle. + /// * `block_height` - Height of the block containing the transaction that provided + /// this bundle. /// * `block_tx_idx` - Index of the transaction within the block /// * `txid` - Identifier of the transaction. /// * `bundle` - Orchard component of the transaction. @@ -204,9 +381,12 @@ impl Wallet { // Check that the wallet is in the correct state to update the note commitment tree with // new outputs. if let Some(last) = &self.last_observed { - if !((block_height == last.height && block_tx_idx == last.block_tx_idx + 1) - || (block_height == last.height + 1 && block_tx_idx == 0)) - { + if !( + // we are observing a subsequent transaction in the same block + (block_height == last.block_height && last.block_tx_idx.map_or(false, |idx| idx < block_tx_idx)) + // or we are observing a new block + || block_height > last.block_height + ) { return Err(WalletError::OutOfOrder( last.clone(), block_height, @@ -215,8 +395,19 @@ impl Wallet { } } - let tx_notes = self.wallet_tx_notes.get(txid); + self.last_observed = Some(LastObserved { + block_height, + block_tx_idx: Some(block_tx_idx), + }); + + // update the block height recorded for the transaction + if let Some(tx_notes) = self.wallet_received_notes.get_mut(txid) { + tx_notes.tx_height = Some(block_height); + } + + let my_notes_for_tx = self.wallet_received_notes.get(txid); for (action_idx, action) in bundle.actions().iter().enumerate() { + // append the note commitment for each action to the witness tree if !self .witness_tree .append(&MerkleHashOrchard::from_cmx(action.cmx())) @@ -224,18 +415,23 @@ impl Wallet { return Err(WalletError::NoteCommitmentTreeFull); } - if let Some(tx_notes) = tx_notes { - if tx_notes.decrypted_notes.contains_key(&action_idx) { - self.witness_tree.witness(); - } + // for notes that are ours, witness the current state of the tree + if my_notes_for_tx + .iter() + .any(|n| n.decrypted_notes.contains_key(&action_idx)) + { + self.witness_tree.witness(); } } Ok(()) } + /// Returns whether the transaction contains any notes either sent to or spent by this + /// wallet. pub fn tx_contains_my_notes(&self, txid: &TxId) -> bool { - self.wallet_tx_notes.get(txid).is_some() + self.wallet_received_notes.get(txid).is_some() + || self.nullifiers.values().any(|v| v.txid == *txid) } pub fn get_filtered_notes( @@ -245,7 +441,7 @@ impl Wallet { ignore_locked: bool, require_spending_key: bool, ) -> Vec<(OutPoint, DecryptedNote)> { - self.wallet_tx_notes + self.wallet_received_notes .iter() .flat_map(|(txid, tx_notes)| { tx_notes @@ -260,9 +456,9 @@ impl Wallet { self.key_store .ivk_for_address(&dnote.note.recipient()) // if `ivk` is `None`, return all notes that match the other conditions - .filter(|dnote_ivk| ivk.iter().all(|ivk| ivk == dnote_ivk)) + .filter(|dnote_ivk| ivk.map_or(true, |ivk| &ivk == dnote_ivk)) .and_then(|dnote_ivk| { - if (ignore_spent && self.spent_notes.contains(&outpoint)) + if (ignore_spent && self.spent_notes.contains_key(&outpoint)) || (ignore_locked && self.locked_notes.contains(&outpoint)) || (require_spending_key && self.key_store.spending_key_for_ivk(dnote_ivk).is_none()) @@ -292,15 +488,34 @@ pub extern "C" fn orchard_wallet_free(wallet: *mut Wallet) { } #[no_mangle] -pub extern "C" fn orchard_wallet_checkpoint(wallet: *mut Wallet) { +pub extern "C" fn orchard_wallet_checkpoint(wallet: *mut Wallet, block_height: u32) -> bool { let wallet = unsafe { wallet.as_mut() }.expect("Wallet pointer may not be null"); - wallet.checkpoint_witness_tree(); + wallet.checkpoint(block_height.into()) } #[no_mangle] -pub extern "C" fn orchard_wallet_rewind(wallet: *mut Wallet) -> bool { +pub extern "C" fn orchard_wallet_is_checkpointed(wallet: *const Wallet) -> bool { + let wallet = unsafe { wallet.as_ref() }.expect("Wallet pointer may not be null"); + wallet.is_checkpointed() +} + +#[no_mangle] +pub extern "C" fn orchard_wallet_rewind( + wallet: *mut Wallet, + to_height: BlockHeight, + blocks_rewound: *mut u32) -> bool { let wallet = unsafe { wallet.as_mut() }.expect("Wallet pointer may not be null"); - wallet.rewind_witness_tree() + let blocks_rewound = unsafe { blocks_rewound.as_mut() }.expect("Return value pointer may not be null."); + match wallet.rewind(to_height) { + Ok(rewound) => { + *blocks_rewound = rewound; + true + } + Err(e) => { + error!("Unable to rewind the wallet to height {:?}: {:?}", to_height, e); + false + } + } } #[no_mangle] diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 7ea734580..520f5aa20 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -205,7 +205,11 @@ uint256 AsyncRPCOperation_sendmany::main_impl() { // Find spendable inputs, and select a minimal set of them that // can supply the required target amount. - auto spendable = pwalletMain->FindSpendableInputs(ztxoSelector_, allowTransparentCoinbase, mindepth_); + SpendableInputs spendable; + { + LOCK2(cs_main, pwalletMain->cs_wallet); + spendable = pwalletMain->FindSpendableInputs(ztxoSelector_, allowTransparentCoinbase, mindepth_); + } if (!spendable.LimitToAmount(targetAmount, dustThreshold)) { CAmount changeAmount{spendable.Total() - targetAmount}; if (changeAmount > 0 && changeAmount < dustThreshold) { diff --git a/src/wallet/gtest/test_orchard_wallet.cpp b/src/wallet/gtest/test_orchard_wallet.cpp index 2ee08c565..2d99aade6 100644 --- a/src/wallet/gtest/test_orchard_wallet.cpp +++ b/src/wallet/gtest/test_orchard_wallet.cpp @@ -72,4 +72,6 @@ TEST(OrchardWalletTests, TxContainsMyNotes) { auto tx2 = FakeOrchardTx(skNotOurs, libzcash::diversifier_index_t(0)); wallet.AddNotesIfInvolvingMe(tx2); EXPECT_FALSE(wallet.TxContainsMyNotes(tx2.GetHash())); + + RegtestDeactivateNU5(); } diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index 71e0ba986..c15cf1974 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -56,14 +56,19 @@ public: return CCryptoKeyStore::Unlock(vMasterKeyIn); } - void IncrementNoteWitnesses(const CBlockIndex* pindex, + void IncrementNoteWitnesses(const Consensus::Params& consensus, + const CBlockIndex* pindex, const CBlock* pblock, SproutMerkleTree& sproutTree, - SaplingMerkleTree& saplingTree) { - CWallet::IncrementNoteWitnesses(pindex, pblock, sproutTree, saplingTree); + SaplingMerkleTree& saplingTree, + bool performOrchardWalletUpdates) { + CWallet::IncrementNoteWitnesses( + consensus, pindex, pblock, sproutTree, saplingTree, performOrchardWalletUpdates); } - void DecrementNoteWitnesses(const CBlockIndex* pindex) { - CWallet::DecrementNoteWitnesses(pindex); + + + void DecrementNoteWitnesses(const Consensus::Params& consensus, const CBlockIndex* pindex) { + CWallet::DecrementNoteWitnesses(consensus, pindex); } void SetBestChain(MockWalletDB& walletdb, const CBlockLocator& loc) { CWallet::SetBestChainINTERNAL(walletdb, loc); @@ -105,7 +110,7 @@ std::pair CreateValidBlock(TestWallet& wallet, wallet.LoadWalletTx(wtx); block.vtx.push_back(wtx); - wallet.IncrementNoteWitnesses(&index, &block, sproutTree, saplingTree); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index, &block, sproutTree, saplingTree, true); return std::make_pair(jsoutpt, saplingNotes[0]); } @@ -726,7 +731,7 @@ TEST(WalletTests, GetConflictedSaplingNotes) { wallet.LoadWalletTx(wtx); // Simulate receiving new block and ChainTip signal - wallet.IncrementNoteWitnesses(&fakeIndex, &block, sproutTree, saplingTree); + wallet.IncrementNoteWitnesses(Params().GetConsensus(),&fakeIndex, &block, sproutTree, saplingTree, true); wallet.UpdateSaplingNullifierNoteMapForBlock(&block); // Retrieve the updated wtx from wallet @@ -984,7 +989,7 @@ TEST(WalletTests, NavigateFromSaplingNullifierToNote) { } // Simulate receiving new block and ChainTip signal - wallet.IncrementNoteWitnesses(&fakeIndex, &block, sproutTree, testNote.tree); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &fakeIndex, &block, sproutTree, testNote.tree, true); wallet.UpdateSaplingNullifierNoteMapForBlock(&block); // Retrieve the updated wtx from wallet @@ -1102,7 +1107,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(&fakeIndex, &block, sproutTree, saplingTree); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &fakeIndex, &block, sproutTree, saplingTree, true); wallet.UpdateSaplingNullifierNoteMapForBlock(&block); // Retrieve the updated wtx from wallet @@ -1239,7 +1244,7 @@ TEST(WalletTests, CachedWitnessesEmptyChain) { CBlockIndex index(block); SproutMerkleTree sproutTree; SaplingMerkleTree saplingTree; - wallet.IncrementNoteWitnesses(&index, &block, sproutTree, saplingTree); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index, &block, sproutTree, saplingTree, true); ::GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); @@ -1248,7 +1253,7 @@ TEST(WalletTests, CachedWitnessesEmptyChain) { EXPECT_TRUE((bool) saplingWitnesses[0]); // Until #1302 is implemented, this should trigger an assertion - EXPECT_DEATH(wallet.DecrementNoteWitnesses(&index), + EXPECT_DEATH(wallet.DecrementNoteWitnesses(Params().GetConsensus(), &index), ".*nWitnessCacheSize > 0.*"); } @@ -1312,7 +1317,7 @@ TEST(WalletTests, CachedWitnessesChainTip) { index2.nHeight = 2; SproutMerkleTree sproutTree2 {sproutTree}; SaplingMerkleTree saplingTree2 {saplingTree}; - wallet.IncrementNoteWitnesses(&index2, &block2, sproutTree2, saplingTree2); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index2, &block2, sproutTree2, saplingTree2, true); auto anchors2 = GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); EXPECT_NE(anchors2.first, anchors2.second); @@ -1323,7 +1328,7 @@ TEST(WalletTests, CachedWitnessesChainTip) { EXPECT_NE(anchors1.second, anchors2.second); // Decrementing should give us the previous anchor - wallet.DecrementNoteWitnesses(&index2); + wallet.DecrementNoteWitnesses(Params().GetConsensus(), &index2); auto anchors3 = GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); EXPECT_FALSE((bool) sproutWitnesses[0]); @@ -1333,7 +1338,7 @@ TEST(WalletTests, CachedWitnessesChainTip) { EXPECT_NE(anchors1.second, anchors3.second); // Re-incrementing with the same block should give the same result - wallet.IncrementNoteWitnesses(&index2, &block2, sproutTree, saplingTree); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index2, &block2, sproutTree, saplingTree, true); auto anchors4 = GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); EXPECT_NE(anchors4.first, anchors4.second); @@ -1343,7 +1348,7 @@ TEST(WalletTests, CachedWitnessesChainTip) { EXPECT_EQ(anchors2.second, anchors4.second); // Incrementing with the same block again should not change the cache - wallet.IncrementNoteWitnesses(&index2, &block2, sproutTree, saplingTree); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index2, &block2, sproutTree, saplingTree, true); std::vector> sproutWitnesses5; std::vector> saplingWitnesses5; @@ -1418,7 +1423,7 @@ TEST(WalletTests, CachedWitnessesDecrementFirst) { // Decrementing (before the transaction has ever seen an increment) // should give us the previous anchor - wallet.DecrementNoteWitnesses(&index2); + wallet.DecrementNoteWitnesses(Params().GetConsensus(), &index2); auto anchors4 = GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); @@ -1429,7 +1434,7 @@ TEST(WalletTests, CachedWitnessesDecrementFirst) { EXPECT_NE(anchors2.second, anchors4.second); // Re-incrementing with the same block should give the same result - wallet.IncrementNoteWitnesses(&index2, &block2, sproutTree, saplingTree); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &index2, &block2, sproutTree, saplingTree, true); auto anchors5 = GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); @@ -1489,7 +1494,7 @@ TEST(WalletTests, CachedWitnessesCleanIndex) { for (size_t i = 0; i < numBlocks; i++) { SproutMerkleTree sproutRiPrevTree {sproutRiTree}; SaplingMerkleTree saplingRiPrevTree {saplingRiTree}; - wallet.IncrementNoteWitnesses(&(indices[i]), &(blocks[i]), sproutRiTree, saplingRiTree); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &(indices[i]), &(blocks[i]), sproutRiTree, saplingRiTree, true); auto anchors = GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); for (size_t j = 0; j < numBlocks; j++) { @@ -1503,7 +1508,7 @@ TEST(WalletTests, CachedWitnessesCleanIndex) { if ((i == 5) || (i == 50)) { // Pretend a reorg happened that was recorded in the block files { - wallet.DecrementNoteWitnesses(&(indices[i])); + wallet.DecrementNoteWitnesses(Params().GetConsensus(), &(indices[i])); auto anchors = GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); for (size_t j = 0; j < numBlocks; j++) { @@ -1516,7 +1521,7 @@ TEST(WalletTests, CachedWitnessesCleanIndex) { } { - wallet.IncrementNoteWitnesses(&(indices[i]), &(blocks[i]), sproutRiPrevTree, saplingRiPrevTree); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &(indices[i]), &(blocks[i]), sproutRiPrevTree, saplingRiPrevTree, true); auto anchors = GetWitnessesAndAnchors(wallet, sproutNotes, saplingNotes, sproutWitnesses, saplingWitnesses); for (size_t j = 0; j < numBlocks; j++) { EXPECT_TRUE((bool) sproutWitnesses[j]); @@ -1910,7 +1915,7 @@ TEST(WalletTests, UpdatedSaplingNoteData) { wallet.LoadWalletTx(wtx); // Simulate receiving new block and ChainTip signal - wallet.IncrementNoteWitnesses(&fakeIndex, &block, sproutTree, testNote.tree); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &fakeIndex, &block, sproutTree, testNote.tree, true); wallet.UpdateSaplingNullifierNoteMapForBlock(&block); // Retrieve the updated wtx from wallet @@ -2057,7 +2062,7 @@ TEST(WalletTests, MarkAffectedSaplingTransactionsDirty) { wallet.LoadWalletTx(wtx); // Simulate receiving new block and ChainTip signal - wallet.IncrementNoteWitnesses(&fakeIndex, &block, sproutTree, saplingTree); + wallet.IncrementNoteWitnesses(Params().GetConsensus(), &fakeIndex, &block, sproutTree, saplingTree, true); wallet.UpdateSaplingNullifierNoteMapForBlock(&block); // Retrieve the updated wtx from wallet diff --git a/src/wallet/orchard.h b/src/wallet/orchard.h index 4d660b2d6..0871bcb3e 100644 --- a/src/wallet/orchard.h +++ b/src/wallet/orchard.h @@ -65,12 +65,32 @@ public: OrchardWallet(const OrchardWallet&) = delete; OrchardWallet& operator=(const OrchardWallet&) = delete; - void CheckpointNoteCommitmentTree() { - orchard_wallet_checkpoint(inner.get()); + /** + * Checkpoint the note commitment tree. This returns `false` and leaves the + * note commitment tree unmodified if the block height does not match the + * last block height scanned for transactions. This must be called exactly + * once per block. + */ + bool CheckpointNoteCommitmentTree(int nBlockHeight) { + return orchard_wallet_checkpoint(inner.get(), (uint32_t) nBlockHeight); } - bool RewindToLastCheckpoint() { - return orchard_wallet_rewind(inner.get()); + /** + * Rewind to the most recent checkpoint, and mark as unspent any notes + * previously identified as having been spent by transactions in the + * latest block. + */ + bool IsCheckpointed() const { + return orchard_wallet_is_checkpointed(inner.get()); + } + + /** + * Rewind to the most recent checkpoint, and mark as unspent any notes + * previously identified as having been spent by transactions in the + * latest block. + */ + bool Rewind(int nBlockHeight, uint32_t& blocksRewoundRet) { + return orchard_wallet_rewind(inner.get(), (uint32_t) nBlockHeight, &blocksRewoundRet); } /** diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 9874eab4f..b4fe8adc7 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -120,7 +120,8 @@ UniValue importprivkey(const UniValue& params, bool fHelp) if (params.size() > 2) fRescan = params[2].get_bool(); - KeyIO keyIO(Params()); + const auto& chainparams = Params(); + KeyIO keyIO(chainparams); CKey key = keyIO.DecodeSecret(strSecret); if (!key.IsValid()) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key encoding"); @@ -146,7 +147,7 @@ UniValue importprivkey(const UniValue& params, bool fHelp) pwalletMain->nTimeFirstKey = 1; // 0 would be considered 'no value' if (fRescan) { - pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true); + pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true, false); } } @@ -233,7 +234,8 @@ UniValue importaddress(const UniValue& params, bool fHelp) LOCK2(cs_main, pwalletMain->cs_wallet); - KeyIO keyIO(Params()); + const auto& chainparams = Params(); + KeyIO keyIO(chainparams); CTxDestination dest = keyIO.DecodeDestination(params[0].get_str()); if (IsValidDestination(dest)) { if (fP2SH) { @@ -249,7 +251,7 @@ UniValue importaddress(const UniValue& params, bool fHelp) if (fRescan) { - pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true); + pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true, false); pwalletMain->ReacceptWalletTransactions(); } @@ -305,7 +307,7 @@ UniValue importpubkey(const UniValue& params, bool fHelp) if (fRescan) { - pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true); + pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true, false); pwalletMain->ReacceptWalletTransactions(); } @@ -380,7 +382,8 @@ UniValue importwallet_impl(const UniValue& params, bool fImportZKeys) int64_t nFilesize = std::max((int64_t)1, (int64_t)file.tellg()); file.seekg(0, file.beg); - KeyIO keyIO(Params()); + const auto& chainparams = Params(); + KeyIO keyIO(chainparams); pwalletMain->ShowProgress(_("Importing..."), 0); // show progress dialog in GUI while (file.good()) { @@ -404,7 +407,7 @@ UniValue importwallet_impl(const UniValue& params, bool fImportZKeys) std::optional seedFpStr = (vstr.size() > 3) ? std::optional(vstr[3]) : std::nullopt; if (spendingkey.has_value()) { auto addResult = std::visit( - AddSpendingKeyToWallet(pwalletMain, Params().GetConsensus(), nTime, hdKeypath, seedFpStr, true, true), spendingkey.value()); + AddSpendingKeyToWallet(pwalletMain, chainparams.GetConsensus(), nTime, hdKeypath, seedFpStr, true, true), spendingkey.value()); if (addResult == KeyAlreadyExists){ LogPrint("zrpc", "Skipping import of zaddr (key already present)\n"); } else if (addResult == KeyNotAdded) { @@ -465,7 +468,7 @@ UniValue importwallet_impl(const UniValue& params, bool fImportZKeys) pwalletMain->nTimeFirstKey = nTimeBegin; LogPrintf("Rescanning last %i blocks\n", chainActive.Height() - pindex->nHeight + 1); - pwalletMain->ScanForWalletTransactions(pindex); + pwalletMain->ScanForWalletTransactions(pindex, false, false); pwalletMain->MarkDirty(); if (!fGood) @@ -771,7 +774,8 @@ UniValue z_importkey(const UniValue& params, bool fHelp) throw JSONRPCError(RPC_INVALID_PARAMETER, "Block height out of range"); } - KeyIO keyIO(Params()); + const auto& chainparams = Params(); + KeyIO keyIO(chainparams); string strSecret = params[0].get_str(); auto spendingkey = keyIO.DecodeSpendingKey(strSecret); if (!spendingkey.has_value()) { @@ -784,7 +788,7 @@ UniValue z_importkey(const UniValue& params, bool fHelp) result.pushKV("address", keyIO.EncodePaymentAddress(addrInfo.second)); // Sapling support - auto addResult = std::visit(AddSpendingKeyToWallet(pwalletMain, Params().GetConsensus()), spendingkey.value()); + auto addResult = std::visit(AddSpendingKeyToWallet(pwalletMain, chainparams.GetConsensus()), spendingkey.value()); if (addResult == KeyAlreadyExists && fIgnoreExistingKey) { return result; } @@ -798,7 +802,7 @@ UniValue z_importkey(const UniValue& params, bool fHelp) // We want to scan for transactions and notes if (fRescan) { - pwalletMain->ScanForWalletTransactions(chainActive[nRescanHeight], true); + pwalletMain->ScanForWalletTransactions(chainActive[nRescanHeight], true, false); } return result; @@ -866,14 +870,15 @@ UniValue z_importviewingkey(const UniValue& params, bool fHelp) throw JSONRPCError(RPC_INVALID_PARAMETER, "Block height out of range"); } - KeyIO keyIO(Params()); + const auto& chainparams = Params(); + KeyIO keyIO(chainparams); string strVKey = params[0].get_str(); auto viewingkey = keyIO.DecodeViewingKey(strVKey); if (!viewingkey.has_value()) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid viewing key"); } - auto addrInfo = std::visit(libzcash::AddressInfoFromViewingKey(Params()), viewingkey.value()); + auto addrInfo = std::visit(libzcash::AddressInfoFromViewingKey(chainparams), viewingkey.value()); UniValue result(UniValue::VOBJ); const string strAddress = keyIO.EncodePaymentAddress(addrInfo.second); result.pushKV("type", addrInfo.first); @@ -894,7 +899,7 @@ UniValue z_importviewingkey(const UniValue& params, bool fHelp) // We want to scan for transactions and notes if (fRescan) { - pwalletMain->ScanForWalletTransactions(chainActive[nRescanHeight], true); + pwalletMain->ScanForWalletTransactions(chainActive[nRescanHeight], true, false); } return result; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9fb627c5c..a3727df65 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1169,9 +1169,13 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, void CWallet::ChainTipAdded(const CBlockIndex *pindex, const CBlock *pblock, SproutMerkleTree sproutTree, - SaplingMerkleTree saplingTree) + SaplingMerkleTree saplingTree, + bool performOrchardWalletUpdates) { - IncrementNoteWitnesses(pindex, pblock, sproutTree, saplingTree); + IncrementNoteWitnesses( + Params().GetConsensus(), + pindex, pblock, + sproutTree, saplingTree, performOrchardWalletUpdates); UpdateSaplingNullifierNoteMapForBlock(pblock); // SetBestChain() can be expensive for large wallets, so do only @@ -1203,17 +1207,18 @@ void CWallet::ChainTip(const CBlockIndex *pindex, // witnesses / rewind the tree std::optional> added) { + const auto& consensus = Params().GetConsensus(); if (added.has_value()) { - ChainTipAdded(pindex, pblock, added->first, added->second); + ChainTipAdded(pindex, pblock, added->first, added->second, 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(Params().GetConsensus()) && + if (!IsInitialBlockDownload(consensus) && pblock->GetBlockTime() > GetTime() - 3 * 60 * 60) { RunSaplingMigration(pindex->nHeight); } } else { - DecrementNoteWitnesses(pindex); + DecrementNoteWitnesses(consensus, pindex); UpdateSaplingNullifierNoteMapForBlock(pblock); } } @@ -1434,6 +1439,7 @@ set CWallet::GetConflicts(const uint256& txid) const } range_n = mapTxSproutNullifiers.equal_range(nullifier); for (TxNullifiers::const_iterator it = range_n.first; it != range_n.second; ++it) { + // TODO: Take into account transaction expiry for v4 transactions; see #5585 result.insert(it->second); } } @@ -1448,9 +1454,13 @@ set CWallet::GetConflicts(const uint256& txid) const } range_o = mapTxSaplingNullifiers.equal_range(nullifier); for (TxNullifiers::const_iterator it = range_o.first; it != range_o.second; ++it) { + // TODO: Take into account transaction expiry; see #5585 result.insert(it->second); } } + + // TODO ORCHARD; see #5593 + return result; } @@ -2238,10 +2248,13 @@ void UpdateWitnessHeights(NoteDataMap& noteDataMap, int indexHeight, int64_t nWi } } -void CWallet::IncrementNoteWitnesses(const CBlockIndex* pindex, - const CBlock* pblockIn, - SproutMerkleTree& sproutTree, - SaplingMerkleTree& saplingTree) +void CWallet::IncrementNoteWitnesses( + const Consensus::Params& consensus, + const CBlockIndex* pindex, + const CBlock* pblockIn, + SproutMerkleTree& sproutTree, + SaplingMerkleTree& saplingTree, + bool performOrchardWalletUpdates) { LOCK(cs_wallet); for (std::pair& wtxItem : mapWallet) { @@ -2249,8 +2262,9 @@ void CWallet::IncrementNoteWitnesses(const CBlockIndex* pindex, ::CopyPreviousWitnesses(wtxItem.second.mapSaplingNoteData, pindex->nHeight, nWitnessCacheSize); } - // ORCHARD: Add a checkpoint before we add information from the new block - orchardWallet.CheckpointNoteCommitmentTree(); + if (performOrchardWalletUpdates && consensus.NetworkUpgradeActive(pindex->nHeight, Consensus::UPGRADE_NU5)) { + assert(orchardWallet.CheckpointNoteCommitmentTree(pindex->nHeight)); + } if (nWitnessCacheSize < WITNESS_CACHE_SIZE) { nWitnessCacheSize += 1; @@ -2259,7 +2273,7 @@ void CWallet::IncrementNoteWitnesses(const CBlockIndex* pindex, const CBlock* pblock {pblockIn}; CBlock block; if (!pblock) { - ReadBlockFromDisk(block, pindex, Params().GetConsensus()); + ReadBlockFromDisk(block, pindex, consensus); pblock = █ } @@ -2303,8 +2317,10 @@ void CWallet::IncrementNoteWitnesses(const CBlockIndex* pindex, } } - // ORCHARD: Update the Orchard note commitment tree. - assert(orchardWallet.AppendNoteCommitments(pindex->nHeight, *pblock)); + // 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)); + } // Update witness heights for (std::pair& wtxItem : mapWallet) { @@ -2357,7 +2373,7 @@ void DecrementNoteWitnesses(NoteDataMap& noteDataMap, int indexHeight, int64_t n } } -void CWallet::DecrementNoteWitnesses(const CBlockIndex* pindex) +void CWallet::DecrementNoteWitnesses(const Consensus::Params& consensus, const CBlockIndex* pindex) { LOCK(cs_wallet); for (std::pair& wtxItem : mapWallet) { @@ -2369,7 +2385,13 @@ void CWallet::DecrementNoteWitnesses(const CBlockIndex* pindex) assert(nWitnessCacheSize > 0); // ORCHARD: rewind to the last checkpoint. - orchardWallet.RewindToLastCheckpoint(); + 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}; + assert(orchardWallet.Rewind(pindex->nHeight - 1, blocksRewound)); + assert(blocksRewound == 1); + } // For performance reasons, we write out the witness cache in // CWallet::SetBestChain() (which also ensures that overall consistency @@ -2843,7 +2865,13 @@ bool CWallet::UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx) * updated; instead, the transaction being in the mempool or conflicted is determined on * the fly in CMerkleTx::GetDepthInMainChain(). */ -bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pblock, const int nHeight, bool fUpdate) +bool CWallet::AddToWalletIfInvolvingMe( + const Consensus::Params& consensus, + const CTransaction& tx, + const CBlock* pblock, + const int nHeight, + bool fUpdate + ) { { // extra scope left in place for backport whitespace compatibility AssertLockHeld(cs_wallet); @@ -2867,7 +2895,9 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pbl } // Orchard - orchardWallet.AddNotesIfInvolvingMe(tx); + if (consensus.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_NU5)) { + orchardWallet.AddNotesIfInvolvingMe(tx); + } if (fExisted || IsMine(tx) || IsFromMe(tx) || sproutNoteData.size() > 0 || saplingNoteData.size() > 0 || orchardWallet.TxContainsMyNotes(tx.GetHash())) { @@ -2899,7 +2929,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pbl void CWallet::SyncTransaction(const CTransaction& tx, const CBlock* pblock, const int nHeight) { LOCK(cs_wallet); - if (!AddToWalletIfInvolvingMe(tx, pblock, nHeight, true)) + if (!AddToWalletIfInvolvingMe(Params().GetConsensus(), tx, pblock, nHeight, true)) return; // Not one of ours MarkAffectedTransactionsDirty(tx); @@ -3880,11 +3910,15 @@ void CWallet::WitnessNoteCommitment(std::vector commitments, * from or to us. If fUpdate is true, found transactions that already * exist in the wallet will be updated. */ -int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate) +int CWallet::ScanForWalletTransactions( + CBlockIndex* pindexStart, + bool fUpdate, + bool isInitScan) { int ret = 0; int64_t nNow = GetTime(); const CChainParams& chainParams = Params(); + const auto& consensus = chainParams.GetConsensus(); CBlockIndex* pindex = pindexStart; @@ -3899,6 +3933,37 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate) pindex = chainActive.Next(pindex); } + // Attempt to rewind the orchard wallet to the rescan point if the wallet has any + // checkpoints. Note data will be restored by the calls to AddToWalletIfInvolvingMe, + // and then the call to `ChainTipAdded` that later occurs for each block will restore + // the witness data that is being removed in the rewind here. + bool performOrchardWalletUpdates{false}; + if (orchardWallet.IsCheckpointed()) { + // We have a checkpoint, so attempt to rewind the Orchard wallet at most as + // far as the NU5 activation block. + auto nu5_height = chainParams.GetConsensus().GetActivationHeight(Consensus::UPGRADE_NU5); + // If there's no activation height, we shouldn't have a checkpoint already, + // and this is a programming error. + assert(nu5_height.has_value()); + // Only attempt to perform scans for Orchard during wallet initialization, + // since we do not support Orchard key import. + if (isInitScan) { + int rewindHeight = std::max(nu5_height.value(), pindex->nHeight); + uint32_t blocksRewound{0}; + if (orchardWallet.Rewind(rewindHeight, blocksRewound)) { + // rewind was successful or a no-op, so perform Orchard wallet updates + LogPrintf("Rewound Orchard wallet by %d blocks.", blocksRewound); + performOrchardWalletUpdates = true; + } else { + // Orchard witnesses will not be able to be correctly updated, because we + // can't rewind far enough. This is an unrecoverable failure; it means that we + // can't get back to a valid wallet state without resetting the wallet all + // the way back to NU5 activation. + throw std::runtime_error("CWallet::ScanForWalletTransactions(): Orchard wallet is out of sync. Please restart your node with -rescan."); + } + } + } + ShowProgress(_("Rescanning..."), 0); // show rescan progress in GUI as dialog or on splashscreen, if -rescan on startup double dProgressStart = Checkpoints::GuessVerificationProgress(chainParams.Checkpoints(), pindex, false); double dProgressTip = Checkpoints::GuessVerificationProgress(chainParams.Checkpoints(), chainActive.Tip(), false); @@ -3908,10 +3973,10 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate) ShowProgress(_("Rescanning..."), std::max(1, std::min(99, (int)((Checkpoints::GuessVerificationProgress(chainParams.Checkpoints(), pindex, false) - dProgressStart) / (dProgressTip - dProgressStart) * 100)))); CBlock block; - ReadBlockFromDisk(block, pindex, Params().GetConsensus()); + ReadBlockFromDisk(block, pindex, consensus); for (CTransaction& tx : block.vtx) { - if (AddToWalletIfInvolvingMe(tx, &block, pindex->nHeight, fUpdate)) { + if (AddToWalletIfInvolvingMe(consensus, tx, &block, pindex->nHeight, fUpdate)) { myTxHashes.push_back(tx.GetHash()); ret++; } @@ -3923,17 +3988,20 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate) // state on the path to the tip of our chain assert(pcoinsTip->GetSproutAnchorAt(pindex->hashSproutAnchor, sproutTree)); if (pindex->pprev) { - if (Params().GetConsensus().NetworkUpgradeActive(pindex->pprev->nHeight, Consensus::UPGRADE_SAPLING)) { + if (consensus.NetworkUpgradeActive(pindex->pprev->nHeight, Consensus::UPGRADE_SAPLING)) { assert(pcoinsTip->GetSaplingAnchorAt(pindex->pprev->hashFinalSaplingRoot, saplingTree)); } } // Increment note witness caches - ChainTipAdded(pindex, &block, sproutTree, saplingTree); + ChainTipAdded(pindex, &block, sproutTree, saplingTree, performOrchardWalletUpdates); pindex = chainActive.Next(pindex); if (GetTime() >= nNow + 60) { nNow = GetTime(); - LogPrintf("Still rescanning. At block %d. Progress=%f\n", pindex->nHeight, Checkpoints::GuessVerificationProgress(chainParams.Checkpoints(), pindex)); + LogPrintf( + "Still rescanning. At block %d. Progress=%f\n", + pindex->nHeight, + Checkpoints::GuessVerificationProgress(chainParams.Checkpoints(), pindex)); } } @@ -5950,7 +6018,7 @@ bool CWallet::InitLoadWallet(const CChainParams& params, bool clearWitnessCaches uiInterface.InitMessage(_("Rescanning...")); LogPrintf("Rescanning last %i blocks (from block %i)...\n", chainActive.Height() - pindexRescan->nHeight, pindexRescan->nHeight); nStart = GetTimeMillis(); - walletInstance->ScanForWalletTransactions(pindexRescan, true); + walletInstance->ScanForWalletTransactions(pindexRescan, true, true); LogPrintf(" rescan %15dms\n", GetTimeMillis() - nStart); walletInstance->SetBestChain(chainActive.GetLocator()); CWalletDB::IncrementUpdateCounter(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8c563337f..bfc8a63e5 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1026,14 +1026,21 @@ protected: /** * pindex is the new tip being connected. */ - void IncrementNoteWitnesses(const CBlockIndex* pindex, - const CBlock* pblock, - SproutMerkleTree& sproutTree, - SaplingMerkleTree& saplingTree); + void IncrementNoteWitnesses( + const Consensus::Params& consensus, + const CBlockIndex* pindex, + const CBlock* pblock, + SproutMerkleTree& sproutTree, + SaplingMerkleTree& saplingTree, + bool performOrchardWalletUpdates + ); /** * pindex is the old tip being disconnected. */ - void DecrementNoteWitnesses(const CBlockIndex* pindex); + void DecrementNoteWitnesses( + const Consensus::Params& consensus, + const CBlockIndex* pindex + ); template void SetBestChainINTERNAL(WalletDB& walletdb, const CBlockLocator& loc) { @@ -1085,7 +1092,12 @@ protected: private: template void SyncMetaData(std::pair::iterator, typename TxSpendMap::iterator>); - void ChainTipAdded(const CBlockIndex *pindex, const CBlock *pblock, SproutMerkleTree sproutTree, SaplingMerkleTree saplingTree); + void ChainTipAdded( + const CBlockIndex *pindex, + const CBlock *pblock, + SproutMerkleTree sproutTree, + SaplingMerkleTree saplingTree, + bool performOrchardWalletUpdates); /* Add a transparent secret key to the wallet. Internal use only. */ CPubKey AddTransparentSecretKey( @@ -1564,13 +1576,22 @@ public: void LoadWalletTx(const CWalletTx& wtxIn); bool AddToWallet(const CWalletTx& wtxIn, CWalletDB* pwalletdb); void SyncTransaction(const CTransaction& tx, const CBlock* pblock, const int nHeight); - bool AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pblock, const int nHeight, bool fUpdate); + bool AddToWalletIfInvolvingMe( + const Consensus::Params& consensus, + const CTransaction& tx, + const CBlock* pblock, + const int nHeight, + bool fUpdate + ); void EraseFromWallet(const uint256 &hash); void WitnessNoteCommitment( std::vector commitments, std::vector>& witnesses, uint256 &final_anchor); - int ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate = false); + int ScanForWalletTransactions( + CBlockIndex* pindexStart, + bool fUpdate, + bool isInitScan); void ReacceptWalletTransactions(); void ResendWalletTransactions(int64_t nBestBlockTime); std::vector ResendWalletTransactionsBefore(int64_t nTime); From 6b8ecc8a31f8e74d10c16de3f929456a59b54e38 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 2 Mar 2022 19:50:14 -0700 Subject: [PATCH 06/16] Reset Orchard wallet state when rescanning from below NU5 activation. --- src/rust/include/rust/orchard/wallet.h | 9 +++++++++ src/rust/src/wallet.rs | 6 ++++++ src/wallet/orchard.h | 22 +++++++++++++++------- src/wallet/wallet.cpp | 18 ++++++++++++++++-- 4 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/rust/include/rust/orchard/wallet.h b/src/rust/include/rust/orchard/wallet.h index 16cfc7f3f..f97191c9b 100644 --- a/src/rust/include/rust/orchard/wallet.h +++ b/src/rust/include/rust/orchard/wallet.h @@ -30,6 +30,15 @@ OrchardWalletPtr* orchard_wallet_new(); */ void orchard_wallet_free(OrchardWalletPtr* wallet); +/** + * Reset the state of the wallet to be suitable for rescan from the NU5 activation + * height. This removes all witness and spentness information from the wallet. The + * keystore is unmodified and decrypted note, nullifier, and conflict data are left + * in place with the expectation that they will be overwritten and/or updated in + * the rescan process. + */ +bool orchard_wallet_reset(OrchardWalletPtr* wallet); + /** * Adds a checkpoint to the wallet's note commitment tree to enable * a future rewind. diff --git a/src/rust/src/wallet.rs b/src/rust/src/wallet.rs index dc0c89a4b..2663165fc 100644 --- a/src/rust/src/wallet.rs +++ b/src/rust/src/wallet.rs @@ -487,6 +487,12 @@ pub extern "C" fn orchard_wallet_free(wallet: *mut Wallet) { } } +#[no_mangle] +pub extern "C" fn orchard_wallet_reset(wallet: *mut Wallet) { + let wallet = unsafe { wallet.as_mut() }.expect("Wallet pointer may not be null"); + wallet.reset(); +} + #[no_mangle] pub extern "C" fn orchard_wallet_checkpoint(wallet: *mut Wallet, block_height: u32) -> bool { let wallet = unsafe { wallet.as_mut() }.expect("Wallet pointer may not be null"); diff --git a/src/wallet/orchard.h b/src/wallet/orchard.h index 0871bcb3e..574fcc1a3 100644 --- a/src/wallet/orchard.h +++ b/src/wallet/orchard.h @@ -66,19 +66,27 @@ public: OrchardWallet& operator=(const OrchardWallet&) = delete; /** - * Checkpoint the note commitment tree. This returns `false` and leaves the - * note commitment tree unmodified if the block height does not match the - * last block height scanned for transactions. This must be called exactly - * once per block. + * Reset the state of the wallet to be suitable for rescan from the NU5 activation + * height. This removes all witness and spentness information from the wallet. The + * keystore is unmodified and decrypted note, nullifier, and conflict data are left + * 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()); + } + + /** + * Checkpoint the note commitment tree. This returns `false` and leaves the note + * commitment tree unmodified if the block height does not match the last block + * height scanned for transactions. This must be called exactly once per block. */ bool CheckpointNoteCommitmentTree(int nBlockHeight) { return orchard_wallet_checkpoint(inner.get(), (uint32_t) nBlockHeight); } /** - * Rewind to the most recent checkpoint, and mark as unspent any notes - * previously identified as having been spent by transactions in the - * latest block. + * Return whether the orchard note commitment tree contains any checkpoints. */ bool IsCheckpointed() const { return orchard_wallet_is_checkpointed(inner.get()); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a3727df65..9d15697e1 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3937,11 +3937,11 @@ int CWallet::ScanForWalletTransactions( // checkpoints. Note data will be restored by the calls to AddToWalletIfInvolvingMe, // and then the call to `ChainTipAdded` that later occurs for each block will restore // the witness data that is being removed in the rewind here. + auto nu5_height = chainParams.GetConsensus().GetActivationHeight(Consensus::UPGRADE_NU5); bool performOrchardWalletUpdates{false}; if (orchardWallet.IsCheckpointed()) { // We have a checkpoint, so attempt to rewind the Orchard wallet at most as // far as the NU5 activation block. - auto nu5_height = chainParams.GetConsensus().GetActivationHeight(Consensus::UPGRADE_NU5); // If there's no activation height, we shouldn't have a checkpoint already, // and this is a programming error. assert(nu5_height.has_value()); @@ -3962,6 +3962,11 @@ int CWallet::ScanForWalletTransactions( throw std::runtime_error("CWallet::ScanForWalletTransactions(): Orchard wallet is out of sync. Please restart your node with -rescan."); } } + } else if (isInitScan && pindex->nHeight < nu5_height) { + // If it's the initial scan and we're starting below the nu5 activation + // height, we're effectively rescanning from genesis and so it's safe + // to update the note commitment tree as we progress. + performOrchardWalletUpdates = true; } ShowProgress(_("Rescanning..."), 0); // show rescan progress in GUI as dialog or on splashscreen, if -rescan on startup @@ -6015,8 +6020,17 @@ bool CWallet::InitLoadWallet(const CChainParams& params, bool clearWitnessCaches return UIError(_("Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node)")); } + // If a rescan would begin at a point before NU5 activation height, reset + // the Orchard wallet state to empty. + if (pindexRescan->nHeight <= Params().GetConsensus().GetActivationHeight(Consensus::UPGRADE_NU5)) { + walletInstance->orchardWallet.Reset(); + } + uiInterface.InitMessage(_("Rescanning...")); - LogPrintf("Rescanning last %i blocks (from block %i)...\n", chainActive.Height() - pindexRescan->nHeight, pindexRescan->nHeight); + LogPrintf( + "Rescanning last %i blocks (from block %i)...\n", + chainActive.Height() - pindexRescan->nHeight, + pindexRescan->nHeight); nStart = GetTimeMillis(); walletInstance->ScanForWalletTransactions(pindexRescan, true, true); LogPrintf(" rescan %15dms\n", GetTimeMillis() - nStart); From 23a73146078f668512c765ce095b53b77207d730 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 3 Mar 2022 18:30:13 -0700 Subject: [PATCH 07/16] Check wallet latest anchor against hashFinalOrchardRoot in ChainTip. This will cause test failures until note commitment tree persistence is implemented. --- src/rust/include/rust/orchard/wallet.h | 7 +++++++ src/rust/src/wallet.rs | 28 ++++++++++++++++++++++---- src/wallet/orchard.h | 6 ++++++ src/wallet/wallet.cpp | 4 ++++ 4 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/rust/include/rust/orchard/wallet.h b/src/rust/include/rust/orchard/wallet.h index f97191c9b..f4d8dc2aa 100644 --- a/src/rust/include/rust/orchard/wallet.h +++ b/src/rust/include/rust/orchard/wallet.h @@ -95,6 +95,13 @@ bool orchard_wallet_append_bundle_commitments( const OrchardBundlePtr* bundle ); +/** + * Returns the root of the wallet's note commitment tree. + */ +void orchard_wallet_commitment_tree_root( + const OrchardWalletPtr* wallet, + unsigned char* root_ret); + /** * Returns whether the specified transaction contains any Orchard notes that belong * to this wallet. diff --git a/src/rust/src/wallet.rs b/src/rust/src/wallet.rs index 2663165fc..624259543 100644 --- a/src/rust/src/wallet.rs +++ b/src/rust/src/wallet.rs @@ -472,6 +472,10 @@ impl Wallet { }) .collect() } + + pub fn note_commitment_tree_root(&self) -> MerkleHashOrchard { + self.witness_tree.root() + } } #[no_mangle] @@ -507,18 +511,23 @@ pub extern "C" fn orchard_wallet_is_checkpointed(wallet: *const Wallet) -> bool #[no_mangle] pub extern "C" fn orchard_wallet_rewind( - wallet: *mut Wallet, + wallet: *mut Wallet, to_height: BlockHeight, - blocks_rewound: *mut u32) -> bool { + blocks_rewound: *mut u32, +) -> 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 blocks_rewound = + unsafe { blocks_rewound.as_mut() }.expect("Return value pointer may not be null."); match wallet.rewind(to_height) { Ok(rewound) => { *blocks_rewound = rewound; true } Err(e) => { - error!("Unable to rewind the wallet to height {:?}: {:?}", to_height, e); + error!( + "Unable to rewind the wallet to height {:?}: {:?}", + to_height, e + ); false } } @@ -559,6 +568,17 @@ pub extern "C" fn orchard_wallet_append_bundle_commitments( true } +#[no_mangle] +pub extern "C" fn orchard_wallet_commitment_tree_root( + wallet: *const Wallet, + root_ret: *mut [u8; 32], +) { + let wallet = unsafe { wallet.as_ref() }.expect("Wallet pointer may not be null"); + let root_ret = unsafe { root_ret.as_mut() }.expect("Cannot return to the null pointer."); + + *root_ret = wallet.note_commitment_tree_root().to_bytes(); +} + #[no_mangle] pub extern "C" fn orchard_wallet_add_spending_key(wallet: *mut Wallet, sk: *const SpendingKey) { let wallet = unsafe { wallet.as_mut() }.expect("Wallet pointer may not be null"); diff --git a/src/wallet/orchard.h b/src/wallet/orchard.h index 574fcc1a3..5649255c5 100644 --- a/src/wallet/orchard.h +++ b/src/wallet/orchard.h @@ -135,6 +135,12 @@ public: return true; } + uint256 GetLatestAnchor() const { + uint256 value; + orchard_wallet_commitment_tree_root(inner.get(), value.begin()); + return value; + } + bool TxContainsMyNotes(const uint256& txid) { return orchard_wallet_tx_contains_my_notes( inner.get(), diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9d15697e1..371e83b0a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2320,6 +2320,7 @@ 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()); } // Update witness heights @@ -2391,6 +2392,9 @@ void CWallet::DecrementNoteWitnesses(const Consensus::Params& consensus, const C uint32_t blocksRewound{0}; assert(orchardWallet.Rewind(pindex->nHeight - 1, blocksRewound)); assert(blocksRewound == 1); + if (consensus.NetworkUpgradeActive(pindex->nHeight - 1, Consensus::UPGRADE_NU5)) { + assert(pindex->pprev->hashFinalOrchardRoot == orchardWallet.GetLatestAnchor()); + } } // For performance reasons, we write out the witness cache in From 9336c5e7f0e14938058278388bedbc95d4bef8d4 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 4 Mar 2022 09:59:11 -0700 Subject: [PATCH 08/16] Remove assertions that require Orchard wallet persistence to satisfy. --- src/wallet/wallet.cpp | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 371e83b0a..7bb633a13 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2319,8 +2319,10 @@ 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()); + orchardWallet.AppendNoteCommitments(pindex->nHeight, *pblock); + // TODO ORCHARD: Enable the following assertions. + //assert(orchardWallet.AppendNoteCommitments(pindex->nHeight, *pblock)); + //assert(pindex->hashFinalOrchardRoot == orchardWallet.GetLatestAnchor()); } // Update witness heights @@ -2390,11 +2392,13 @@ void CWallet::DecrementNoteWitnesses(const Consensus::Params& consensus, const C // pindex->nHeight is the height of the block being removed, so we rewind // to the previous block height uint32_t blocksRewound{0}; - assert(orchardWallet.Rewind(pindex->nHeight - 1, blocksRewound)); - assert(blocksRewound == 1); - if (consensus.NetworkUpgradeActive(pindex->nHeight - 1, Consensus::UPGRADE_NU5)) { - assert(pindex->pprev->hashFinalOrchardRoot == orchardWallet.GetLatestAnchor()); - } + orchardWallet.Rewind(pindex->nHeight - 1, blocksRewound); + // TODO ORCHARD: Enable the following assertions. + //assert(orchardWallet.Rewind(pindex->nHeight - 1, blocksRewound)); + //assert(blocksRewound == 1); + //if (consensus.NetworkUpgradeActive(pindex->nHeight - 1, Consensus::UPGRADE_NU5)) { + // assert(pindex->pprev->hashFinalOrchardRoot == orchardWallet.GetLatestAnchor()); + //} } // For performance reasons, we write out the witness cache in From 6171f27b02347e4ebfa6b49bf5156160752681e3 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 28 Feb 2022 11:12:37 -0700 Subject: [PATCH 09/16] Add a test for Orchard note detection. We can't actually test rollback via RPC tests until wallet persistence is implemented. This implements a rollback scenario that will should pass after wallet persistence is implemented. --- qa/pull-tester/rpc-tests.py | 1 + qa/rpc-tests/wallet_orchard.py | 145 +++++++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+) create mode 100755 qa/rpc-tests/wallet_orchard.py diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py index 7c8ed42f0..66c5c75cd 100755 --- a/qa/pull-tester/rpc-tests.py +++ b/qa/pull-tester/rpc-tests.py @@ -73,6 +73,7 @@ BASE_SCRIPTS= [ 'wallet_changeindicator.py', 'wallet_import_export.py', 'wallet_isfromme.py', + 'wallet_orchard.py', 'wallet_nullifiers.py', 'wallet_sapling.py', 'wallet_sendmany_any_taddr.py', diff --git a/qa/rpc-tests/wallet_orchard.py b/qa/rpc-tests/wallet_orchard.py new file mode 100755 index 000000000..28f58c19a --- /dev/null +++ b/qa/rpc-tests/wallet_orchard.py @@ -0,0 +1,145 @@ +#!/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 . + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + NU5_BRANCH_ID, + assert_equal, + get_coinbase_address, + nuparams, + start_nodes, + wait_and_assert_operationid_status +) + +from decimal import Decimal + +# Test wallet behaviour with the Orchard protocol +class WalletOrchardTest(BitcoinTestFramework): + def __init__(self): + super().__init__() + self.num_nodes = 4 + + def setup_nodes(self): + return start_nodes(self.num_nodes, self.options.tmpdir, [[ + '-experimentalfeatures', + '-orchardwallet', + nuparams(NU5_BRANCH_ID, 210), + ]] * self.num_nodes) + + def run_test(self): + # Sanity-check the test harness + assert_equal(self.nodes[0].getblockcount(), 200) + + # Get a new orchard-only unified address + acct1 = self.nodes[1].z_getnewaccount()['account'] + addrRes1 = self.nodes[1].z_getaddressforaccount(acct1, ['orchard']) + assert_equal(acct1, addrRes1['account']) + assert_equal(addrRes1['pools'], ['orchard']) + ua1 = addrRes1['unifiedaddress'] + + # Verify that we have only an Orchard component + receiver_types = self.nodes[0].z_listunifiedreceivers(ua1) + assert_equal(set(['orchard']), set(receiver_types)) + + # Verify balance + assert_equal({'pools': {}, 'minimum_confirmations': 1}, self.nodes[1].z_getbalanceforaccount(acct1)) + + # Send some sapling funds to node 2 for later spending after we split the network + acct2 = self.nodes[2].z_getnewaccount()['account'] + addrRes2 = self.nodes[2].z_getaddressforaccount(acct2, ['sapling', 'orchard']) + assert_equal(acct2, addrRes2['account']) + ua2 = addrRes2['unifiedaddress'] + saplingAddr2 = self.nodes[2].z_listunifiedreceivers(ua2)['sapling'] + + recipients = [{"address": saplingAddr2, "amount": Decimal('10')}] + myopid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[0], myopid) + + # Mine the tx & activate Orchard + self.sync_all() + self.nodes[0].generate(10) + self.sync_all() + + # Check the value on saplingAddr2 + assert_equal( + {'pools': {'sapling': {'valueZat': Decimal('1000000000')}}, 'minimum_confirmations': 1}, + self.nodes[2].z_getbalanceforaccount(acct2)) + + # Node 0 shields some funds + # t-coinbase -> Orchard + recipients = [{"address": ua1, "amount": Decimal('10')}] + myopid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), 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': Decimal('1000000000')}}, 'minimum_confirmations': 1}, + self.nodes[1].z_getbalanceforaccount(acct1)) + + # Split the network + # TODO: Enable after wallet persistence. + # self.split_network() + + # Send another tx to ua1 + recipients = [{"address": ua1, "amount": Decimal('10')}] + myopid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[0], myopid) + + # Mine the tx & generate a majority chain on the 0/1 side of the split + self.sync_all() + self.nodes[0].generate(10) + self.sync_all() + + assert_equal( + {'pools': {'orchard': {'valueZat': Decimal('2000000000')}}, 'minimum_confirmations': 1}, + self.nodes[1].z_getbalanceforaccount(acct1)) + + # On the other side of the split, send some funds to node 3 + acct3 = self.nodes[3].z_getnewaccount()['account'] + addrRes3 = self.nodes[3].z_getaddressforaccount(acct3, ['sapling', 'orchard']) + assert_equal(acct3, addrRes3['account']) + ua3 = addrRes3['unifiedaddress'] + + recipients = [{"address": ua3, "amount": Decimal('1')}] + myopid = self.nodes[2].z_sendmany(ua2, recipients, 1, 0, True) + wait_and_assert_operationid_status(self.nodes[2], myopid) + + self.sync_all() + self.nodes[2].generate(1) + self.sync_all() + + assert_equal( + {'pools': {'sapling': {'valueZat': Decimal('900000000')}}, 'minimum_confirmations': 1}, + self.nodes[2].z_getbalanceforaccount(acct2)) + + assert_equal( + {'pools': {'orchard': {'valueZat': Decimal('100000000')}}, 'minimum_confirmations': 1}, + self.nodes[3].z_getbalanceforaccount(acct3)) + + # TODO: enable after wallet persistence + # # Rejoin the network. + # self.join_network() + + # split 0/1's chain should have won, so their wallet balance should be consistent + assert_equal( + {'pools': {'orchard': {'valueZat': Decimal('2000000000')}}, 'minimum_confirmations': 1}, + self.nodes[1].z_getbalanceforaccount(acct1)) + + # TODO: enable after wallet persistence + # # split 2/3's chain should have been rolled back, so their txn should have been + # # un-mined + # assert_equal( + # {'pools': {'sapling': {'valueZat': Decimal('1000000000')}}, 'minimum_confirmations': 1}, + # self.nodes[2].z_getbalanceforaccount(acct2)) + + # assert_equal( + # {'pools': {}, 'minimum_confirmations': 1}, + # self.nodes[3].z_getbalanceforaccount(acct3)) + +if __name__ == '__main__': + WalletOrchardTest().main() From faffe2e084d6fe5e7dda058e980c8fa8a3df2c64 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 4 Mar 2022 11:36:03 -0700 Subject: [PATCH 10/16] Assert we never attempt to checkpoint the Orchard wallet at a negative block height. Co-authored-by: Daira Hopwood --- src/wallet/orchard.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wallet/orchard.h b/src/wallet/orchard.h index 5649255c5..8a9f8f6c0 100644 --- a/src/wallet/orchard.h +++ b/src/wallet/orchard.h @@ -82,6 +82,7 @@ public: * height scanned for transactions. This must be called exactly once per block. */ bool CheckpointNoteCommitmentTree(int nBlockHeight) { + assert(nBlockHeight >= 0); return orchard_wallet_checkpoint(inner.get(), (uint32_t) nBlockHeight); } From 97895a6f6d76785068896e6e9c1fedced147700f Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 4 Mar 2022 16:51:47 -0700 Subject: [PATCH 11/16] Apply suggestions from code review Co-authored-by: Daira Hopwood --- src/rust/src/wallet.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/rust/src/wallet.rs b/src/rust/src/wallet.rs index 624259543..8913698d6 100644 --- a/src/rust/src/wallet.rs +++ b/src/rust/src/wallet.rs @@ -125,7 +125,7 @@ pub struct Wallet { /// The in-memory index from nullifier to the outpoint of the note from which that /// nullifier was derived. nullifiers: BTreeMap, - /// The incremental merkle tree used to track note commitments and witnesses for notes + /// The incremental Merkle tree used to track note commitments and witnesses for notes /// belonging to the wallet. witness_tree: BridgeTree, /// The block height at which the last checkpoint was created, if any. @@ -136,9 +136,9 @@ pub struct Wallet { /// Notes marked as locked (currently reserved for pending transactions) locked_notes: BTreeSet, /// Notes marked as spent as a consequence of their nullifiers having been - /// observed in bundle action inputs. The keys of this map are the notes - /// that have been spent, and the values are the outpoints identifying - /// the actions in which they are spent. + /// observed in bundle action inputs. The keys of this map are the outpoints + /// where the spent notes were created, and the values are the outpoints + /// identifying the actions in which they are spent. spent_notes: BTreeMap, /// For each nullifier which appears more than once in transactions that this /// wallet has observed, the set of outpoints corresponding to those nullifiers. @@ -331,9 +331,10 @@ impl Wallet { // that we can detect when the note is later spent. if let Some(fvk) = self.key_store.viewing_keys.get(&ivk) { let nf = note.nullifier(fvk); - // if we already have an entry for this nullifier that does not correspond - // to the current outpoint, then add the existing outpoint to the - // conflicts map. + // If we already have an entry for this nullifier that does not correspond + // to the current outpoint, then add the existing outpoint and the current + // outpoint to the conflicts map. (The existing output will already be + // present if this is not the first detected conflict for this nullifier.) if let Some(op) = self.nullifiers.get(&nf) { if op != &outpoint { let conflicts = self.conflicts.entry(nf).or_insert_with(BTreeSet::new); From c9ef5cd45a6ca4d2e8e74495b1129b48d89290ee Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 7 Mar 2022 16:39:09 -0700 Subject: [PATCH 12/16] Add an `InPoint` type to the Orchard wallet to fix incorrect conflict data. --- src/rust/src/wallet.rs | 55 ++++++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/src/rust/src/wallet.rs b/src/rust/src/wallet.rs index 8913698d6..5eb5ec278 100644 --- a/src/rust/src/wallet.rs +++ b/src/rust/src/wallet.rs @@ -30,12 +30,20 @@ pub struct LastObserved { block_tx_idx: Option, } +/// A pointer to a particular action in an Orchard transaction output. #[derive(Copy, Clone, 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)] +pub struct InPoint { + txid: TxId, + action_idx: usize, +} + #[derive(Debug, Clone)] pub struct DecryptedNote { note: Note, @@ -137,12 +145,12 @@ pub struct Wallet { locked_notes: BTreeSet, /// Notes marked as spent as a consequence of their nullifiers having been /// observed in bundle action inputs. The keys of this map are the outpoints - /// where the spent notes were created, and the values are the outpoints + /// where the spent notes were created, and the values are the inpoints /// identifying the actions in which they are spent. - spent_notes: BTreeMap, + spent_notes: BTreeMap, /// For each nullifier which appears more than once in transactions that this /// wallet has observed, the set of outpoints corresponding to those nullifiers. - conflicts: BTreeMap>, + conflicts: BTreeMap>, } #[derive(Debug, Clone)] @@ -300,17 +308,30 @@ impl Wallet { // Check for spends of our notes by matching against the nullifiers // we're tracking, and when we detect one, associate the current - // txid as spending the note. + // txid and action as spending the note. for (action_idx, action) in bundle.actions().iter().enumerate() { let nf = action.nullifier(); + // find the outpoint where the note being spent in this action was created if let Some(outpoint) = self.nullifiers.get(nf) { - self.spent_notes.insert( - *outpoint, - OutPoint { - txid: *txid, - action_idx, - }, - ); + let inpoint = InPoint { + txid: *txid, + action_idx, + }; + + // If we already have an entry for this nullifier that does not correspond + // to the current inpoint, then add the existing inpoint and the current + // inpoint to the conflicts map. (The existing inpoint will already be + // present if this is not the first detected conflict for this nullifier.) + if let Some(cur_inpoint) = self.spent_notes.get(outpoint) { + if cur_inpoint != &inpoint { + let conflicts = self.conflicts.entry(*nf).or_insert_with(BTreeSet::new); + conflicts.insert(*cur_inpoint); + conflicts.insert(inpoint); + } + } + + // optimistically consider the latest inpoint to be the correct one + self.spent_notes.insert(*outpoint, inpoint); } } @@ -331,18 +352,6 @@ impl Wallet { // that we can detect when the note is later spent. if let Some(fvk) = self.key_store.viewing_keys.get(&ivk) { let nf = note.nullifier(fvk); - // If we already have an entry for this nullifier that does not correspond - // to the current outpoint, then add the existing outpoint and the current - // outpoint to the conflicts map. (The existing output will already be - // present if this is not the first detected conflict for this nullifier.) - if let Some(op) = self.nullifiers.get(&nf) { - if op != &outpoint { - let conflicts = self.conflicts.entry(nf).or_insert_with(BTreeSet::new); - conflicts.insert(*op); - conflicts.insert(outpoint); - }; - } - self.nullifiers.insert(nf, outpoint); } From c3cf1419367643945a1eb2db4152c17047222dbe Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 7 Mar 2022 19:08:41 -0700 Subject: [PATCH 13/16] Apply suggestions from code review Co-authored-by: str4d --- qa/rpc-tests/wallet_orchard.py | 4 ++-- src/rust/include/rust/orchard/wallet.h | 9 ++++++--- src/wallet/orchard.h | 6 +++--- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/qa/rpc-tests/wallet_orchard.py b/qa/rpc-tests/wallet_orchard.py index 28f58c19a..cf2d5e665 100755 --- a/qa/rpc-tests/wallet_orchard.py +++ b/qa/rpc-tests/wallet_orchard.py @@ -57,12 +57,12 @@ class WalletOrchardTest(BitcoinTestFramework): myopid = self.nodes[0].z_sendmany(get_coinbase_address(self.nodes[0]), recipients, 1, 0) wait_and_assert_operationid_status(self.nodes[0], myopid) - # Mine the tx & activate Orchard + # Mine the tx & activate NU5 self.sync_all() self.nodes[0].generate(10) self.sync_all() - # Check the value on saplingAddr2 + # Check the value sent to saplingAddr2 was received in node 2's account assert_equal( {'pools': {'sapling': {'valueZat': Decimal('1000000000')}}, 'minimum_confirmations': 1}, self.nodes[2].z_getbalanceforaccount(acct2)) diff --git a/src/rust/include/rust/orchard/wallet.h b/src/rust/include/rust/orchard/wallet.h index f4d8dc2aa..af253af68 100644 --- a/src/rust/include/rust/orchard/wallet.h +++ b/src/rust/include/rust/orchard/wallet.h @@ -40,8 +40,9 @@ void orchard_wallet_free(OrchardWalletPtr* wallet); bool orchard_wallet_reset(OrchardWalletPtr* wallet); /** - * Adds a checkpoint to the wallet's note commitment tree to enable - * a future rewind. + * Checkpoint the note commitment tree. This returns `false` and leaves the note + * commitment tree unmodified if the block height specified is not the successor + * to the last block height checkpointed. */ bool orchard_wallet_checkpoint( OrchardWalletPtr* wallet, @@ -55,7 +56,9 @@ bool orchard_wallet_checkpoint( bool orchard_wallet_is_checkpointed(const OrchardWalletPtr* wallet); /** - * Rewinds to the most recently added checkpoint. + * Rewinds to the most recent checkpoint, and marks as unspent any notes + * previously identified as having been spent by transactions in the + * latest block. */ bool orchard_wallet_rewind( OrchardWalletPtr* wallet, diff --git a/src/wallet/orchard.h b/src/wallet/orchard.h index 8a9f8f6c0..e89ed7a2e 100644 --- a/src/wallet/orchard.h +++ b/src/wallet/orchard.h @@ -78,8 +78,8 @@ public: /** * Checkpoint the note commitment tree. This returns `false` and leaves the note - * commitment tree unmodified if the block height does not match the last block - * height scanned for transactions. This must be called exactly once per block. + * commitment tree unmodified if the block height specified is not the successor + * to the last block height checkpointed. */ bool CheckpointNoteCommitmentTree(int nBlockHeight) { assert(nBlockHeight >= 0); @@ -94,7 +94,7 @@ public: } /** - * Rewind to the most recent checkpoint, and mark as unspent any notes + * Rewinds to the most recent checkpoint, and marks as unspent any notes * previously identified as having been spent by transactions in the * latest block. */ From 51cb37e541201b8186e110d0104f27dbb2951ff3 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 7 Mar 2022 22:02:19 -0700 Subject: [PATCH 14/16] Fix missing update to `last_checkpoint` on rewind. --- src/rust/src/wallet.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/rust/src/wallet.rs b/src/rust/src/wallet.rs index 5eb5ec278..4ba7b5745 100644 --- a/src/rust/src/wallet.rs +++ b/src/rust/src/wallet.rs @@ -286,6 +286,12 @@ impl Wallet { } } self.last_observed = Some(last_observed); + self.last_checkpoint = if checkpoint_count > blocks_to_rewind as usize { + Some(checkpoint_height - blocks_to_rewind) + } else { + // checkpoint_count == blocks_to_rewind + None + }; Ok(blocks_to_rewind) } else { From 9284c5552b6fb19f9bc1b5fc4a7a0e6a33009744 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 8 Mar 2022 11:47:13 -0700 Subject: [PATCH 15/16] Track mined-ness instead of spent-ness of notes in the Orchard wallet. Tracking spentness would require the Orchard wallet to be aware of the local node's mempool contents; instead, the Orchard wallet now only tracks whether a note is known to be mined and the C++ wallet checks selected notes to determine whether any transactions that consume those notes exist in the mempool. In addition: * Locking of notes will be delegated to the C++ side. * The type of the recipient in wallet::NoteMetadata was changed from `*const Address` to `*mut Address` to reflect the fact that it is owned memory that must be freed. * Nullifier data is no longer cleared during rewinds. --- src/rust/include/rust/orchard/wallet.h | 15 ++- src/rust/src/wallet.rs | 126 +++++++++++++++---------- src/wallet/orchard.h | 24 ++++- src/wallet/wallet.cpp | 24 ++++- src/wallet/wallet.h | 1 + 5 files changed, 132 insertions(+), 58 deletions(-) diff --git a/src/rust/include/rust/orchard/wallet.h b/src/rust/include/rust/orchard/wallet.h index af253af68..bb9208f71 100644 --- a/src/rust/include/rust/orchard/wallet.h +++ b/src/rust/include/rust/orchard/wallet.h @@ -181,13 +181,24 @@ typedef void (*push_callback_t)(void* resultVector, const RawOrchardNoteMetadata void orchard_wallet_get_filtered_notes( const OrchardWalletPtr* wallet, const OrchardIncomingViewingKeyPtr* ivk, - bool ignoreSpent, - bool ignoreLocked, + bool ignoreMined, bool requireSpendingKey, void* resultVector, push_callback_t push_cb ); + +typedef void (*push_txid_callback_t)(void* resultVector, unsigned char[32]); + +void orchard_wallet_get_potential_spends( + const OrchardWalletPtr* wallet, + const unsigned char txid[32], + const uint32_t action_idx, + void* resultVector, + push_txid_callback_t push_cb + ); + + #ifdef __cplusplus } #endif diff --git a/src/rust/src/wallet.rs b/src/rust/src/wallet.rs index 4ba7b5745..432e321d6 100644 --- a/src/rust/src/wallet.rs +++ b/src/rust/src/wallet.rs @@ -1,6 +1,7 @@ use incrementalmerkletree::{bridgetree::BridgeTree, Frontier, Tree}; use libc::c_uchar; use std::collections::{BTreeMap, BTreeSet}; +use std::convert::TryInto; use tracing::error; use zcash_primitives::{ @@ -69,7 +70,7 @@ pub struct TxNotes { pub struct NoteMetadata { txid: [u8; 32], action_idx: u32, - recipient: *const Address, + recipient: *mut Address, note_value: i64, memo: [u8; 512], } @@ -122,6 +123,12 @@ impl KeyStore { pub fn ivk_for_address(&self, addr: &Address) -> Option<&IncomingViewingKey> { self.payment_addresses.get(&OrderedAddress::new(*addr)) } + + pub fn get_nullifier(&self, note: &Note) -> Option { + self.ivk_for_address(¬e.recipient()) + .and_then(|ivk| self.viewing_keys.get(ivk)) + .map(|fvk| note.nullifier(fvk)) + } } pub struct Wallet { @@ -141,16 +148,15 @@ pub struct Wallet { /// The block height and transaction index of the note most recently added to /// `witness_tree` last_observed: Option, - /// Notes marked as locked (currently reserved for pending transactions) - locked_notes: BTreeSet, - /// Notes marked as spent as a consequence of their nullifiers having been - /// observed in bundle action inputs. The keys of this map are the outpoints - /// where the spent notes were created, and the values are the inpoints - /// identifying the actions in which they are spent. - spent_notes: BTreeMap, + /// Notes marked as mined as a consequence of their nullifiers having been observed + /// in bundle action inputs in bundles appended to the commitment tree. The keys of + /// this map are the outpoints where the spent notes were created, and the values + /// are the inpoints identifying the actions in which they are spent. + mined_notes: BTreeMap, /// For each nullifier which appears more than once in transactions that this - /// wallet has observed, the set of outpoints corresponding to those nullifiers. - conflicts: BTreeMap>, + /// wallet has observed, the set of inpoints where those nullifiers were + /// observed as as having been spent. + potential_spends: BTreeMap>, } #[derive(Debug, Clone)] @@ -177,9 +183,8 @@ impl Wallet { witness_tree: BridgeTree::new(MAX_CHECKPOINTS), last_checkpoint: None, last_observed: None, - locked_notes: BTreeSet::new(), - spent_notes: BTreeMap::new(), - conflicts: BTreeMap::new(), + mined_notes: BTreeMap::new(), + potential_spends: BTreeMap::new(), } } @@ -195,8 +200,7 @@ impl Wallet { self.witness_tree = BridgeTree::new(MAX_CHECKPOINTS); self.last_checkpoint = None; self.last_observed = None; - self.locked_notes = BTreeSet::new(); - self.spent_notes = BTreeMap::new(); + self.mined_notes = BTreeMap::new(); } /// Checkpoints the note commitment tree. This returns `false` and leaves the note @@ -276,8 +280,11 @@ impl Wallet { }) .collect(); - self.spent_notes.retain(|_, v| to_retain.contains(&v.txid)); - self.nullifiers.retain(|_, v| to_retain.contains(&v.txid)); + 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 set to `None` as the transaction will no longer have been + // observed as having been mined. for (txid, n) in self.wallet_received_notes.iter_mut() { // Erase block height information for any received notes // that have been un-mined by the rewind. @@ -317,27 +324,16 @@ impl Wallet { // txid and action as spending the note. for (action_idx, action) in bundle.actions().iter().enumerate() { let nf = action.nullifier(); - // find the outpoint where the note being spent in this action was created - if let Some(outpoint) = self.nullifiers.get(nf) { - let inpoint = InPoint { - txid: *txid, - action_idx, - }; - - // If we already have an entry for this nullifier that does not correspond - // to the current inpoint, then add the existing inpoint and the current - // inpoint to the conflicts map. (The existing inpoint will already be - // present if this is not the first detected conflict for this nullifier.) - if let Some(cur_inpoint) = self.spent_notes.get(outpoint) { - if cur_inpoint != &inpoint { - let conflicts = self.conflicts.entry(*nf).or_insert_with(BTreeSet::new); - conflicts.insert(*cur_inpoint); - conflicts.insert(inpoint); - } - } - - // optimistically consider the latest inpoint to be the correct one - self.spent_notes.insert(*outpoint, inpoint); + // If a nullifier corresponds to one of our notes, add its inpoint as a + // potential spend (the transaction may not end up being mined). + if self.nullifiers.contains_key(nf) { + self.potential_spends + .entry(*nf) + .or_insert_with(BTreeSet::new) + .insert(InPoint { + txid: *txid, + action_idx, + }); } } @@ -438,6 +434,18 @@ impl Wallet { { self.witness_tree.witness(); } + + // For nullifiers that are ours that we detect as spent by this action, + // we will record that input as being mined. + if let Some(outpoint) = self.nullifiers.get(action.nullifier()) { + self.mined_notes.insert( + *outpoint, + InPoint { + txid: *txid, + action_idx, + }, + ); + } } Ok(()) @@ -453,8 +461,7 @@ impl Wallet { pub fn get_filtered_notes( &self, ivk: Option<&IncomingViewingKey>, - ignore_spent: bool, - ignore_locked: bool, + ignore_mined: bool, require_spending_key: bool, ) -> Vec<(OutPoint, DecryptedNote)> { self.wallet_received_notes @@ -474,8 +481,7 @@ impl Wallet { // if `ivk` is `None`, return all notes that match the other conditions .filter(|dnote_ivk| ivk.map_or(true, |ivk| &ivk == dnote_ivk)) .and_then(|dnote_ivk| { - if (ignore_spent && self.spent_notes.contains_key(&outpoint)) - || (ignore_locked && self.locked_notes.contains(&outpoint)) + if (ignore_mined && self.mined_notes.contains_key(&outpoint)) || (require_spending_key && self.key_store.spending_key_for_ivk(dnote_ivk).is_none()) { @@ -660,8 +666,7 @@ pub type PushCb = unsafe extern "C" fn(obj: Option, meta: NoteMetadata); pub extern "C" fn orchard_wallet_get_filtered_notes( wallet: *const Wallet, ivk: *const IncomingViewingKey, - ignore_spent: bool, - ignore_locked: bool, + ignore_mined: bool, require_spending_key: bool, result: Option, push_cb: Option, @@ -669,17 +674,40 @@ pub extern "C" fn orchard_wallet_get_filtered_notes( let wallet = unsafe { wallet.as_ref() }.expect("Wallet pointer may not be null."); let ivk = unsafe { ivk.as_ref() }; - for (outpoint, dnote) in - wallet.get_filtered_notes(ivk, ignore_spent, ignore_locked, require_spending_key) - { - let recipient = Box::new(dnote.note.recipient()); + for (outpoint, dnote) in wallet.get_filtered_notes(ivk, ignore_mined, require_spending_key) { let metadata = NoteMetadata { txid: *outpoint.txid.as_ref(), action_idx: outpoint.action_idx as u32, - recipient: Box::into_raw(recipient), + recipient: Box::into_raw(Box::new(dnote.note.recipient())), note_value: dnote.note.value().inner() as i64, memo: dnote.memo, }; unsafe { (push_cb.unwrap())(result, metadata) }; } } + +pub type PushTxId = unsafe extern "C" fn(obj: Option, txid: *const [u8; 32]); + +#[no_mangle] +pub extern "C" fn orchard_wallet_get_potential_spends( + wallet: *const Wallet, + txid: *const [c_uchar; 32], + action_idx: u32, + result: Option, + push_cb: Option, +) { + let wallet = unsafe { wallet.as_ref() }.expect("Wallet pointer may not be null."); + let txid = TxId::from_bytes(*unsafe { txid.as_ref() }.expect("txid may not be null.")); + + if let Some(inpoints) = wallet + .wallet_received_notes + .get(&txid) + .and_then(|txnotes| txnotes.decrypted_notes.get(&action_idx.try_into().unwrap())) + .and_then(|dnote| wallet.key_store.get_nullifier(&dnote.note)) + .and_then(|nf| wallet.potential_spends.get(&nf)) + { + for inpoint in inpoints { + unsafe { (push_cb.unwrap())(result, inpoint.txid.as_ref()) }; + } + } +} diff --git a/src/wallet/orchard.h b/src/wallet/orchard.h index e89ed7a2e..0a1375d8e 100644 --- a/src/wallet/orchard.h +++ b/src/wallet/orchard.h @@ -193,20 +193,36 @@ public: void GetFilteredNotes( std::vector& orchardNotesRet, const std::optional& ivk, - bool ignoreSpent, - bool ignoreLocked, + bool ignoreMined, bool requireSpendingKey) const { orchard_wallet_get_filtered_notes( inner.get(), ivk.has_value() ? ivk.value().inner.get() : nullptr, - ignoreSpent, - ignoreLocked, + ignoreMined, requireSpendingKey, &orchardNotesRet, PushOrchardNoteMeta ); } + + static void PushTxId(void* txidsRet, unsigned char txid[32]) { + uint256 txid_out; + std::copy(txid, txid + 32, txid_out.begin()); + reinterpret_cast*>(txidsRet)->push_back(txid_out); + } + + std::vector GetPotentialSpends(const OrchardOutPoint& outPoint) const { + std::vector result; + orchard_wallet_get_potential_spends( + inner.get(), + outPoint.hash.begin(), + outPoint.n, + &result, + PushTxId + ); + return result; + } }; #endif // ZCASH_ORCHARD_WALLET_H diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7bb633a13..f8367be65 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2032,11 +2032,17 @@ SpendableInputs CWallet::FindSpendableInputs( for (const auto& ivk : orchardIvks) { std::vector incomingNotes; - orchardWallet.GetFilteredNotes(incomingNotes, ivk, true, true, true); + orchardWallet.GetFilteredNotes(incomingNotes, ivk, true, true); for (const auto& noteMeta : incomingNotes) { + if (IsOrchardSpent(noteMeta.GetOutPoint())) { + continue; + } + auto mit = mapWallet.find(noteMeta.GetOutPoint().hash); if (mit != mapWallet.end() && mit->second.GetDepthInMainChain() >= minDepth) { + // Check whether any of the potential spends exist in the main chain or + // the mempool. unspent.orchardNoteMetadata.push_back(noteMeta); } } @@ -2099,6 +2105,16 @@ bool CWallet::IsSaplingSpent(const uint256& nullifier) const { return false; } +bool CWallet::IsOrchardSpent(const OrchardOutPoint& outpoint) const { + for (const auto& txid : orchardWallet.GetPotentialSpends(outpoint)) { + std::map::const_iterator mit = mapWallet.find(txid); + if (mit != mapWallet.end() && mit->second.GetDepthInMainChain() >= 0) { + return true; // Spent + } + } + return false; +} + void CWallet::AddToTransparentSpends(const COutPoint& outpoint, const uint256& wtxid) { mapTxSpends.insert(make_pair(outpoint, wtxid)); @@ -6430,7 +6446,6 @@ void CWallet::GetFilteredNotes( orchardNotes, ivk.value(), ignoreSpent, - ignoreLocked, requireSpendingKey); } } @@ -6440,11 +6455,14 @@ void CWallet::GetFilteredNotes( orchardNotes, std::nullopt, ignoreSpent, - ignoreLocked, requireSpendingKey); } for (auto& noteMeta : orchardNotes) { + if (ignoreSpent && IsOrchardSpent(noteMeta.GetOutPoint())) { + continue; + } + auto wtx = GetWalletTx(noteMeta.GetOutPoint().hash); if (wtx) { if (wtx->GetDepthInMainChain() >= minDepth) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index bfc8a63e5..5ff294cb5 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1357,6 +1357,7 @@ public: bool IsSpent(const uint256& hash, unsigned int n) const; bool IsSproutSpent(const uint256& nullifier) const; bool IsSaplingSpent(const uint256& nullifier) const; + bool IsOrchardSpent(const OrchardOutPoint& outpoint) const; bool IsLockedCoin(uint256 hash, unsigned int n) const; void LockCoin(COutPoint& output); From 19ce19155cbd7c0a9ffa233b3986dffefdb3d723 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 8 Mar 2022 15:35:15 -0700 Subject: [PATCH 16/16] Apply suggestions from code review Co-authored-by: Daira Hopwood Co-authored-by: str4d --- src/rust/include/rust/orchard/wallet.h | 24 ++++++++++++++++++++---- src/rust/src/wallet.rs | 8 ++------ src/wallet/orchard.h | 1 + src/wallet/wallet.cpp | 2 -- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/rust/include/rust/orchard/wallet.h b/src/rust/include/rust/orchard/wallet.h index bb9208f71..8c0d7684a 100644 --- a/src/rust/include/rust/orchard/wallet.h +++ b/src/rust/include/rust/orchard/wallet.h @@ -59,6 +59,15 @@ bool orchard_wallet_is_checkpointed(const OrchardWalletPtr* wallet); * Rewinds to the most recent checkpoint, and marks as unspent any notes * previously identified as having been spent by transactions in the * latest block. + * + * 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`. + * + * 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. */ bool orchard_wallet_rewind( OrchardWalletPtr* wallet, @@ -71,8 +80,7 @@ bool orchard_wallet_rewind( * wallet's incoming viewing keys, and adds those notes to the wallet. * * The provided bundle must be a component of the transaction from which - * `txid` was derived, and the specified block height must be the height - * at which this transaction was mined. + * `txid` was derived. */ void orchard_wallet_add_notes_from_bundle( OrchardWalletPtr* wallet, @@ -173,10 +181,12 @@ typedef void (*push_callback_t)(void* resultVector, const RawOrchardNoteMetadata * uses the provided callback to push RawOrchardNoteMetadata values * corresponding to those notes on to the provided result vector. Note that * the push_cb callback can perform any necessary conversion from a - * RawOrchardNoteMetadata value prior in addition to modifying the provided + * RawOrchardNoteMetadata value in addition to modifying the provided * result vector. * * If `ivk` is null, all notes belonging to the wallet will be returned. + * The `RawOrchardNoteMetadata::addr` pointers for values provided to the + * callback must be manually freed by the caller. */ void orchard_wallet_get_filtered_notes( const OrchardWalletPtr* wallet, @@ -188,8 +198,14 @@ void orchard_wallet_get_filtered_notes( ); -typedef void (*push_txid_callback_t)(void* resultVector, unsigned char[32]); +typedef void (*push_txid_callback_t)(void* resultVector, unsigned char txid[32]); +/** + * Returns a vector of transaction IDs for transactions that have been observed + * as spending the given outpoint (transaction ID and action index) by using + * the `push_cb` callback to push transaction IDs onto the provided result + * vector. + */ void orchard_wallet_get_potential_spends( const OrchardWalletPtr* wallet, const unsigned char txid[32], diff --git a/src/rust/src/wallet.rs b/src/rust/src/wallet.rs index 432e321d6..e420453f8 100644 --- a/src/rust/src/wallet.rs +++ b/src/rust/src/wallet.rs @@ -213,8 +213,7 @@ impl Wallet { // block must be checkpointed if self .last_checkpoint - .iter() - .any(|last_height| block_height != *last_height + 1) + .map_or(false, |last_height| block_height != last_height + 1) { return false; } @@ -428,10 +427,7 @@ impl Wallet { } // for notes that are ours, witness the current state of the tree - if my_notes_for_tx - .iter() - .any(|n| n.decrypted_notes.contains_key(&action_idx)) - { + if my_notes_for_tx.map_or(false, |n| n.decrypted_notes.contains_key(&action_idx)) { self.witness_tree.witness(); } diff --git a/src/wallet/orchard.h b/src/wallet/orchard.h index 0a1375d8e..658c2656d 100644 --- a/src/wallet/orchard.h +++ b/src/wallet/orchard.h @@ -99,6 +99,7 @@ public: * latest block. */ bool Rewind(int nBlockHeight, uint32_t& blocksRewoundRet) { + assert(nBlockHeight >= 0); return orchard_wallet_rewind(inner.get(), (uint32_t) nBlockHeight, &blocksRewoundRet); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index f8367be65..744f6b74c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2041,8 +2041,6 @@ SpendableInputs CWallet::FindSpendableInputs( auto mit = mapWallet.find(noteMeta.GetOutPoint().hash); if (mit != mapWallet.end() && mit->second.GetDepthInMainChain() >= minDepth) { - // Check whether any of the potential spends exist in the main chain or - // the mempool. unspent.orchardNoteMetadata.push_back(noteMeta); } }