fa16e740b602ee94de5a194e1be25583f340e968
7
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  DanSandClaude Opus 4.8
|
81f803948c |
hygiene: Phase 6 (partial) — fix 7 verified bugs + quick-wins + dedup
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>
|
||
|
DanSandClaude Opus 4.8
|
aff6101987 |
hygiene: Phase 2 — DragonX-authored cleanup (13 findings)
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>
|
||
|
DanSandClaude Opus 5
|
65130c3120 |
async/wallet: close the loose ends around automated operations
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>
|
||
|
DanSandClaude Opus 5
|
358011bd54 |
wallet: resolve the autoshield destination at startup
pwalletMain->autoShieldAddress was only ever written by a round running in the
current process, so on any node without an explicit -autoshieldaddress,
z_autoshieldstatus reported an empty destination from startup until the first
round fired. disabled_reason was empty on that path too (rpcwallet.cpp), so the
RPC showed autoshield true, running false, no address and no explanation -- the
exact silent state z_autoshieldstatus was added to eliminate. On a restored
wallet, which pre-derives the whole -mnemonicsaplinggap window and therefore
always has an in-gap account to pick, the answer was known at startup and simply
not computed.
Split the read-only half of resolveDestination into a free function shared with
init: the configured override if set, else the lowest in-gap account
m/32'/coin'/i' the wallet already holds. init calls it once when autoshield is
enabled and no explicit address was given.
It deliberately does not generate a key. Deriving a fresh sapling account as a
side effect of populating a status field would mutate the wallet to make an RPC
prettier, so step 3 of resolveDestination -- the generation path, which must stay
inside the operation where an unlocked wallet is already established -- is left
where it was. A brand-new wallet holds nothing in-gap, so the field stays empty
there and disabled_reason now says why instead of being blank.
Behaviour is otherwise unchanged: same derivation, same lowest-index-wins rule,
same refusal to trust CKeyMetadata, same caching for the life of the process.
Verified on an isolated regtest chain, 12/12:
fresh wallet -> address "", reason "no destination resolved yet; one will
be derived from the HD seed on the first round"
after one round -> address set, reason empty
after RESTART -> address visible with NO block mined since (height 12 both
sides), and z_listaddresses still holds exactly 1 address,
so init derived nothing
-autoshieldaddress-> still overrides the derived destination, and the round
shields into it
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
DanSandClaude Opus 5
|
9a8f17b2c8 |
wallet: bound autoshield rounds and lock their inputs
Two defects in a single autoshield round, both of the quiet kind.
The size estimate reserved a flat 2000 bytes for "header + sietch outputs", but
every autoshield tx carries three Sapling OutputDescriptions -- the change note
plus the two Sietch dummies -- and the real fixed cost is ~2937 bytes. Measured
across seven live mainnet coinbase shields: 113.5 bytes per input and 2936.6
+/- 0.8 bytes fixed, of which 3 * 948 = 2844 is the output descriptions. The
estimate was therefore short by ~937 bytes before a single input was counted.
Inputs are charged AUTOSHIELD_CTXIN_DUST_SIZE = 148, which is conservative for
the default P2PK coinbase but exact for P2PKH, so with a P2PKH coinbase a
backlog of 1332..1337 utxos passed the estimate and built a tx over
MAX_TX_SIZE_AFTER_SAPLING. CommitTransaction calls AddToWallet before
AcceptToMemoryPool, so a rejected oversize tx leaves its inputs reading as spent.
z_shieldcoinbase caps a manual shield at SHIELD_COINBASE_DEFAULT_LIMIT = 50
utxos; autoshield dropped that cap and relied on the byte estimate alone.
Restore one -- AUTOSHIELD_MAX_INPUTS = 400 -- so the byte arithmetic is no longer
the only thing between a large backlog and an oversize transaction. The
remainder is shielded on the next round.
Second, the proof build deliberately runs without cs_wallet so wallet RPCs are
not stalled, which leaves a multi-second window in which a concurrent
z_shieldcoinbase or z_sendmany can re-select the same coinbase outputs.
AvailableCoins already honours IsLockedCoin and z_shieldcoinbase already
brackets its selection with LockCoin/UnlockCoin; autoshield made zero LockCoin
calls. Take the locks under cs_wallet at selection time and release them via
RAII, since several early returns sit between selection and commit and a leaked
lock would exclude those coins from every future round.
Verified on an isolated regtest chain with a 540-utxo backlog:
round 1 logged "reached per-round input cap (400)" and committed exactly 400
inputs in a 48351-byte tx (estimate 62300, limit 200000)
round 2 took the remaining 151; backlog drained 540 -> 0
listlockunspent showed 400 coins locked mid-round and 0 afterwards
48351 bytes for 400 inputs implies 2937 bytes of fixed overhead, agreeing
with the mainnet measurement to 14 bytes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
DanSandClaude Opus 5
|
dad162a89a |
wallet: pick the autoshield destination by seed re-derivation, in-gap only
resolveDestination() took the first spendable address in std::set order, which orders on the raw Sapling diversifier (zcash/Address.hpp:95-98) -- uncorrelated with anything the operator can see, and unstable across restarts as addresses are added. Worse, GetSaplingPaymentAddresses() also returns z_importkey/z_importwallet addresses, and CKeyMetadata is NOT evidence of provenance: both hdKeypath and seedFp are copied verbatim out of the import source (rpcdump.cpp:511-516 -> wallet.cpp:5440-5441) with no verification. A crafted import can therefore claim this wallet's seedFp and keypath m/32'/coin'/0' and capture every shielded mining reward into a key no seed restore can reproduce. Filtering on metadata would not have caught that. Derive instead. A bare -mnemonic/-hdseed restore pre-derives exactly -mnemonicsaplinggap sapling accounts from index 0 with saplingAccountCounter reset (init.cpp:2349-2355), so the only self-recoverable destinations are the default addresses of m/32'/<coin>'/i' for i below the gap. Walk that window from the seed and take the lowest index the wallet holds a spending key for. Deriving is the only authoritative test and cannot be spoofed. When nothing in the window is held yet, derive the next account -- but only if it will land inside the window. GenerateNewSaplingZKey does not derive at saplingAccountCounter: its do/while skips indices already held (wallet.cpp:150-157), so a bare counter-below-gap test is unsound. Predict the lowest free index at or above the counter and post-verify the returned address. If no free account remains below the gap, refuse the round and leave the coinbase transparent -- transparent funds are still recoverable through the 1000-key transparent gap, an unfindable note is not. Refusing is safe: main_impl turns a false return into a clean skip, and main() always advances nextAutoShield and clears fAutoShieldRunning, so a refused round cannot latch the feature off. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
DanSandClaude Opus 4.8
|
2ebbbc777c |
wallet: add default-on auto-shield-coinbase; drain miner coinbase into a z-addr
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> |