diff --git a/src/gtest/test_wallet_notecache.cpp b/src/gtest/test_wallet_notecache.cpp index 7ae9e18a8..dd54dea47 100644 --- a/src/gtest/test_wallet_notecache.cpp +++ b/src/gtest/test_wallet_notecache.cpp @@ -20,6 +20,7 @@ #include "zcash/zip32.h" int32_t hush_dpowconfs(int32_t height, int32_t numconfs); +int tx_height(const uint256 &hash); using namespace libzcash; @@ -191,7 +192,7 @@ void ExpectSameEntries(const std::vector& want, const std::vec // TearDown, because every gtest suite runs in the same process. class WalletNoteCache : public ::testing::Test { protected: - static const int TIP_HEIGHT = 30; + static constexpr int TIP_HEIGHT = 30; CWallet wallet; SaplingPaymentAddress paA, paB, paC; // A and C spendable, B viewing key only @@ -361,10 +362,39 @@ TEST_F(WalletNoteCache, FixtureExercisesEveryFilter) { } } -// The walk returns exactly what the 69bb035e7 loop returned, in the same order, for every filter -// combination. +// The walk returns exactly what the 69bb035e7 loop returned on a -txindex node, in the same order, +// for every filter combination. minDepth 11/12/22 sit on the fixture's depth-11 and depth-21 txs. TEST_F(WalletNoteCache, FilteredNotesMatchLegacy) { - ExpectEquivalent({0, 1}); + ExpectEquivalent({0, 1, 2, 11, 12, 22}); +} + +// minDepth > 1 (sweep and consolidation use 11) locates each tx's block from its depth. The old +// tx_height() re-read the tx through the txindex, and without one (as here, and on a -txindex=0 +// node) it never found a fully shielded tx, so nothing was returned at all. +TEST_F(WalletNoteCache, MinDepthAboveOneUsesWalletHeight) { + // Unspent notes at depth >= 11: tx3's 3 (depth 11) and tx4's 1 (depth 21); tx6 (12) is spent. + EXPECT_EQ(4u, Filtered({}, 11, INT_MAX, true, false, false).size()); + EXPECT_EQ(5u, Filtered({}, 11, INT_MAX, false, false, false).size()); + EXPECT_EQ(1u, Filtered({}, 12, INT_MAX, true, false, false).size()) << "depth 11 is below minDepth 12"; + EXPECT_EQ(3u, Filtered({}, 11, 11, true, false, false).size()) << "depth 21 is above maxDepth 11"; + EXPECT_EQ(0u, Filtered({}, 22, INT_MAX, false, false, false).size()); + for (const SaplingNoteEntry& e : Filtered({}, 2, INT_MAX, false, false, false)) + EXPECT_GE(e.confirmations, 2) << "unmined and shallower txs stay out"; + + std::set none; + std::vector legacy; + LegacyFilteredNotes(wallet, legacy, none, 11, INT_MAX, true, false, false, tx_height); + EXPECT_EQ(0u, legacy.size()) << "the old code found nothing here without a txindex"; +} + +TEST_F(WalletNoteCache, HeightFromDepth) { + LOCK(cs_main); + EXPECT_EQ(TIP_HEIGHT, HeightFromDepth(1)); + EXPECT_EQ(TIP_HEIGHT - 10, HeightFromDepth(11)); + EXPECT_EQ(0, HeightFromDepth(0)) << "mempool"; + EXPECT_EQ(0, HeightFromDepth(-1)) << "not in the chain or mempool"; + for (const auto& d : depthOf) + EXPECT_EQ(minedHeight.at(d.first), HeightFromDepth(wallet.mapWallet.at(d.first).GetDepthInMainChain())); } // Two notes recorded under A's viewing key that it can't actually decrypt (real outputs to someone diff --git a/src/init.cpp b/src/init.cpp index d04e45fc5..2e79b57d2 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -2504,6 +2504,14 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) } } + // Sweep and consolidation pick notes with GetFilteredNotes at minDepth > 1. Older versions + // located each note's block by re-reading its tx through the txindex, so on a -txindex=0 node + // every fully shielded tx was skipped and these never moved those notes. Now they will. + if (!fTxIndex && (pwalletMain->fSweepEnabled || pwalletMain->fSaplingConsolidationEnabled)) { + LogPrintf("%s: -txindex=0 with -zsweep/-consolidation: shielded notes that versions before this " + "fix never saw on such nodes are now included in sweeps and consolidations\n", __func__); + } + //Set Automatic Coinbase Shielding (default ON, conditional: self-guards //on nodes where it cannot act - no owned coinbase, external mineraddress, //or locked wallet). Closes the transparent-coinbase leak for miners. diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 22ff1ec6c..ea129d510 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4309,7 +4309,7 @@ UniValue z_listunspent(const UniValue& params, bool fHelp, const CPubKey& mypk) for (auto & entry : saplingEntries) { UniValue obj(UniValue::VOBJ); - int nHeight = tx_height(entry.op.hash); + int nHeight = HeightFromDepth(entry.confirmations); int dpowconfs = hush_dpowconfs(nHeight, entry.confirmations); // Only return notarized results when minconf>1 @@ -4642,7 +4642,7 @@ UniValue z_listreceivedbyaddress(const UniValue& params, bool fHelp, const CPubK if (boost::get(&zaddr) != nullptr) { for (SaplingNoteEntry & entry : saplingEntries) { UniValue obj(UniValue::VOBJ); - int nHeight = tx_height(entry.op.hash); + int nHeight = HeightFromDepth(entry.confirmations); int dpowconfs = hush_dpowconfs(nHeight, entry.confirmations); // Only return notarized results when minconf>1 if (nMinDepth > 1 && dpowconfs == 1) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 482118474..ff86165de 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -76,7 +76,6 @@ bool fPayAtLeastCustomFee = true; CBlockIndex *hush_chainactive(int32_t height); extern int32_t HUSH_LOADINGBLOCKS; int32_t hush_dpowconfs(int32_t height,int32_t numconfs); -int tx_height( const uint256 &hash ); bool fTxDeleteEnabled = false; bool fTxConflictDeleteEnabled = false; int fDeleteInterval = DEFAULT_TX_DELETE_INTERVAL; @@ -5715,6 +5714,16 @@ bool CMerkleTx::AcceptToMemoryPool(bool fLimitFree, bool fRejectAbsurdFee) return ::AcceptToMemoryPool(mempool, state, *this, fLimitFree, NULL, fRejectAbsurdFee); } +// Replaces tx_height() on the wallet's note paths. tx_height() re-read the tx from disk through +// GetTransaction, once per wallet tx (or per returned note) under LOCK2, and on a -txindex=0 node it +// never found a fully shielded tx, so those notes were silently skipped. The depth already locates +// the block: GetDepthInMainChain is chainActive.Height() - height + 1 for an active-chain tx. +int HeightFromDepth(int nDepth) +{ + AssertLockHeld(cs_main); + return nDepth > 0 ? chainActive.Height() - nDepth + 1 : 0; +} + /** * Find notes in the wallet filtered by payment address, min depth and ability to spend. * These notes are decrypted and added to the output parameter vector, outEntries. @@ -5773,7 +5782,7 @@ void CWallet::GetFilteredNotes( // cs_main is held, so the depth can't change during the walk. const int nDepth = wtx.GetDepthInMainChain(); if (minDepth > 1) { - int nHeight = tx_height(wtx.GetHash()); + int nHeight = HeightFromDepth(nDepth); if ( nHeight == 0 ) { continue; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index bd3421b8f..08327a9b3 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -366,6 +366,11 @@ struct SaplingNoteEntry int confirmations; }; +/** Height of the active-chain block holding a wallet tx that is nDepth deep (as GetDepthInMainChain + * returns it, and SaplingNoteEntry::confirmations holds it), or 0 when the tx is unconfirmed or not + * in the active chain. Caller holds cs_main. */ +int HeightFromDepth(int nDepth); + /** A transaction with a merkle branch linking it to the block chain. */ class CMerkleTx : public CTransaction {