diff --git a/src/Makefile.gtest.include b/src/Makefile.gtest.include index 2f3100f11..613b215be 100644 --- a/src/Makefile.gtest.include +++ b/src/Makefile.gtest.include @@ -14,7 +14,8 @@ hush_gtest_SOURCES = \ gtest/test_randomx_preverify.cpp \ gtest/test_hdtransparent.cpp \ gtest/test_mnemonic_compat.cpp \ - gtest/test_stratum_jobid.cpp + gtest/test_stratum_jobid.cpp \ + gtest/test_wallet_notedata.cpp hush_gtest_CPPFLAGS = $(AM_CPPFLAGS) -DMULTICORE -fopenmp -DBINARY_OUTPUT -DCURVE_ALT_BN128 -DSTATIC $(BITCOIN_INCLUDES) hush_gtest_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) diff --git a/src/gtest/test_wallet_notedata.cpp b/src/gtest/test_wallet_notedata.cpp new file mode 100644 index 000000000..fddee8875 --- /dev/null +++ b/src/gtest/test_wallet_notedata.cpp @@ -0,0 +1,155 @@ +// Copyright (c) 2016-2024 The Hush developers +// Distributed under the GPLv3 software license, see the accompanying +// file COPYING or https://www.gnu.org/licenses/gpl-3.0.en.html + +#include + +#include "primitives/transaction.h" +#include "uint256.h" +#include "wallet/wallet.h" + +namespace { + +class NoteDataWallet : public CWallet { +public: + bool UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx, bool fSameBlock = true) { + return CWallet::UpdatedNoteData(wtxIn, wtx, fSameBlock); + } +}; + +libzcash::SaplingIncomingViewingKey TestIvk() +{ + return libzcash::SaplingIncomingViewingKey(uint256S("0101010101010101010101010101010101010101010101010101010101010101")); +} + +// A note as the wallet holds it once witnessed: nullifier set, witnesses cached at a height. +SaplingNoteData WitnessedNote(const SaplingMerkleTree& tree, int height, const uint256& nullifier) +{ + SaplingNoteData nd(TestIvk(), nullifier); + nd.witnesses.push_front(tree.witness()); + nd.witnessHeight = height; + return nd; +} + +// A note as FindMySaplingNotes returns it during a replay or rescan: no nullifier, no witnesses. +SaplingNoteData FreshNote() +{ + return SaplingNoteData(TestIvk()); +} + +} + +// A replayed or rescanned tx must keep the witnesses AND the nullifier it already had, and must not +// report an update: dropping the nullifier made the next block clear and reseed every carried witness. +TEST(WalletNoteData, ReplayKeepsNullifierAndWitnesses) { + NoteDataWallet wallet; + CWalletTx wtx(&wallet, CTransaction()); + uint256 txid = uint256S("aa"); + SaplingOutPoint op0(txid, 0); + + SaplingMerkleTree tree; + tree.append(uint256S("01")); + const uint256 nf = uint256S("f00d"); + wtx.mapSaplingNoteData[op0] = WitnessedNote(tree, 100, nf); + wtx.mapSaplingNoteData[op0].witnessRootValidated = true; + auto before = wtx.mapSaplingNoteData; + + CWalletTx replay(&wallet, CTransaction()); + replay.mapSaplingNoteData[op0] = FreshNote(); + + EXPECT_FALSE(wallet.UpdatedNoteData(replay, wtx)); + ASSERT_EQ(1u, wtx.mapSaplingNoteData.size()); + const SaplingNoteData& nd = wtx.mapSaplingNoteData.at(op0); + ASSERT_TRUE(static_cast(nd.nullifier)); + EXPECT_EQ(nf, *nd.nullifier); + EXPECT_EQ(100, nd.witnessHeight); + EXPECT_TRUE(nd.witnesses == before.at(op0).witnesses); + // ...but the kept witness is re-checked against the active chain once (in-memory flag only). + EXPECT_FALSE(nd.witnessRootValidated); +} + +// A tx re-mined in a different block (reorg) has its notes at new tree positions: the old nullifier +// must not be carried, so VerifyAndSetInitialWitness reseeds the note at its new position. +TEST(WalletNoteData, MovedTxDropsNullifier) { + NoteDataWallet wallet; + CWalletTx wtx(&wallet, CTransaction()); + SaplingOutPoint op0(uint256S("ee"), 0); + + SaplingMerkleTree tree; + tree.append(uint256S("04")); + wtx.mapSaplingNoteData[op0] = WitnessedNote(tree, 500, uint256S("cafe")); + + CWalletTx moved(&wallet, CTransaction()); + moved.mapSaplingNoteData[op0] = FreshNote(); + + EXPECT_TRUE(wallet.UpdatedNoteData(moved, wtx, false)); + const SaplingNoteData& nd = wtx.mapSaplingNoteData.at(op0); + EXPECT_FALSE(static_cast(nd.nullifier)); +} + +// When the incoming data really adds something (a note for a newly added key), the tx is updated, +// and the notes it already had keep their witnesses, witness height and nullifier together. +TEST(WalletNoteData, NewNoteUpdatesAndKeepsExistingNoteState) { + NoteDataWallet wallet; + CWalletTx wtx(&wallet, CTransaction()); + uint256 txid = uint256S("bb"); + SaplingOutPoint op0(txid, 0); + SaplingOutPoint op1(txid, 1); + + SaplingMerkleTree tree; + tree.append(uint256S("02")); + const uint256 nf = uint256S("beef"); + wtx.mapSaplingNoteData[op0] = WitnessedNote(tree, 200, nf); + + CWalletTx incoming(&wallet, CTransaction()); + incoming.mapSaplingNoteData[op0] = FreshNote(); + incoming.mapSaplingNoteData[op1] = FreshNote(); + + EXPECT_TRUE(wallet.UpdatedNoteData(incoming, wtx)); + ASSERT_EQ(2u, wtx.mapSaplingNoteData.size()); + const SaplingNoteData& nd0 = wtx.mapSaplingNoteData.at(op0); + ASSERT_TRUE(static_cast(nd0.nullifier)); + EXPECT_EQ(nf, *nd0.nullifier); + EXPECT_EQ(200, nd0.witnessHeight); + EXPECT_EQ(1u, nd0.witnesses.size()); + const SaplingNoteData& nd1 = wtx.mapSaplingNoteData.at(op1); + EXPECT_FALSE(static_cast(nd1.nullifier)); + EXPECT_TRUE(nd1.witnesses.empty()); + EXPECT_EQ(-1, nd1.witnessHeight); +} + +// Nothing is carried for a note that has no witnesses (PART B2): its height and nullifier describe +// no witness, so they must not survive onto the incoming data. +TEST(WalletNoteData, UnwitnessedNoteCarriesNothing) { + NoteDataWallet wallet; + CWalletTx wtx(&wallet, CTransaction()); + SaplingOutPoint op0(uint256S("cc"), 0); + + SaplingNoteData stale(TestIvk(), uint256S("dead")); + stale.witnessHeight = 300; + wtx.mapSaplingNoteData[op0] = stale; + + CWalletTx incoming(&wallet, CTransaction()); + incoming.mapSaplingNoteData[op0] = FreshNote(); + + EXPECT_TRUE(wallet.UpdatedNoteData(incoming, wtx)); + const SaplingNoteData& nd = wtx.mapSaplingNoteData.at(op0); + EXPECT_FALSE(static_cast(nd.nullifier)); + EXPECT_EQ(-1, nd.witnessHeight); + EXPECT_TRUE(nd.witnesses.empty()); +} + +// Incoming data with no Sapling notes never touches the wallet's note data. +TEST(WalletNoteData, EmptyIncomingIsNoUpdate) { + NoteDataWallet wallet; + CWalletTx wtx(&wallet, CTransaction()); + SaplingOutPoint op0(uint256S("dd"), 0); + SaplingMerkleTree tree; + tree.append(uint256S("03")); + wtx.mapSaplingNoteData[op0] = WitnessedNote(tree, 400, uint256S("abcd")); + + CWalletTx incoming(&wallet, CTransaction()); + EXPECT_FALSE(wallet.UpdatedNoteData(incoming, wtx)); + EXPECT_EQ(1u, wtx.mapSaplingNoteData.size()); + EXPECT_EQ(400, wtx.mapSaplingNoteData.at(op0).witnessHeight); +} diff --git a/src/init.cpp b/src/init.cpp index ce8322dc4..d04e45fc5 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1016,7 +1016,8 @@ extern int32_t HUSH_REWIND; // --- Adaptive coins-cache sizing ------------------------------------------------------------- // The in-memory UTXO/coins cache (nCoinCacheUsage) is the biggest lever on IBD speed: a bigger -// cache means far fewer chainstate flushes to disk. We size it to use most of RAM, but a scheduled +// cache means fewer size-triggered chainstate flushes to disk (the chainstate is also flushed at +// least every DATABASE_FLUSH_INTERVAL / DATABASE_FLUSH_BLOCKS, see main.h). We size it to use most of RAM, but a scheduled // background task (AdjustCoinCacheForMemoryPressure, registered in AppInit2) shrinks the target when // free system memory runs low — e.g. the user opens other apps — and grows it back when memory frees // up, always leaving a reserve free for the rest of the system. The existing per-block flush @@ -2229,6 +2230,9 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) // ********************************************************* Step 8: load wallet #ifdef ENABLE_WALLET + // Set when the startup rescan below is interrupted: the wallet's state then ends at the scan's + // own checkpoint, not at the tip the notifier starts from. + bool fWalletScanIncomplete = false; if (fDisableWallet) { pwalletMain = NULL; LogPrintf("Wallet disabled!\n"); @@ -2666,6 +2670,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) } else { LogPrintf("Rescan did not complete; leaving the best-block locator at the scan's own " "checkpoint so the remaining range is rescanned on the next start\n"); + fWalletScanIncomplete = true; } // Restore wallet transaction metadata after -zapwallettxes=1 @@ -2736,6 +2741,21 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) { LOCK(cs_main); pindexLastTip = chainActive.Tip(); +#ifdef ENABLE_WALLET + // The wallet's state reflects the block the notifier starts from, unless the startup + // rescan was interrupted: then it ends at the scan's checkpoint, and the hourly + // SetBestChain must keep stamping that so the next start rescans the rest. + if (pwalletMain) { + const CBlockIndex* pindexWallet = pindexLastTip; + if (fWalletScanIncomplete) { + CWalletDB walletdb(pwalletMain->strWalletFile); + CBlockLocator locator; + if (walletdb.ReadBestBlock(locator)) + pindexWallet = FindForkInGlobalIndex(chainActive, locator); + } + pwalletMain->SetWalletTip(pindexWallet); + } +#endif } boost::function threadnotifywallets = boost::bind(&ThreadNotifyWallets, pindexLastTip); threadGroup.create_thread( @@ -2777,8 +2797,24 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) uiInterface.InitMessage(_("Activating best chain...")); // scan for better chains in the block chain database, that are not yet connected in the active best chain CValidationState state; + const CBlockIndex* pindexBeforeActivate = NULL; + { + LOCK(cs_main); + pindexBeforeActivate = chainActive.Tip(); + } if ( !ActivateBestChain(true,state)) strErrors << "Failed to connect best block"; + else { + // Persist a startup replay now: the wallet's witness rebuild runs next and can hold + // cs_main for a long time, and a node killed before any later flush would replay it all again. + bool fTipMoved; + { + LOCK(cs_main); + fTipMoved = chainActive.Tip() != pindexBeforeActivate; + } + if (fTipMoved) + FlushStateToDisk(); + } } else { LogPrintf("HUSH_REWIND < 0\n"); } diff --git a/src/main.cpp b/src/main.cpp index 6bc40c659..492d0b66b 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3738,6 +3738,7 @@ bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) { static int64_t nLastWrite = 0; static int64_t nLastFlush = 0; static int64_t nLastSetChain = 0; + static int nLastFlushHeight = -1; std::set setFilesToPrune; bool fFlushForPrune = false; try { @@ -3763,6 +3764,9 @@ bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) { if (nLastSetChain == 0) { nLastSetChain = nNow; } + if (nLastFlushHeight < 0) { + nLastFlushHeight = chainActive.Height(); + } size_t cacheSize = pcoinsTip->DynamicMemoryUsage(); // The cache is large and close to the limit, but we have time now (not in the middle of a block processing). bool fCacheLarge = mode == FLUSH_STATE_PERIODIC && cacheSize * (10.0/9) > nCoinCacheUsage; @@ -3772,8 +3776,10 @@ bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) { bool fPeriodicWrite = mode == FLUSH_STATE_PERIODIC && nNow > nLastWrite + (int64_t)DATABASE_WRITE_INTERVAL * 1000000; // It's been very long since we flushed the cache. Do this infrequently, to optimize cache usage. bool fPeriodicFlush = mode == FLUSH_STATE_PERIODIC && nNow > nLastFlush + (int64_t)DATABASE_FLUSH_INTERVAL * 1000000; + // Many blocks connected since the last flush, e.g. a long replay inside one ActivateBestChain call. + bool fBlockCountFlush = (mode == FLUSH_STATE_IF_NEEDED || mode == FLUSH_STATE_PERIODIC) && chainActive.Height() - nLastFlushHeight >= DATABASE_FLUSH_BLOCKS; // Combine all conditions that result in a full cache flush. - bool fDoFullFlush = (mode == FLUSH_STATE_ALWAYS) || fCacheLarge || fCacheCritical || fPeriodicFlush || fFlushForPrune; + bool fDoFullFlush = (mode == FLUSH_STATE_ALWAYS) || fCacheLarge || fCacheCritical || fPeriodicFlush || fFlushForPrune || fBlockCountFlush; // Write blocks and block index to disk. if (fDoFullFlush || fPeriodicWrite) { // Depend on nMinDiskSpace to ensure we can write block index @@ -3827,6 +3833,7 @@ bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) { if (!pcoinsTip->Flush()) return AbortNode(state, "Failed to write to coin database"); nLastFlush = nNow; + nLastFlushHeight = chainActive.Height(); } if ((mode == FLUSH_STATE_ALWAYS || mode == FLUSH_STATE_PERIODIC) && nNow > nLastSetChain + (int64_t)DATABASE_WRITE_INTERVAL * 1000000) { // Update best block in wallet (so we can detect restored wallets). diff --git a/src/main.h b/src/main.h index 007e67891..89e962497 100644 --- a/src/main.h +++ b/src/main.h @@ -143,8 +143,15 @@ static const unsigned int MAX_LOCATOR_SZ = 101; static unsigned int BLOCK_DOWNLOAD_WINDOW = 1024; /** Time to wait (in seconds) between writing blocks/block index to disk. */ static const unsigned int DATABASE_WRITE_INTERVAL = 60 * 60; -/** Time to wait (in seconds) between flushing chainstate to disk. */ -static const unsigned int DATABASE_FLUSH_INTERVAL = 24 * 60 * 60; +/** Time to wait (in seconds) between flushing chainstate to disk. Upstream uses 24h to keep a large + * coins cache warm, but DragonX's cache stays a few MiB (private chain, few transparent outputs), so + * the size triggers never fire and this timer is all that persists the chainstate while a node runs: + * a node that never reached 24h uptime or a clean shutdown replayed weeks of blocks on every start. */ +static const unsigned int DATABASE_FLUSH_INTERVAL = 60 * 60; +/** Also flush chainstate once this many blocks were connected since the last flush. A long + * ActivateBestChain (startup replay, catching up after downtime) only flushes IF_NEEDED until it + * returns, so without this a node killed part-way through loses the whole replay. */ +static const int DATABASE_FLUSH_BLOCKS = 5000; /** Maximum length of reject messages. */ static const unsigned int MAX_REJECT_MESSAGE_LENGTH = 111; static const int64_t DEFAULT_MAX_TIP_AGE = 24 * 60 * 60; diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index e138b7322..f1018833f 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -203,6 +203,9 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) _("Error: A fatal internal error occurred, see debug.log for details"), "", CClientUIInterface::MSG_ERROR); StartShutdown(); + // Don't hand the wallet an empty block as if it had been processed. + fDisconnectIncomplete = true; + break; } // Let wallets know transactions went from 1-confirmed to @@ -239,6 +242,8 @@ void ThreadNotifyWallets(CBlockIndex *pindexLastTip) _("Error: A fatal internal error occurred, see debug.log for details"), "", CClientUIInterface::MSG_ERROR); StartShutdown(); + // Don't hand the wallet an empty block as if it had been processed. + break; } // Tell wallet about transactions that went from mempool diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 443498cba..425bda60d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -43,6 +43,7 @@ #include "wallet/asyncrpcoperation_autoshieldcoinbase.h" #include #include +#include #include #include #include @@ -575,6 +576,23 @@ void CWallet::ChainTip(const CBlockIndex *pindex, DecrementNoteWitnesses(pindex); UpdateNullifierNoteMapForBlock(pblock); } + + // Track the block the wallet's state reflects, advancing only along blocks delivered in order: + // a gap (the startup rescan was interrupted, so the wallet sits at its checkpoint behind the + // notifier) pins the pointer, and SetBestChain keeps stamping the checkpoint. + LOCK(cs_wallet); + if (added) { + if (pindexWalletTip == nullptr || pindex->pprev == pindexWalletTip) + pindexWalletTip = pindex; + } else if (pindex == pindexWalletTip) { + pindexWalletTip = pindex->pprev; + } +} + +void CWallet::SetWalletTip(const CBlockIndex* pindex) +{ + LOCK(cs_wallet); + pindexWalletTip = pindex; } void CWallet::RunSaplingSweep(int blockHeight) { @@ -801,10 +819,22 @@ bool CWallet::CommitAutomatedTx(const CTransaction& tx) { void CWallet::SetBestChain(const CBlockLocator& loc) { + // FlushStateToDisk passes the chain tip, but ThreadNotifyWallets delivers blocks to the wallet + // asynchronously and can lag it. Stamping a locator ahead of what the wallet has processed makes + // the next start skip those blocks for good (no rescan, no notification): notes received in them + // go missing and spends in them go unseen. So stamp the block the wallet's state reflects (seeded + // by init before the notifier starts, then advanced by ChainTip). Init's own calls, made before + // it is seeded, pass the right locator themselves. + CBlockLocator locator = loc; + { + LOCK2(cs_main, cs_wallet); + if (pindexWalletTip != nullptr) + locator = chainActive.GetLocator(pindexWalletTip); + } // Default ctor => fFlushOnClose=true => ~CDB runs a full BDB txn_checkpoint over the entire // cache. Fine for the hourly/shutdown callers; ruinous inside a scan (see SetBestChainNoFlush). CWalletDB walletdb(strWalletFile); - SetBestChainINTERNAL(walletdb, loc); + SetBestChainINTERNAL(walletdb, locator); } bool CWallet::SetBestChainNoFlush(const CBlockLocator& loc) @@ -1477,17 +1507,25 @@ void CWallet::BuildWitnessCache(const CBlockIndex* pindex, bool witnessOnly) work.reserve(pending.size()); for (const PendingNote& p : pending) work.push_back(p.nd); - // Only a substantial bulk rebuild (e.g. a one-time post-upgrade repair) is worth parallelizing - // and logging; routine 1-block tip extension runs serially to avoid per-block thread-spawn - // overhead and log spam. + // Only a substantial bulk rebuild (e.g. a one-time post-upgrade repair) is logged and has its + // anchors checked; routine 1-block tip extension stays quiet to avoid per-block log spam. const bool bulkRebuild = (rangeLen > 100); + // Parallelize whenever the extension itself is expensive, not only for a bulk range: a routine + // 1-block extension appends each of the block's commitments to every lagging note, so a large + // wallet and a busy block (13k notes x 20 commitments) is seconds of hashing under + // cs_main/cs_wallet. Spawning the threads costs ~1ms, so only small jobs stay serial. + uint64_t nCms = 0; + for (const std::vector& cms : blockCms) + nCms += cms.size(); + const bool fParallel = bulkRebuild || (uint64_t)work.size() * nCms >= 2000; + int nPar = (int)GetArg("-witnessbuildthreads", 0); if (nPar <= 0) nPar = (int)std::thread::hardware_concurrency(); if (nPar <= 0) nPar = 1; if (nPar > (int)work.size()) nPar = (int)work.size(); if (nPar < 1) nPar = 1; - if (!bulkRebuild) nPar = 1; + if (!fParallel) nPar = 1; std::atomic failed(false); std::mutex failMtx; @@ -1499,14 +1537,27 @@ void CWallet::BuildWitnessCache(const CBlockIndex* pindex, bool witnessOnly) // clone-every-block path for A/B verification. bool fastRebuild = GetBoolArg("-witnessfastrebuild", true); + // A bulk rebuild reports how many notes failed to reach the canonical anchor. root() walks the + // full depth-32 path (~2.4ms), so the check runs here on the workers and never on the routine + // per-block path, where it used to hold cs_main/cs_wallet for ~30s per block on a ~13k-note + // wallet purely to feed a log line that path never prints. + const uint256 canonicalRoot = pindex->hashFinalSaplingRoot; + std::atomic nUnrecoverable(0); + auto checkRoot = [&](const SaplingNoteData* nd) { + if (bulkRebuild && (nd->witnesses.empty() || nd->witnesses.front().root() != canonicalRoot)) + nUnrecoverable++; + }; + auto worker = [&](int tid) { try { for (size_t k = (size_t)tid; k < work.size(); k += (size_t)nPar) { SaplingNoteData* nd = work[k]; int startH = nd->witnessHeight; int nBlocks = tipHeight - startH; - if (nBlocks <= 0) + if (nBlocks <= 0) { + checkRoot(nd); continue; + } if (!fastRebuild || nBlocks <= CACHE) { // Reference / shallow path: clone every block (preserves pre-existing older snapshots). @@ -1547,6 +1598,7 @@ void CWallet::BuildWitnessCache(const CBlockIndex* pindex, bool witnessOnly) nd->witnesses.pop_back(); nd->witnessHeight = tipHeight; } + checkRoot(nd); } } catch (const std::exception& e) { std::lock_guard lk(failMtx); @@ -1561,9 +1613,18 @@ void CWallet::BuildWitnessCache(const CBlockIndex* pindex, bool witnessOnly) if (nPar <= 1) { worker(0); } else { + // If the OS refuses a thread (task limit, memory), run the remaining shards here instead of + // letting the exception unwind past joinable threads (std::terminate). Each shard still runs + // exactly once, so the result is unchanged. std::vector threads; threads.reserve(nPar); - for (int t = 0; t < nPar; t++) threads.emplace_back(worker, t); + int t = 0; + try { + for (; t < nPar; t++) threads.emplace_back(worker, t); + } catch (const std::system_error& e) { + LogPrintf("%s: could only start %d of %d witness threads (%s); running the rest inline\n", __func__, t, nPar, e.what()); + } + for (int u = t; u < nPar; u++) worker(u); for (std::thread& th : threads) th.join(); } int64_t tExtend = GetTimeMicros() - e0; @@ -1578,20 +1639,16 @@ void CWallet::BuildWitnessCache(const CBlockIndex* pindex, bool witnessOnly) // is unrecoverable here: it is left flagged (the GetSaplingNoteWitnesses majority-anchor guard // skips it for spends) and reported, rather than spun on indefinitely. witnessRootValidated is // in-memory only, so a fresh validation pass still runs on each restart and after any reorg - // (DecrementNoteWitnesses clears it), keeping the heal self-correcting. + // (DecrementNoteWitnesses clears it), keeping the heal self-correcting. The unrecoverable count + // itself comes from checkRoot() on the workers, and only for a bulk rebuild. if (!work.empty()) { - const uint256& canonicalRoot = pindex->hashFinalSaplingRoot; - int nUnrecoverable = 0; - for (SaplingNoteData* nd : work) { - if (nd->witnesses.empty() || nd->witnesses.front().root() != canonicalRoot) - nUnrecoverable++; + for (SaplingNoteData* nd : work) nd->witnessRootValidated = true; - } if (bulkRebuild) { LogPrintf("%s: rebuilt %u note witness cache(s) to height %d in %ldms using %d thread(s)%s\n", __func__, (unsigned)work.size(), tipHeight, (long)((tRead + tExtend) / 1000), nPar, - nUnrecoverable - ? strprintf(" [WARNING: %d note(s) could not be rebuilt to the canonical anchor and were skipped]", nUnrecoverable).c_str() + nUnrecoverable.load() + ? strprintf(" [WARNING: %d note(s) could not be rebuilt to the canonical anchor and were skipped]", nUnrecoverable.load()).c_str() : ""); } } @@ -1913,6 +1970,8 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD bool fUpdated = false; if (!fInsertedNew) { + // Re-mined in a different block (a reorg): its notes sit at new tree positions. + bool fSameBlock = wtx.hashBlock.IsNull() || wtxIn.hashBlock.IsNull() || wtxIn.hashBlock == wtx.hashBlock; // Merge if (!wtxIn.hashBlock.IsNull() && wtxIn.hashBlock != wtx.hashBlock) { @@ -1925,7 +1984,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD wtx.nIndex = wtxIn.nIndex; fUpdated = true; } - if (UpdatedNoteData(wtxIn, wtx)) { + if (UpdatedNoteData(wtxIn, wtx, fSameBlock)) { fUpdated = true; } if (wtxIn.fFromMe && wtxIn.fFromMe != wtx.fFromMe) @@ -1962,29 +2021,66 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD return true; } -bool CWallet::UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx) +// True if both maps hold the same notes with identical persisted state (every field SaplingNoteData +// serializes). SaplingNoteData::operator== ignores the witnesses, so it cannot tell a real change +// from a redundant update. +static bool SaplingNoteDataIdentical(const mapSaplingNoteData_t& a, const mapSaplingNoteData_t& b) +{ + if (a.size() != b.size()) + return false; + for (const mapSaplingNoteData_t::value_type& item : a) { + mapSaplingNoteData_t::const_iterator it = b.find(item.first); + if (it == b.end()) + return false; + const SaplingNoteData& x = item.second; + const SaplingNoteData& y = it->second; + if (!(x.ivk == y.ivk) || x.nullifier != y.nullifier || x.witnessHeight != y.witnessHeight || x.witnesses != y.witnesses) + return false; + } + return true; +} + +bool CWallet::UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx, bool fSameBlock) { bool unchangedSaplingFlag = (wtxIn.mapSaplingNoteData.empty() || wtxIn.mapSaplingNoteData == wtx.mapSaplingNoteData); - if (!unchangedSaplingFlag) { - auto tmp = wtxIn.mapSaplingNoteData; - // Ensure we keep any cached witnesses we may already have + if (unchangedSaplingFlag) + return false; - for (const std::pair nd : wtx.mapSaplingNoteData) { - if (tmp.count(nd.first) && nd.second.witnesses.size() > 0) { - tmp.at(nd.first).witnesses.assign( - nd.second.witnesses.cbegin(), nd.second.witnesses.cend()); - //PART B2: only carry over witnessHeight TOGETHER with the witnesses it describes. - //Copying it unconditionally (when witnesses are NOT copied) advances the height past - //the actual witness state for a whole tx's notes at once = the batch desync originator. - tmp.at(nd.first).witnessHeight = nd.second.witnessHeight; - } + auto tmp = wtxIn.mapSaplingNoteData; + // Ensure we keep any cached witnesses we may already have + + for (const mapSaplingNoteData_t::value_type& nd : wtx.mapSaplingNoteData) { + if (tmp.count(nd.first) && nd.second.witnesses.size() > 0) { + tmp.at(nd.first).witnesses.assign( + nd.second.witnesses.cbegin(), nd.second.witnesses.cend()); + //PART B2: only carry over witnessHeight TOGETHER with the witnesses it describes. + //Copying it unconditionally (when witnesses are NOT copied) advances the height past + //the actual witness state for a whole tx's notes at once = the batch desync originator. + tmp.at(nd.first).witnessHeight = nd.second.witnessHeight; + //The nullifier is derived from the note and its witness position, so it travels with the + //witnesses too. wtxIn comes from FindMySaplingNotes, which never sets it; dropping it made + //VerifyAndSetInitialWitness clear and reseed the witnesses just carried over, so replaying + //or rescanning already-witnessed txs forced a rebuild of every one of their notes. A tx + //re-mined in another block is the exception: its notes moved, so the old nullifier is + //wrong and dropping it still reseeds them at the new position. + if (nd.second.nullifier && fSameBlock) + tmp.at(nd.first).nullifier = nd.second.nullifier; } - - // Now copy over the updated note data - wtx.mapSaplingNoteData = tmp; } - return !unchangedSaplingFlag; + // A replayed or rescanned tx usually carries nothing new; don't rewrite it to disk. Do have + // VerifyAndSetInitialWitness re-check the kept witnesses against the active chain once (the + // flag is in memory only): the chain may have moved under them, and it keeps a rescan able to + // find and reseed a witness that went bad, without rebuilding the healthy ones. + if (SaplingNoteDataIdentical(tmp, wtx.mapSaplingNoteData)) { + for (mapSaplingNoteData_t::value_type& item : wtx.mapSaplingNoteData) + item.second.witnessRootValidated = false; + return false; + } + + // Now copy over the updated note data + wtx.mapSaplingNoteData = tmp; + return true; } /** diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 107814971..ebcb64bb1 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -965,8 +965,12 @@ private: template void SyncMetaData(std::pair::iterator, typename TxSpendMap::iterator>); + // Last block ThreadNotifyWallets delivered to ChainTip, i.e. the block the wallet's state + // reflects (guarded by cs_wallet). SetBestChain stamps this instead of the chain tip. + const CBlockIndex* pindexWalletTip = nullptr; + protected: - bool UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx); + bool UpdatedNoteData(const CWalletTx& wtxIn, CWalletTx& wtx, bool fSameBlock); void MarkAffectedTransactionsDirty(const CTransaction& tx); /* the hd chain data model (chain counters) */ @@ -1294,6 +1298,8 @@ public: bool CommitAutomatedTx(const CTransaction& tx); /** Saves witness caches and best block locator to disk. Overrides CValidationInterface. */ void SetBestChain(const CBlockLocator& loc); + // Seed the block the wallet's state reflects, before ThreadNotifyWallets starts delivering. + void SetWalletTip(const CBlockIndex* pindex); /** As SetBestChain, but for use INSIDE a long scan: opens the wallet DB with fFlushOnClose=false * so the call does not trigger a full BDB txn_checkpoint over the whole cache (see the comment * at CWallet::SetBestChain), and reports whether the write actually committed. */