- UpdatedNoteData now keeps the incoming plaintext when it keeps the
wallet's own entries. When it returns "unchanged" (equal note data,
or SaplingNoteDataIdentical), any entry lacking a plaintext takes the
incoming one, for the same outpoint and ivk only. The startup rescan
replays loaded txs with FindMySaplingNotes' freshly decrypted data;
that plaintext was thrown away, so the warm-up decrypted those notes a
second time. The cache is outside every comparison, so the function
still returns false and nothing is rewritten.
- -checknotecache is read once at init into fCheckNoteCache, instead of
a mapArgs lookup on every GetFilteredNotes call. Its help text and
comment now say what it checks: every note the walk considers, before
the address filter, so a corrupt cached address can't drop a note
unchecked.
- GetFilteredNotes pre-sizes its result only when there's no address
filter, so a one-address call no longer reserves room for the whole
wallet.
- WarmSaplingNoteCache returns its counts (notes, filled, no such
output, undecryptable), and FillSaplingNoteCache records the threads
it used, so tests can tell the cases apart.
- WarmSaplingNoteCache's comment no longer claims nothing can wait on
cs_wallet while it runs: the RPCs allowed during warmup can.
Tests:
- ReplayWithPlaintextIsNoUpdate pins the rule that the plaintext cache
stays out of the "did this tx change?" decision. A rescan replays a
witnessed note loaded without plaintext, using FindMySaplingNotes'
data that carries one. That must be no update: no rewrite, and the
witness and nullifier are kept. The stored entry then gains the
plaintext. Checked by mutation: with cache presence added to
SaplingNoteDataIdentical, this test fails and every other test still
passes.
- ParallelFillMatchesSerial uses 460 notes and asserts the thread count.
So the 7-thread case really runs 7 threads, with uneven shards
(460 % 7 != 0). Before, it was capped at 4 and repeated the 4-thread
case.
- ColdParallelFillMatchesLegacy: a cold GetFilteredNotes over ~270
candidates fills on 4 threads and matches the 69bb035e7 loop entry
for entry.
- WarmUpSkipsBadNotes asserts the warm-up's separate counts (1 no such
output, 2 undecryptable), so removing the out-of-range guard is now
caught.
- CachedPlaintextMatchesFreshDecrypt checks exact counts.
CheckNoteCacheRepairsADifference also tampers with a memo, and looks
entries up by outpoint. The meaningless GetHash check is dropped.
- A ScopedArg / ScopedCheckNoteCache RAII helper restores -args and
fCheckNoteCache even when an assertion returns early.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With -checknotecache=1 (debug help), GetFilteredNotes decrypts every
note it returns again. If the cached plaintext differs from the fresh
decryption, it logs the note and replaces the cache entry with the
fresh value. It costs the full decryption the cache exists to avoid, so
it is off by default; it is there for validating the cache on a real
wallet, or for diagnosing a suspected cache problem in the field.
CheckNoteCacheRepairsADifference: by default a tampered cache entry is
returned as is (the cache is trusted); with the option it is repaired
and the fresh value is returned.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The startup warm-up, or a lazy fill after -notecachewarmup=0, decrypts
the whole wallet: ~0.5ms per note, ~6s serial for 12k notes. Each
decryption is independent: it reads its own output and writes only its
own note's cachedPlaintext. So FillSaplingNoteCache now splits batches
of 128 or more notes round-robin across -witnessbuildthreads (default:
hardware_concurrency) workers, at least 64 notes per thread. It follows
BuildWitnessCache's Phase 2 pattern:
- the caller keeps cs_wallet held throughout;
- workers take no lock and assert none; under DEBUG_LOCKORDER a fresh
thread has no lock stack;
- per-shard fill counters are summed after join;
- nothing escapes a worker: a throwing decryption leaves that note
unfilled, like one that doesn't decrypt;
- if the OS refuses a thread, the remaining shards run inline.
librustzcash is already called concurrently by BuildWitnessCache's
workers, and libsodium is thread-safe after sodium_init.
ParallelFillMatchesSerial: 300 notes across two keys and diversified
addresses decrypt to exactly the same cache with 1, 4 and 7 threads.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The plaintext cache is in memory only, so a restart empties it. Without
a warm-up, the first balance call after a restart decrypts the whole
wallet under LOCK2(cs_main, cs_wallet): ~6s for a 12k-note wallet. That
is the observed failure: restart, the GUI connects, and the node looks
stuck.
CWallet::WarmSaplingNoteCache decrypts every note whose plaintext isn't
cached, spent ones included, because z_listreceivedbyaddress
(ignoreSpent=false) is called once per address by the GUI's history
scan.
init runs it at the end of the wallet block, just before
SetBroadcastTransactions. At that point RPC is still in warmup (it
shows "Decrypting wallet notes...") and the wallet notification
thread, whose startup witness rebuild can hold cs_wallet for a long
time, has not started yet. It takes cs_wallet only. -notecachewarmup=0
(debug help) skips it, which leaves the lazy fill in GetFilteredNotes
to do the work.
Unlike GetFilteredNotes, the warm-up never asserts. A note that doesn't
decrypt under its ivk, or names an output its tx lacks, is counted in
the summary line and listed under -debug=zrpcunsafe. A corrupt note
therefore can't stop the node from starting, or from serving the
key-export RPCs allowed during warmup; GetFilteredNotes still asserts
on it, as before.
Tests:
- WarmUpFillsEveryNote: spent, locked, unconfirmed and stale-block
notes included; walks afterwards decrypt nothing.
- WarmUpSkipsBadNotes: undecryptable and out-of-range notes are skipped
without aborting.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GetFilteredNotes, the walk behind every balance and note-listing RPC,
sweep, consolidation, z_sendmany and z_mergetoaddress, decrypted every
note it looked at on every call: ~0.5ms of Jubjub arithmetic per note,
under LOCK2(cs_main, cs_wallet). A note's plaintext is fixed by its
output (the txid commits to the ciphertext) and the ivk that decrypts
it, so it never needs to be decrypted twice.
SaplingNoteData gains an in-memory-only
boost::optional<SaplingNoteCache> {note, memo}; the address is the
note's (d, pk_d). It follows the witnessRootValidated precedent:
- never serialized, and left out of operator== and
SaplingNoteDataIdentical, so filling it never makes a tx look changed
and get rewritten (the replay rewrite storm 69bb035e7 removed);
- FindMySaplingNotes fills it from the decryption that found the note
(both the spending-key and the viewing-key-only loop);
- UpdatedNoteData's replace branch keeps a plaintext the wallet already
had, for the same outpoint and ivk only;
- GetFilteredNotes now runs in two passes: pass 1 applies the tx,
spent and locked filters and collects the notes that pass; whatever
lacks a plaintext (everything after a restart, until the startup
warm-up in a later commit) is decrypted once via the new
FillSaplingNoteCache; pass 2 applies the address and spending-key
filters and emits entries in the same order as before.
Spentness, spendability, lock state and depth are still decided on every
call; only the plaintext is cached.
The A0 equivalence test passes unchanged (warm). New tests:
- ColdCacheMatchesLegacy: the same grid with the cache dropped before
every call;
- CachedPlaintextMatchesFreshDecrypt: eager and lazy fills;
- WarmPassDoesNotDecrypt: a fill counter shows a warm walk decrypts
nothing, and spent or locked notes only when the caller asks for
them;
- CacheNeverSerialized: byte-identical SER_DISK/NETWORK/GETHASH output,
empty after a load;
- UpdatedNoteDataAndTheCache: no rewrite on a cache-only difference,
carried on replace, never across another ivk;
- SpendabilityNotCached;
- FindMySaplingNotesFillsCache.
Memory: ~616 bytes per note (the memo is 512), ~8MB for a 13k-note
wallet, next to ~100KB per note of cached witnesses.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SaplingNotePlaintext::decrypt(ciphertext, ivk, epk, cmu) derives
pk_d = [ivk]g_d to check the note commitment, then throws it away. The
wallet then calls ivk.address(d), which derives pk_d again, and
plaintext.note(ivk), which derives it a third time. Each derivation is a
Jubjub scalar multiplication, ~115us, measured. Including the key
agreement and the commitment check, the whole sequence is ~775us per
note.
decrypt_with_note() runs the same decryption and checks and returns the
note built from that pk_d. ivk_to_pkd succeeding already implies a
valid diversifier, which is all ivk.address() checks on top. It gets its
own name rather than a decrypt() overload: an overload taking a fifth
argument by non-const reference would capture calls meant for the
(epk, esk, pk_d, cmu) variant. The cmu check stays.
GetFilteredNotes and FindMySaplingNotes' spending-key loop use it. That
saves two of the three derivations per note in every balance walk, and
one per found note during scans and rescans (trial decryption of
outputs that aren't ours still fails at the AEAD, as before).
FindMySaplingNotes also no longer copies each OutputDescription.
New tests: MatchesDecryptThenNote checks decrypt_with_note field for
field against decrypt() + note(ivk), over default and diversified
addresses, a wrong ivk and a wrong cmu. FindMySaplingNotes pins what
discovery returns: notes per key, diversified addresses to register,
foreign outputs ignored. Both pass before and after the switch.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review follow-ups to the GetFilteredNotes / RPC timing logs:
- Time with std::chrono::steady_clock instead of GetTimeMicros(), which
is the system clock. A wall-clock step otherwise writes false
">= 500ms" lines unconditionally; on a WSL2 host the clock was seen
stepping +3.6s every ~32s.
- A long-poll getblocktemplate deliberately blocks (with cs_main
released) until a new block arrives, so it is logged only under
-debug=bench.
- Log the method as "RPC method=<name> took <n>ms". A bare " rescan "
token followed by "<n>ms" matches ObsidianDragon's rescan-finished
log heuristic.
- The always-on slow-walk line keeps just the filters and the walk
time; the wallet-size counts (txs, notes, spent, entries) are logged
only with -debug=bench, so a shared debug.log doesn't carry them.
- The -txindex=0 startup line names the version.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
For minDepth > 1, GetFilteredNotes located each wallet tx's block with
tx_height(), which calls GetTransaction and re-reads the tx from disk
through the txindex. It did this once per wallet tx, transparent ones
included, while holding LOCK2. z_listunspent and z_listreceivedbyaddress
did it once more per returned note. Without -txindex, GetTransaction can
only find a tx that still has unspent transparent outputs. tx_height()
then returned 0 for every fully shielded tx, and the tx was skipped. So
on a -txindex=0 node, sweep and consolidation (minDepth 11) and any
minconf >= 2 call never saw those notes.
The wallet already has the depth, and for an active-chain tx
GetDepthInMainChain is chainActive.Height() - height + 1. The new
HeightFromDepth() inverts that (0 when unconfirmed or off the active
chain) and replaces tx_height() at all three sites. The height only
feeds the == 0 skip and hush_dpowconfs. Both sides hold cs_main for the
whole call, so the tip can't move between the walk and the RPC tail.
On a -txindex node the results are identical: the txindex block and
wtx.hashBlock are the same block. The extended equivalence test now
also covers minDepth 2/11/12/22. On -txindex=0 nodes, fully shielded
notes now show up for minDepth > 1. With -zsweep or -consolidation
enabled, those notes will now be swept and consolidated, so init logs
that once. MinDepthAboveOneUsesWalletHeight pins the new behaviour and
shows that the old path returned nothing without a txindex.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GetFilteredNotes copied every CWalletTx by value before any filter ran,
then copied each SaplingNoteData by value again. Both copies deep-copy
the note's witness list (up to WITNESS_CACHE_SIZE witnesses, ~100KB per
note on a mainnet wallet): ~40us per note per pass, measured. Spent and
locked notes were also fully decrypted (~0.6ms of Jubjub arithmetic per
note) before the spent and locked checks threw them away. -deletetx is
off by default, so spent notes accumulate.
- iterate mapWallet and mapSaplingNoteData by const reference;
- skip txs with no notes of ours before CheckFinalTx;
- read GetDepthInMainChain once per tx instead of 2+N times (cs_main is
held, so it can't change during the walk);
- run the spent and locked checks before the decryption;
- z_listreceivedbyaddress no longer copies the whole CWalletTx per
entry to read its time, and z_sendmany's sort comparator takes its
entries by reference.
The filters have no side effects, so the result set and its order are
unchanged; the A0 equivalence test passes unchanged. One deliberate
difference: a spent or locked note whose ciphertext no longer decrypts
under its ivk is now skipped instead of tripping the decrypt assert. The
new SpentAndLockedNotesAreNotDecrypted test pins this (it aborts on the
old ordering).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GetFilteredNotes logged "GetFilteredNotes ignoreLocked=N" on every call.
The balance RPCs run it on every GUI poll, so that line was log spam on
a fast wallet, and on a slow one the gaps between its timestamps were
the only timing there was. Those gaps also count lock waits and the
previous RPC's own post-processing.
It now logs one summary per walk: the filters, txs that passed the
tx-level checks out of the wallet total, notes examined, notes skipped
as spent, entries returned, and the walk time. The summary is always
written when the walk takes >= 500ms (that long under cs_main stalls
block connection) and otherwise only with -debug=bench. The line has no
lock-wait figure: every caller already holds LOCK2(cs_main, cs_wallet)
and CCriticalSection is recursive, so the inner LOCK2 never waits.
Lock waits are measured where they happen instead. CRPCTable::execute
logs "RPC <method> took <n>ms" under the same >= 500ms / -debug=bench
rule. The per-locked-note line moves to -debug=zrpcunsafe, since it
logs txid prefixes.
No behaviour change otherwise.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A note found through an incoming viewing key only (z_importviewingkey)
has no full viewing key behind it. UpdateSaplingNullifierNoteMapWithTx
skips such notes, so they never get a nullifier. VerifyAndSetInitialWitness
treated "no nullifier" as "witness needs reseeding" on every call: it
cleared the note's witness, re-read the block from disk, reseeded it,
and then dereferenced the nullifier that never came. dragonxd aborted
in the txnotify thread (boost::optional "is_initialized" assert) on the
first block confirming a note to such an address, and again after every
restart, before the block index was flushed.
Such a note can't be spent, so it needs no witness. Skip it there; if
its spending key is imported later, it has a full viewing key and the
existing no-nullifier rule seeds it then. SaplingWitnessMinimumHeight
now takes the optional nullifier. A note without one is never extended
by BuildWitnessCache (its pending-note gate requires one), so it must
not lower the rebuild's start height. Every call site that worked before
passes a set nullifier and behaves as before.
The deref came in with the 2020 witness rework (6c48397cf). The new
gtest ViewingKeyOnlyNoteIsNotWitnessed fails on the old code (the note
reaches the reseed path).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A v1.3.0 node with a ~13.6k-note wallet (-dbcache=4084) looked hung for hours:
every start replayed ~62k blocks from a chainstate 26 days stale, then ran a
47-60 min all-core witness rebuild holding LOCK2(cs_main, cs_wallet) (RPC answered
HTTP 500 "Work queue depth exceeded", peers timed out), the GUI killed it before
anything was flushed, and the next start did it all again. Even after escaping,
each new block cost ~30s under LOCK2, so the node fell behind a 36s chain.
D1 BuildWitnessCache: the post-rebuild latch computed witnesses.front().root()
(~2.4ms) for every pending note on every block, only to feed a log line that
is printed for bulk rebuilds alone. The anchor check now runs on the Phase-2
workers and only for a bulk rebuild; the per-block path just sets the latch.
Phase 2 also runs in parallel whenever the job is big (notes x commitments
>= 2000), not only for a bulk range, so busy blocks no longer extend every
note serially. If the OS refuses a worker thread, the remaining shards run
inline instead of unwinding past joinable threads (std::terminate).
D2 FlushStateToDisk: the coins cache was only flushed on shutdown, gettxoutsetinfo
or after 24h; on this chain it stays a few MiB, so the size triggers never
fire and a node that never reached 24h uptime or a clean stop kept its
chainstate (and block index) weeks behind. Flush at least hourly and every
DATABASE_FLUSH_BLOCKS (5000) connected blocks -- the latter also inside one
long ActivateBestChain -- and right after the startup ActivateBestChain when
it moved the tip.
D3 UpdatedNoteData: a replayed or rescanned tx arrives with FindMySaplingNotes
data (no nullifier). Witnesses and witnessHeight were carried over but the
nullifier was dropped, so VerifyAndSetInitialWitness cleared and reseeded the
witnesses on the next block -- the replay above rewrote 3,750 txs and forced
the full rebuild. Carry the nullifier with the witnesses, unless the tx was
re-mined in a different block (its notes moved; reseed as before). When the
merged data is identical, don't rewrite the tx; just clear the in-memory
witnessRootValidated so the next block re-checks the kept roots once (a
rescan still finds and reseeds a witness that went bad, without rebuilding
the healthy ones). The carry loop no longer deep-copies every note.
SetBestChain: with hourly coin flushes the wallet locator must not run ahead
of the async notifier, or a crash permanently skips the blocks it had not yet
delivered. Stamp the block the wallet's state reflects (pindexWalletTip,
seeded in init before ThreadNotifyWallets starts -- at the startup rescan's
checkpoint if that rescan was interrupted -- and advanced contiguously by
ChainTip), like zcashd's ChainTipAdded. ThreadNotifyWallets no longer hands
the wallet an empty block after a fatal block-read failure.
Tests: new gtest test_wallet_notedata.cpp (5 tests); 23/23 gtests pass, and the
two core D3 tests fail with D3 reverted. Regtest A/B against master with a
1,008-note wallet: notify-thread CPU per block 1,780ms -> 3ms; worst LOCK2 RPC
stall 1.6-2.9s -> 10-15ms on ordinary blocks and 4.6-7.1s -> ~1.6s on a block
carrying a 40-output self-send; a z_importkey rescan went from 35 reseeds plus a
bulk rebuild (10.6s) to 0 reseeds, same balance; after kill -9 at 150 blocks a
build with a 100-block flush threshold restarted at 100, master at 0. Two
adversarial review rounds; every confirmed finding is addressed here.
Not covered: the hourly SetBestChain/flush paths were reviewed, not exercised
(GetTimeMicros ignores mocktime); no run yet against a copy of a real large wallet.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An aborted or shutdown-interrupted rescan discarded all of its progress: nothing
advanced the on-disk best-block locator, so the next run started over. On a large
shielded wallet that is a multi-minute witness rebuild repeated from scratch.
Writes the locator ONCE, at the interrupt, rather than periodically through the
scan. That delivers the whole stated benefit -- "resume where it stopped" -- at
the cost of exactly one SetBestChain per interrupted scan. A periodic checkpoint
would additionally survive SIGKILL and power loss, a much weaker requirement, and
it is the part that carries all the cost: SetBestChainINTERNAL is O(whole wallet)
regardless of progress, so on the reported 5.3k-tx / 7.5k-note wallet a checkpoint
every 2500 blocks would plausibly cost more than the rebuild it was meant to save.
If someone later demonstrates a need for crash resilience mid-scan, add it then,
with a measured interval and fFlush=false -- not before.
Two guards, both necessary:
- CONTIGUITY. The RPC entry points (rescan / importprivkey / z_importkey /
z_importviewingkey) take a caller-supplied start height validated only against
chainActive.Height(), never against the wallet's own persisted locator. A scan
beginning above that locator must not checkpoint at all, or it would record
the skipped range as scanned and hide any funds in it. Logged once when it
applies, so an operator can see why an interrupt did not persist.
- MONOTONICITY. A checkpoint may only advance the locator, never move it back.
The locator points at the last FULLY PROCESSED block -- the parent of the block we
were about to scan -- so resume restarts one block early. CChain::GetLocator pushes
its argument first and FindForkInGlobalIndex returns that same block, so resume
begins AT it; the one-block overlap is deliberate and idempotent, since AddToWallet
only takes its merge path when the transaction is already present.
No witness work is done at the interrupt, which answers the two "//TODO: should we
update witness caches?" comments this replaces: witnesses are re-derived from each
note's own witnessHeight independently of the locator, and witnessRootValidated is
never serialized, so every note is revalidated against hashFinalSaplingRoot on the
next start. A mid-scan checkpoint does capture notes at mixed witnessHeights -- the
in-loop BuildWitnessCache(pindex, true) returns before the extension phase and only
seeds new notes at their own transaction's height -- but that state is recoverable:
the post-loop BuildWitnessCache(tip, false) levels every note, and it now occurs at
most once per scan instead of hundreds of times.
Uses SetBestChainNoFlush so the checkpoint does not trigger a full BDB
txn_checkpoint over the whole cache, and reports failure rather than silently
leaving the operator to discover the replay.
Depends on the fLastRescanCompleted gate in the previous commit: without it
init.cpp would overwrite this checkpoint with a tip locator immediately after the
scan returns.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FU87LdsJZiZkfq1eXubpeo
Two independent fund-visibility bugs on the rescan path, both pre-existing.
1. AN INTERRUPTED RESCAN RECORDED ITSELF AS COMPLETE.
ScanForWalletTransactions returns a bare `int ret` (a found-tx count) on both its
abort and shutdown bail-outs, so the caller could not tell "finished" from
"stopped at block H". init.cpp then ran, unconditionally:
pwalletMain->ScanForWalletTransactions(pindexRescan, true);
pwalletMain->SetBestChain(chainActive.GetLocator()); // TIP locator
An interrupted scan therefore stamped the wallet as scanned all the way to the
chain tip. On the next start, the `chainActive.Tip() != pindexRescan` guard just
above sees no work to do and skips the rescan entirely, so every transaction in
the never-scanned range stays out of mapWallet -- permanently invisible to
getbalance and unspendable. That is exactly the case `-rescan` exists for: a key
import, right after ClearNoteWitnessCache has run.
Adds CWallet::fLastRescanCompleted, set false at scan entry and true only on the
normal exit, and gates the init.cpp locator write on it. Deliberately NOT
overloading the int return, which callers already use as a tx count.
2. fAbortRescan WAS NEVER RESET.
wallet.h declares it, AbortRescan() sets it true, and nothing in src/ ever sets
it false. The abortrescan RPC is live. After one call, for the remaining lifetime
of the process:
- every ScanForWalletTransactions returns immediately, so re-running an import
silently no-ops while the RPC still reports success;
- BuildWitnessCache bails on the same flag on EVERY call, including the routine
per-block extension from ChainTip.
Witness heights then diverge across notes while the chain advances, and
GetSaplingNoteWitnesses elects the majority root as the anchor and returns
boost::none for every note that disagrees. Those notes still show in the balance
but cannot be spent. Cleared at scan entry and consumed in the abort branch.
Also in BuildWitnessCache's abort branch: stop clearing fRescanning. A witness
rebuild is not a rescan and must not touch a flag ScanForWalletTransactions owns.
3. Both bail-out log lines had two format specifiers and one argument:
LogPrintf("%s: Rescan aborted at block %d\n", pwalletMain->rescanHeight);
tinyformat's "too many conversion specifiers" guard is disabled in this tree
(tinyformat.h: `if(*fmt != '\0' && 0 ) // disabled due to complaints`), so this
does not throw -- verified by compiling the exact call against this tree's
tinyformat.h. It emits "3841207: Rescan aborted at block " with the height in the
__func__ slot, the real height dropped, and the trailing newline swallowed so the
next log line concatenates onto it. On the one path an operator has to diagnose
an interrupted rescan, the only record was corrupt. Fixed at both sites, and the
state updates moved above the log call.
4. Makes SetBestChainINTERNAL report whether the atomic write committed, so a
checkpointing caller can tell (six failure paths returned void). Adds
SetBestChainNoFlush, which opens the wallet DB with fFlushOnClose=false: the
default ctor makes ~CDB run a full BDB txn_checkpoint over the whole cache, which
is fine hourly but not from inside a scan -- the same flush wallet.cpp already
documents avoiding elsewhere "for performance reasons". Nothing calls it yet.
5. SetBestChainINTERNAL took each CWalletTx by value, deep-copying every note's
witness deque (WITNESS_CACHE_SIZE entries) per transaction per call, purely to
serialize it. Now by const reference.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FU87LdsJZiZkfq1eXubpeo
A wallet.dat repaired by v1.0.3-or-earlier -salvagewallet could not be opened by
this build at all. Those builds' IsKeyType has no "hdchain" case, so salvage
dropped the record; LoadWallet then returned DB_CORRUPT and told the user to
restore from a seed phrase. There is no such recovery path: the node aborts
before the RPC server exists, -usemnemonic defaulted to 0 in v1.0.3 so many of
these wallets never had a phrase, and SetHDSeedFromMnemonic refuses a non-empty
wallet, so "move wallet.dat aside" discards every non-HD key salvage preserved.
The guard itself was right to refuse to DERIVE -- a missing hdchain falls back to
SetNull defaults, clearing fMnemonicSeed and switching derivation from the
64-byte BIP39 seed to the raw 32-byte entropy, an entirely different key tree.
It was wrong to refuse to OPEN. Split the two:
- "hdchain" record ABSENT (the salvage signature, tracked by a new
wss.fHDChainSeen set before any parse attempt) -> open DEGRADED.
- "hdchain" record PRESENT but unreadable -> still DB_CORRUPT; that signals
wider file damage.
Degraded means the wallet spends, receives and rescans normally but refuses to
derive any new HD key. Gated at the single choke point GetHDSeedForDerivation
rather than at the two generators, so z_getnewaddress/sendmany/shieldcoinbase
raise a clean error and autoshield skips. hdChain.seedFp is deliberately left
null, which keeps IsHDTransparentEnabled false so getnewaddress falls back to the
legacy random-key path -- gating it instead would break TopUpKeyPool, change
addresses and block templates. -rescan is soft-set because an old salvage also
dropped defaultkey and bestblock, which would otherwise look like a first run and
skip the rescan, leaving a permanently zero balance.
NEW DETECTOR, covering strictly more damage than the guard beside it. A wallet
salvaged by v1.0.3 and then USED on v1.0.3 persists a SetNull-derived hdchain
carrying seedFp=null. On this build fHDChainRead is then true, the guard never
fires, the node starts perfectly clean -- and derives into the wrong key tree
forever, silently. That state is unforgeable by any legitimate writer, since
InstallHDSeed always stores seedFp = seed.Fingerprint(), so compare the loaded
chain's seedFp against the fingerprint taken from the hdseed/chdseed record KEY
(which works while an encrypted wallet is locked) and degrade on mismatch.
Also:
- init.cpp: abort in the DB_CORRUPT branch itself, as DB_NEED_REWRITE already
does. Falling through ran several hundred more lines against a wallet just
declared corrupt, including SetHDSeedOrigin(), which WRITES to it. Error text
no longer advises -mnemonic (wrong twice over, see above).
- rpcdump.cpp: z_exportwallet discarded GetHDSeedForDerivation's return and
emitted the line regardless, so a failure wrote a blank seed next to a
legitimate-looking BLAKE2b-of-empty fingerprint -- a backup that looks valid
and restores nothing.
- Corrected the guard's comment: the saplingAccountCounter half was overstated.
Both generators loop while(Have...Key(...)), so a low counter walks forward
past existing accounts rather than colliding with them.
qa/r4-salvaged-wallet-harness.sh builds the victim wallets with the on-disk
v1.0.1 release binary and asserts the behaviour end to end. It REFUSES to run
outside a network namespace and re-checks getconnectioncount==0, because those
old binaries predate the regtest seed-injection fix and would otherwise dial the
live network. 11/11 pass:
C healthy wallet opens and is NOT flagged (no false positives)
A salvaged wallet opens, flagged, persists no hdchain, z_getnewaddress refused,
getnewaddress still works, z_exportwallet emits no bogus HDSeed line
B poisoned wallet opens and the seedFp detector fires
Note the harness asserts "no hdchain synthesised", NOT "wallet.dat unchanged":
a control run showed normal startup rewrites wallet.dat for healthy wallets too.
This is the access/detection half. Proving fMnemonicSeed by re-deriving against
held keys, and repairing the record, is deliberately left out: it requires a
write, and the affected population is unmeasured. dev's own salvage already
preserves hdchain, so the class is closed going forward.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FU87LdsJZiZkfq1eXubpeo
From the Phase-6 structural scoping: land the verified behavioral bugs and
the safe quick-wins/dedups now; the large refactors (monolith splits, ~180
header globals) and consensus-adjacent items stay deferred. Built clean;
self-mined; verifychain=true.
BUGS (verified by reading the code):
- net.cpp CNode::Ban: a braceless `if (subNet.Match(...))` left
`pnode->fDisconnect = true;` OUTSIDE the guard, so banning any one subnet
marked EVERY connected peer for disconnect (dropped the whole peer set).
Wrapped the two statements in braces. (LIVE, high severity.)
- rpcdump.cpp importwallet: the `!fGood -> throw "Error adding some keys"`
check was trapped inside the `if (fRescan)` block, so importwallet with
rescan=false silently reported success when key import failed. Hoisted the
check before the rescan branch and cleaned the garbled braces/indentation.
- wallet.cpp CommitTransaction: ignored AddToWallet()'s return, so a failed
disk-persist of a just-signed spend was swallowed while the tx broadcast.
Now logs a hard error on failure.
- hush_nSPV_fullnode.h: the UTXOS branch declared `uint8_t filter` while the
twin TXIDS branch uses `uint32_t filter`; dragon_rwnum switches on
sizeof(filter), so the utxos path parsed only 1 of 4 wire filter bytes.
Widened to uint32_t.
- asyncrpcoperation_sweep.cpp: LogPrintf("%s ... %s", one-arg) read a missing
vararg; added the __func__ argument.
- rpcdump.cpp importprivkey: inner `auto secret_key` shadowed the outer
uint8_t and changed the type into DecodeCustomSecret; dropped the shadow.
- rpcdump.cpp getrescaninfo: char[8] + sprintf("%.4f") overflows when the
ratio >= 10.0 (transient reorg); widened to char[16] + snprintf.
QUICK WINS: removed the duplicate DRAGON_MAXSCRIPTSIZE #define; pinned the
dead HUSH3-branch NOTARISATION_SCAN_LIMIT_BLOCKS to 1440; fixed init typos
(fRequestShutdown, RPC warmup).
DEDUP: extracted the 19-line try/catch error-mapping block — copy-pasted
identically into all six async operations — into
AsyncRPCOperation::set_error_from_current_exception(), so the mapping is
edited in one place. Behavior-identical (verified all six blocks were byte-
identical first).
Deferred (endorsed by the scoping, better as their own PRs): addrman Select_
dedup, the pow.cpp powLimit helper (consensus file), the miner CreateNewBlock
lock-asymmetry, the wallet monolith splits, and the ~180-global / consensus-
retarget / Komodo-heritage work.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fifth phase of the code-hygiene remediation, done per-file (18 files) via an
18-agent Workflow with strict guardrails. No consensus or security VALUES
changed (verified: pow/hush_utils/hush_bitcoind/net.h/crypter/stratum have
zero non-comment numeric changes); built clean, self-mined, verifychain=true.
Totals: 57 stale comments corrected, 46 user-facing branding fixes, 6
behavior-preserving named constants, 4 stratum warnings, 77 items deliberately
left (consensus/security values, copyright headers).
- Stale comments -> DragonX: corrected HUSH3/HUSH/Komodo/Equihash/Arrakis
leftovers that misdescribed the active chain (pow AWT/lwma notes, txdb
"Equihash solution" -> RandomX, wallet CLTV/overwinter/Sapling@1 notes,
util datadir provenance, crypter's stale mapSproutSpendingKeys invariant).
Replaced unprofessional/editorializing comments (profane TODOs, "developers
are elite") with neutral technical notes, preserving the real design info.
- User-facing branding: dumpwallet header "created by Hush" -> "DragonX";
RPC help text hushd/hush-cli/hushprivkey/hushaddress/Agama -> dragonx*;
rebranded provably-dead "HUSH3" symbol-fallback literals to DRAGONX.
- Named constants (same values, non-consensus/non-security): addrman
peer-selection factors (kChanceFactorGrowth/kChanceScale/deprioritize),
sietch MIN_ZOUTS, an init notarization-DB cache size.
- Left the security macros (ECC/TFM_TIMING_RESISTANT=420, comment-clarified
only), the consensus subsidy/commission literals + 128/129 TRANSITION
(comment only), and copyright headers untouched.
- Added a prominent WARNING that the stratum server is Equihash-era and NOT
updated for DragonX's RandomX PoW (flagged, not rewritten).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fourth phase of the code-hygiene remediation, addressing the ~1,082 stray
fprintf(stderr)/printf debug calls so consensus and hot paths stop spewing
to stderr/stdout. Done per-file (18 files) with conservative rules; the tree
builds clean, self-mines, and `verifychain` re-validates the whole chain
(pow/txdb/coins/miner paths) with -debug=1 enabling every converted line —
zero tinyformat/format-arg exceptions.
Net across 18 files: 174 commented-out debug lines deleted, 173 unconditional
live prints converted to LogPrint("<cat>",...)/LogPrintf (net/mining/pow/nspv/
zrpc categories, format strings + args preserved exactly), 40 pure-noise or
sensitive prints deleted, and 194 calls DELIBERATELY LEFT (already behind
fDebug/fZdebug guards, or genuine startup/fatal-error output that must reach
the console before logging init).
Notable:
- Deleted sensitive success-path dumps (nSPV SIG_TXHASH + full tx input/
output/change amounts; kvupdate privkey/pubkey hex) that were writing key
and amount material straight to stderr/stdout.
- Removed the raw 32-byte target hex dumps in the zawy adaptive-PoW helpers
and the legacy one-shot `if(height==340000)` HUSH artifact in pow.cpp.
- Converted per-tx relay + ban/banlist (net), per-setgenerate MININGTHREADS
(rpc/mining), signrawtransaction TXPOW, and per-message nSPV traces.
- Left format/arg-mismatched lines untouched (flagged) to avoid introducing
tinyformat runtime throws.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Third phase of the code-hygiene remediation: remove provably-dead code.
Each in-file deletion was adversarially verified (a per-finding investigator
plus a skeptic that greps the tree to refute deletability); the sanity grep
confirms zero dangling references, and the tree builds and self-mines clean.
Removed wholesale:
- src/cc/dapps/ (9 files, ~6,939 lines): a git-tracked but never-built Komodo
DEX / z-migration tool (hushdex.c, zmigrate.c, cJSON.c, ...). Nothing in
Makefile.am/configure.ac references it.
In-file dead code:
- pow.cpp: the `#ifdef original_algo` oldRT_CST_RST function (the macro is
never defined in source or build flags) and two `if ( 0 )` debug blocks.
- walletdb.cpp: the "orphaned staking transaction" cleanup path (deadTxns) —
DragonX is RandomX PoW with no staking, so the guard is always false and the
block never runs; also drop the now-unused static/extern decls.
- cc/eval.h: the ProcessCC / Eval::ImportCoin / ImportPayout / DisputePayout
declarations that have no definition anywhere in the tree.
- rpc/crosschain.cpp: the crosschainproof stub RPC (returned {} unconditionally)
and its registrations in rpc/server.cpp, rpc/server.h, rpc/client.cpp.
- coins.cpp: the commented-out `//TODO: delete` Sprout PushAnchor template.
- wallet.cpp: the permanently-zero KMD `interest2` term in CreateTransaction.
- rpc/net.cpp: hush_longestchain's always-zero `n` var (num > (n>>1) => num>0)
and its `if ( 0 )` debug branch.
- hush_nSPV.h / hush_nSPV_fullnode.h: three `if ( 0 && ... )` dead debug branches.
- net.cpp, crypter.h, saplingconsolidation.cpp (dup set_error_code),
shieldcoinbase.cpp (`donation < 0` on a uint8_t), cclib.cpp (unused
FAUCET2SIZE): single-line dead-code fixes.
Deliberately NOT touched (verification refuted the audit's "dead on DragonX"
premise): the ~2,250-line HUSH3 checkpoint block, the miner notary/timelock
paths, and hush_gateway.h — all reachable at runtime via -ac_name / -ac_*
args (the inherited Komodo assetchain model), and hush_gateway's
hush_opretvalidate is called from ConnectBlock (consensus). Dropping those
requires a deliberate decision to remove HAC/HUSH3 mode, tracked separately.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Second phase of the code-hygiene remediation, covering the items the
DragonX team introduced or the rebrand missed. No consensus behavior
changes; validated on a local self-mining node.
Log cruft:
- chainparams: route the startup ">>>>>>>>>>" banner and the port line
through LogPrintf/LogPrint("net") instead of fprintf(stderr).
- pow: drop the fprintf(stderr) hash-mismatch dump that duplicated the
LogPrintf copy verbatim.
- miner: gate the RandomXDatasetManager per-alloc/per-VM address dumps and
MemDiag /proc reads behind LogPrint("randomx"); keep a one-line
"allocated shared dataset (N GB)" summary at default verbosity.
Named constants (single source of truth in wallet.h):
- DEFAULT_AUTOSHIELD_FEE/INTERVAL, MIN_AUTOSHIELD_INTERVAL,
DEFAULT_AUTOSHIELD_MIN_UTXOS, AUTOSHIELD_MIN/MAX_FEE for the autoshield
option parsing and help text (were bare 10000/25/5 literals repeated
across init.cpp and wallet.h).
- AUTO_OP_TARGET_HEIGHT_OFFSET replaces the three copy-pasted
`blockHeight + 5` async-op scheduling offsets.
- AUTO_OP_EXPIRY_DELTA replaces the three per-file *_EXPIRY_DELTA=15
constants (sweep/consolidation/autoshield) with one shared value.
- Move DEFAULT_AUTOSHIELD_FEE out of the op header into wallet.h so it no
longer collides when both headers are included.
Error surfacing:
- init: report clamped/out-of-range -autoshieldinterval/-autoshieldfee via
InitWarning() (surfaces to GUI/log) instead of fprintf(stderr).
Rebrand / dead foreign-chain code (approved removals):
- server: drop the HUSH3 special-cases in stop() and HelpExampleCli; the
cli example now shows "dragonx-cli" instead of "hush-cli".
- getinfo (misc): remove the stale dPoW notarization block (notarized,
prevMoMheight, notarizedhash, notarizedtxid, notarizedtxid_height,
HUSHnotarized_height, notarized_confirms) — DragonX is a private chain
from genesis with no active dPoW. Also fixes the hardcoded "HUSH3" that
made getinfo query a foreign chain's notarization.
- delete the dead Komodo notary RPCs getera/getdragonjson/
getnotarysendmany/geterablockheights (getera returned 0;
getnotarysendmany was marked "this is broke") and their registrations.
- remove the dead ASSETCHAINS_EQUIHASH reporting branches in getinfo and
getmininginfo — DragonX is RandomX-only.
chainparams: document why the upstream Equihash params and the literal
Bitcoin genesis are retained under RandomX (do not "fix" them).
Deferred: the hdSeedOrigin int->string switch in z_autoshieldstatus
(no shared helper exists to reuse; low value).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Six small defects, all found by an audit of the automated-operation path and all
verified present before changing anything.
AsyncRPCQueue::addOperation returned void and silently dropped the operation when
the queue was closed or finishing. Every caller assumed success: the three
schedulers left their running flag set with nothing in flight (which then blocks
every later round), and z_sendmany / z_shieldcoinbase / z_mergetoaddress returned
an opid for work that would never run -- z_shieldcoinbase and z_mergetoaddress
having already locked their selected coins in the constructor. It now returns
bool; the schedulers release the flag and log, and the three RPCs raise an error
instead of handing back an opid. Coin locks are memory-only, so a refusal at
shutdown reclaims them with the process; the lie about success was the defect.
Nothing ever removed finished automated operations from the queue's map.
popOperationForId is reached only from z_getoperationresult, so on a node running
autoshield every 25 blocks the map grew by one entry per round forever. The
schedulers now pop the operation they just cancelled. The worker already handles
a missing id ("cannot find operation in map, may have been removed",
asyncrpcqueue.cpp), and it releases lock_ before calling main(), so popping under
cs_wallet introduces no lock cycle.
The autoshield operation built its transaction against targetHeight_, the
enqueue-time height, while SetExpiryHeight and the network-upgrade straddle guard
both used tipHeight. Since the builder's height selects the consensus branch id,
the guard was checking a height the transaction was not signed against -- it could
not prevent the failure it exists to prevent. Now tipHeight throughout.
cancel() in the sweep, consolidation and autoshield operations set CANCELLED
unconditionally, dropping the base class's guard entirely. The schedulers cancel
the previous operation when they enqueue the next, so a round that had already
SUCCEEDED got its result relabelled as cancelled. Restored a narrower guard: still
cancellable while READY or EXECUTING (the base class refuses the latter, which
would defeat cancellation here), but a terminal state is left alone.
CommitAutomatedTx dumped the whole transaction to stderr on every commit,
duplicating the LogPrintf that CommitTransaction does one call later. ToString()
emits a line per input, so with the 400-input autoshield cap that was tens of KB
of stderr per round. Removed.
Also corrected a comment that credited the immature-coinbase exclusion to
fOnlySpendable (the argument is fOnlyConfirmed; the exclusion is unconditional in
AvailableCoins), and noted that AUTOSHIELD_CTXIN_P2SH_SIZE is a byte size that
merely happens to share the value 400 with the input cap.
Verified on an isolated regtest chain, 8/8: nine consecutive autoshield rounds
succeed after the builder-height change; the operation map stays at 1 entry across
all nine (it grew one per round before); stderr totals 1608 bytes for the whole
run with zero CommitAutomatedTx dumps, while debug.log still records all nine
commits via CommitTransaction; no round is relabelled cancelled; funds shield
correctly and no coin locks leak.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A successful-but-incomplete sweep round deliberately returns with fSweepRunning
still set and nextSweep unadvanced, as a "keep draining next block" baton. Every
early return in RunSaplingSweep, though, leaves that baton set without
re-dispatching -- and RunSaplingConsolidation, which is gated on fSweepRunning,
then returns without advancing nextConsolidation. So the "consolidation is
within 5 blocks" blackout at the top of RunSaplingSweep never lifts: sweep waits
on consolidation, consolidation waits on sweep, and neither runs again.
That much is pre-existing. What is new is that autoshield now shares the gate --
RunAutoShieldCoinbase returns early on fSweepRunning || fConsolidationRunning --
so a wedged sweep silently disables coinbase shielding too, with
z_autoshieldstatus reporting autoshield true, running false, and no reason.
Only honour the baton while a sweep operation is genuinely in flight: if the
operation for saplingSweepOperationId is absent or has reached a terminal state,
drop the stale flag and let the checks below decide afresh. The drain model is
unchanged -- nextSweep is still unadvanced, so the next block re-dispatches.
Also report the deferral in z_autoshieldstatus, so mutual exclusion with sweep
or consolidation reads as a deferral rather than an unexplained idle.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
New wallets now get an exportable 24-word phrase instead of a random seed that
no phrase can ever reproduce. Only wallets with no seed yet are affected;
GenerateNewSeed is reachable from one place, under !HaveHDSeed(), and all three
key stores refuse to replace an existing seed.
This is safe to default on now that the storage form is backwards compatible: the
expanded 64-byte BIP39 seed is what gets stored, so a binary predating any of
this reads it and derives the same keys.
Verified on an isolated chain before flipping:
- a new-format wallet reopened with the tagged v1.1.0 binary, which has no
knowledge of the entropy record, listed identical addresses;
- restoring only the 24 words into a fresh datadir recovered every address.
Note this changes what a new wallet is, not what an existing one is: the same
entropy yields a different key tree depending on which side of this commit
created the wallet. Nothing migrates, and nothing needs to.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mnemonic wallets stored the 32-byte BIP39 entropy as the HD seed and relied on
CHDChain.fMnemonicSeed to tell the deriver to expand it first. That flag is
version-gated in the CHDChain serialisation, so a binary that predates it reads
the record, never consumes the trailing byte, and derives from the raw entropy --
a different key tree, silently, with no error.
Store the expanded 64-byte BIP39 seed instead, with fMnemonic = false, and keep
the entropy in its own display-only record. Derivation then reads the stored
bytes directly on every binary, old or new, so key trees are identical and no
CHDChain version bump or minversion fence is needed. The wallet format stays
readable by earlier releases rather than becoming one-way.
Addresses are unchanged: the previous format expanded the entropy on every read
and fed the same 64 bytes to Master(). This is also the form the tree already
round-trips through -- z_exportwallet dumps the expanded seed, and restoring that
hex via -hdseed installs it with fMnemonic = false.
Write order is load-bearing: seed first, entropy second. A crash between them
leaves a wallet with a seed and no phrase, which is merely inconvenient. The
reverse would leave an entropy record with no seed, and the next start would mint
a different seed while the wallet still held a phrase for the old one.
-usemnemonic still defaults to false; only explicit opt-in and -mnemonic restores
take this path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Wires the key store secret to the database records: load and store paths on
CWallet, the two ReadKeyValue arms, and the export path.
GetMnemonicPhrase now prefers the entropy record and verifies it before printing:
a phrase is only returned if expanding it reproduces the seed derivation actually
uses. It falls back to the existing fMnemonicSeed path, so wallets that store the
entropy AS the seed keep working unchanged.
IsMnemonicSeed() now means "a phrase is available" rather than "the seed is the
entropy", which is what every caller actually wants.
Still a no-op on every existing wallet: nothing creates an entropy record yet, so
GetMnemonicEntropy returns false and the old code path is taken.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CWalletDB::WriteCryptedHDSeed wrote the "chdseed" record and left "hdseed" in
place, unlike WriteCryptedKey which erases "key"/"wkey" after writing "ckey".
No erase of "hdseed" existed anywhere in src/wallet/.
CDB::Rewrite does not save us: EncryptWallet calls it with pszSkip defaulted, so
it copies every surviving record verbatim into the new file. The result is that a
wallet created unencrypted and later encrypted keeps its raw HD seed in cleartext
on disk permanently, and reloads it into memory on every start.
Add CWalletDB::EraseHDSeed and call it from CWallet::SetCryptedHDSeed after the
encrypted record is written, through the same CWalletDB so it shares
EncryptWallet's transaction. Erase returns true on DB_NOTFOUND, so a wallet that
was never written in plaintext is unaffected.
The erase is deliberately best-effort and only logs on failure. A hard failure
here propagates into CCryptoKeyStore::EncryptKeys, which EncryptWallet turns into
assert(false) with half the keys encrypted in memory; a warning is strictly
better than that.
Note this path is only reachable with -developerencryptwallet, which is
experimental and off by default on this chain, so this is a latent fix rather
than a live one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Autoshield sends mined coinbase to a seed-derived z-address, so the records that
decide how the seed derives keys become fund-safety critical. Three of them were
not treated that way.
InstallHDSeed wrote the seed record before the chain record, non-transactionally.
A crash between the two left a wallet holding a seed with no hdchain: on the next
load hdChain silently reverts to defaults, clearing fMnemonicSeed -- which
switches the derivation input from the expanded BIP39 seed to the raw entropy --
and resetting saplingAccountCounter. Write the chain first; the opposite torn
state is harmless and self-heals, because HaveHDSeed() is then false and init
installs again.
The hdchain record was read as a bare deserialise inside a catch-all with strErr
never set, and it is not a key type, so a corrupt record was downgraded to a
non-critical error and the node booted into the wrong key tree. Report it, track
whether it was read, and refuse to load a wallet that holds a seed but no
readable hdchain. Also preserve hdchain through a keys-only salvage, which would
otherwise drop it and produce exactly the state we now refuse.
GenerateNewSeed silently fell back to a random seed when BIP39 generation failed,
producing a wallet that looks mnemonic-capable but whose words can never be
exported and which no seed phrase can restore. A user who asked for -usemnemonic
now gets that or a hard failure.
Finally, record how the seed came to exist -- created on an empty wallet,
restored from -mnemonic/-hdseed, retrofitted onto a pre-existing seedless wallet,
or predating this record. A retrofitted seed is in no backup the user already
holds, so a feature that moves funds into addresses only that seed can re-derive
must not enable itself there by default. Nothing consumes this yet.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The zaddr-sweep op cleared fSweepRunning/nextSweep only on the sweepComplete
success path (inside main_impl), so a cancelled or throwing sweep left
fSweepRunning stuck true. Since fSweepRunning now also gates consolidation and
the default-on autoshield, a persistently failing sweep (e.g. a corrupt-witness
note) would wedge all three background ops for the session.
Move the scheduler bookkeeping into main() so it runs on every terminal state
(success/failure/exception/cancel), guarded by op id. Preserve the intended
"keep draining every block until swept" model: on a successful-but-incomplete
round the flag stays set and nextSweep is not advanced; on completion OR on
failure/exception the flag is released and nextSweep backs off one interval, so
a failing sweep no longer retries every block or wedges the other ops.
Also fix RunSaplingSweep to take cs_wallet itself (was AssertLockHeld, a no-op
in release builds) since ChainTip does not hold it there and the driver mutates
scheduler state + enqueues -- matching RunSaplingConsolidation and
RunAutoShieldCoinbase.
sweepComplete is recorded via a new member; saplingSweepOperationId made public.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Sapling auto-consolidation scheduler never advanced nextConsolidation after
init and never set fConsolidationRunning, so once the tip passed the init
threshold `-consolidation` dispatched a consolidation op every block instead of
once per -consolidationinterval, and every guard that reads fConsolidationRunning
(in RunSaplingSweep, and in the new autoshield driver) was dead.
Mirror the intended scheduler model:
- RunSaplingConsolidation sets fConsolidationRunning=true before dispatch and
self-guards with `if (fConsolidationRunning) return;`.
- The consolidation op advances nextConsolidation = consolidationInterval +
tipHeight and clears fConsolidationRunning on every terminal state
(success/failure/exception/cancel), guarded by op id so only the current op
mutates scheduler state.
- saplingConsolidationOperationId moved to public so the op can read it.
Restores the documented once-per-interval cadence and makes the
sweep/consolidation/autoshield mutual-exclusion guards effective.
Note: the sweep op has the same latent wedge (fSweepRunning/nextSweep are
cleared only on the sweepComplete success path, so a cancelled or throwing
sweep leaves fSweepRunning stuck true) - left for a follow-up; this commit is
the template to port.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On this ac_private=1 chain miners accumulate one transparent coinbase UTXO per
block (the only transparent output the chain permits). It had to be shielded
manually via z_shieldcoinbase, and left unshielded it is the sole persistent
metadata leak on the chain and the source of miner UTXO-fragmentation send
failures.
Add AsyncRPCOperation_autoshieldcoinbase: a periodic, default-on wallet op
driven from CWallet::ChainTip alongside sweep/consolidation, draining matured
coinbase into a wallet-owned Sapling z-address in size-bounded batches.
- Enqueue-only driver (RunAutoShieldCoinbase): takes only cs_wallet in the
notify context; all gathering runs on the async worker under LOCK2(cs_main,
cs_wallet).
- Dedicated op (not a reuse of z_shieldcoinbase) so it never toggles mining.
- Default-ON but conditional: silent no-op on -disablewallet, external
-mineraddress, non-mining, or locked wallets (explicit IsLocked() guard).
- Destination is reuse-then-create; -autoshieldaddress override is
spend-key-validated at init so funds cannot be stranded.
- Sweep-model bookkeeping: advances nextAutoShield and clears the running flag
on every terminal state; an op-id guard stops a stale op clobbering scheduler
state; a self-guard stops cancel/re-enqueue churn.
- Sietch-padded output shape matches manual z_shieldcoinbase txns.
Config: -autoshield (default true), -autoshieldinterval, -autoshieldaddress,
-autoshieldfee (range-validated), -autoshieldminutxos.
Incorporates fixes from an 8-angle code review: CAmount fee type with init-time
range validation, op-id-guarded flag bookkeeping, driver self-guard, and a
tipHeight-consistent NU-activation guard.
Note: the fConsolidationRunning / nextConsolidation consolidation-scheduler
wedge is a pre-existing bug, left for a separate change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BuildWitnessCache sizes its blockCms buffer from pindex->GetHeight() but the
Phase-1 loop walks the active chain (chainActive.Next), terminating only on
pbi==pindex. If a reorg moved pindex off the active chain while the notify
thread lagged (cs_main is released between per-block ChainTip calls) and the new
active tip is taller, the loop never reaches pindex and, once past pindex's
height, writes blockCms[h-startHeight] out of bounds -- a heap overflow.
Rebuilding witnesses for an abandoned block is meaningless anyway, so bail early
when pindex is not on the active chain; cs_main is held for the whole function,
so the check cannot race the loop.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Derive transparent (t-addr) keys from the HD seed and add BIP39 mnemonic seed
phrases that are byte-for-byte compatible with SilentDragonXLite, so the same
24 words recover the same shielded and transparent addresses in either wallet.
HD transparent keys:
- Derive t-keys from the seed at m/44'/coin'/0'/0/i (were random CKeys).
- CHDChain gains a version-gated transparent counter; existing wallets load
unchanged. GenerateNewKey routes through DeriveNewChildKey when enabled
(-hdtransparent, default on).
- Restore from a seed hex via -hdseed with gap-limit pre-derivation; birthday
pinned to genesis so the rescan is not clipped.
BIP39 seed phrases:
- Wire the vendored trezor BIP39 lib (src/crypto/bip39) into the build, fix its
BIP39_WORDS guard, and disable the insecure mnemonic cache.
- Match SDXLite exactly: English wordlist, empty passphrase, PBKDF2 64-byte
seed, coin type 141, ZIP-32 m/32'/141'/i' and BIP44 m/44'/141'/0'/0/i. Store
the 32-byte entropy and expand to the 64-byte seed on demand.
- Restore via -mnemonic, create via -usemnemonic, reveal via z_exportmnemonic.
Verified by gtests including a known-answer BIP39 seed vector and z/t address
derivation checks (src/gtest/test_hdtransparent.cpp, test_mnemonic_compat.cpp).
Docs in doc/hd-transparent-keys.md and doc/seed-phrase.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Wallets upgraded across the 1.0.1->1.0.2 network transition could end up
with note witnesses stuck at a stale height, causing z_sendmany /
z_mergetoaddress to fail to build a valid spend. Root cause was a trio of
issues that let a desynced witnessHeight perpetuate instead of self-healing:
- DecrementNoteWitnesses left witnessRootValidated and the witness deque in
an asymmetric state on the size<=1 path.
- VerifyAndSetInitialWitness blindly trusted witnessHeight instead of
validating the cached root against the chain, so a bad height survived.
- UpdatedNoteData copied witnessHeight even when no witnesses were present.
- witnessRootValidated was uninitialized and never serialized, so a garbage
true value could short-circuit the self-heal.
Fixes:
- Default witnessRootValidated to false (in-memory only; never serialized).
- VerifyAndSetInitialWitness now validates the cached witness root against
the block's hashFinalSaplingRoot and reseeds on mismatch.
- Symmetric reset of witness state in DecrementNoteWitnesses.
- Guard the witnessHeight copy in UpdatedNoteData behind a non-empty
witnesses check.
- Defensive majority-root guard in GetSaplingNoteWitnesses.
Also rewrites BuildWitnessCache to rebuild the witness cache in parallel
(per-block commitment extraction + worker pool), cutting a full repair from
~28 min to ~2 min. Tunable via -witnessbuildthreads and -witnessfastrebuild;
output verified byte-identical to the serial path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Update compiler references from gcc-8 to gcc-15 across build system
(build-mac.sh, darwin.mk, Makefile_custom)
- Use system Rust (rustup) instead of bundled Rust 1.32.0 for librustzcash
to fix rlib linker incompatibility on macOS Sequoia
- Replace deprecated std::random_shuffle with std::shuffle (net.cpp,
transaction_builder.cpp, wallet.cpp)
- Fix -std=gnu17 -> -std=gnu++17 for C++ targets (libzcash, libhush)
- Fix nodiscard warning in glibcxx_sanity.cpp
- Replace deprecated OSMemoryBarrier with std::atomic_thread_fence in LevelDB
- Add -Wno-error=deprecated-declarations to CXXFLAGS for third-party headers
- Fix REMAINING_ARGS unbound variable in build.sh
- Add --disable-tests handling to build-mac.sh
- Update README with correct macOS build dependencies and instructions
By taking a wallet lock first and then main later we run into
a potential deadlock :
2024-09-13 11:14:37 POTENTIAL DEADLOCK DETECTED
2024-09-13 11:14:37 Previous lock order was:
2024-09-13 11:14:37 (1) cs_wallet wallet/wallet.cpp:985
2024-09-13 11:14:37 (2) cs_main wallet/wallet.cpp:890
2024-09-13 11:14:37 Current lock order is:
2024-09-13 11:14:37 (2) cs_main wallet/wallet.cpp:2845
2024-09-13 11:14:37 (1) cs_wallet wallet/wallet.cpp:2845
We do not seem to need this lock for the entire DecrementNoteWitnesses function,
we need it only when calling GetSaplingSpendDepth. Also protects against
the case in the future where some code without cs_main calls GetSaplingSpendDepth.
Another possibly better approach would be to take a lock inside
GetSaplingSpendDepth since we want the lock to apply to as little
code as possible and it is GetSaplingSpendDepth that calls GetDepthInMainChain
which requires a cs_main lock .
DanSandClaude Opus 5.5