From bf3c33c53ad610a8a032ac36ae05a6c597167382 Mon Sep 17 00:00:00 2001 From: DanS Date: Tue, 14 Jul 2026 20:24:45 -0500 Subject: [PATCH] revert(net): remove header-accept RandomX check; keep nMinimumChainWork + #8 An adversarial re-review found the header-accept RandomX check (b9fdc7981 + 7e9b2c661 header-PoW + d2124a303 defer) to be a persistent source of consensus-liveness bugs: it derives the RandomX key from the ACTIVE chain (hush_chainactive), the wrong branch for reorg/side-branch/catch-up headers, so it repeatedly false-rejected validly-mined headers and DoS(100)-hard-banned honest peers (IBD-tail catch-up and deep-reorg cases); the defer fix and an extend-tip fix each addressed one case while leaving/creating others (an extend-tip variant re-opened an unbounded post-IBD side-branch flood). It only mitigated a low-harm resource DoS -- forged headers bloat mapBlockIndex memory/ disk but are never SELECTED (nMinimumChainWork) and the full RandomX + target check still runs at block-connect. Revert to fCheckPOW=0 at header-accept (original behavior). A comment in AcceptBlockHeader records that any re-attempt must derive the key from the header's OWN ancestry (pindexPrev->GetAncestor), never the active chain. Also hardens two issues the same review found: - #8 IBD header cap now bounds against the VALIDATED chainActive.Height() (attacker-hard) instead of pindexBestHeader, which a forward-extending flood advanced in lockstep, defeating the cap. - opreturn_burn only emits a change output above the dust threshold; a sub-dust change made the returned tx non-standard/unrelayable. Co-Authored-By: Claude Opus 4.8 --- src/main.cpp | 41 +++++++++++++++++----------------------- src/pow.cpp | 16 ++-------------- src/wallet/rpcwallet.cpp | 7 +++++-- 3 files changed, 24 insertions(+), 40 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index e6d45bc88..1375ab064 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -5457,27 +5457,15 @@ bool AcceptBlockHeader(int32_t *futureblockp,const CBlockHeader& block, CValidat } return true; } - // SECURITY (header-flood DoS): once synced, verify PoW at header-accept time so a peer cannot - // flood unbounded PoW-less headers into mapBlockIndex (they now fail RandomX -> DoS-ban). During - // IBD keep fCheckPOW=0 for fast header sync; the full RandomX/target check runs at block connect. - // Resolve the parent up-front so CheckBlockHeader receives THIS header's own (still-NULL) pindex - // and its CORRECT height (parent height + 1) — never the caller's reused *ppindex, which in a - // HEADERS batch aliases the PREVIOUS header. Passing *ppindex here made (a) the height a stale - // value (0 for a fresh header, or the prior header's height when aliased) so post-IBD - // RandomXValidationRequired() saw a below-activation height and CheckRandomXSolution returned true - // WITHOUT verifying, and (b) the (pindex && pindex->fRandomXVerified) short-circuit fire on an - // as-yet-unverified header — both silently defeating the post-IBD header-flood PoW gate. The - // authoritative parent validation (prev-not-found / prev-invalid) still runs unchanged below; this - // lookup is read-only and under cs_main, so it cannot disagree with it. IBD stays fast: fCheckPOW - // is still 0 during IBD, so no RandomX is computed here regardless of the height. - CBlockIndex* pindexPrevForHeight = NULL; - { - BlockMap::iterator miPrev = mapBlockIndex.find(block.hashPrevBlock); - if (miPrev != mapBlockIndex.end()) - pindexPrevForHeight = miPrev->second; - } - int32_t nHeaderHeight = (pindexPrevForHeight != NULL) ? pindexPrevForHeight->GetHeight() + 1 : 0; - if (!CheckBlockHeader(futureblockp,nHeaderHeight,pindex, block, state, IsInitialBlockDownload() ? 0 : 1)) { + // Header-accept does NOT verify RandomX PoW (fCheckPOW=0). The RandomX key for a header is derived + // from the block at keyHeight on the header's OWN branch, which is not reliably resolvable at + // header-accept time (reorg / side-branch / catch-up headers are not on the active chain), so a + // header-time RandomX check repeatedly false-rejected valid reorg headers and hard-banned honest + // peers (see audit notes; reverted b9fdc7981/7e9b2c661/defer). The full RandomX + target check runs + // at block-connect with the correct branch key. Fake low-work chains are gated from SELECTION by + // nMinimumChainWork; the per-peer IBD header cap bounds flood memory. Do NOT re-enable a header-time + // RandomX check without first deriving the key from the header's own ancestry (pindexPrev->GetAncestor). + if (!CheckBlockHeader(futureblockp,*ppindex!=0?(*ppindex)->GetHeight():0,*ppindex, block, state, 0)) { if ( *futureblockp == 0 ) { LogPrintf("%s: CheckBlockHeader futureblock=0\n", __func__); return false; @@ -7895,13 +7883,18 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, // store while in IBD. Honest headers-first sync needs at most ~chain-length headers from a // peer; one that floods far past 2x the known chain length is only trying to bloat // mapBlockIndex/leveldb (such headers are never selected -- nMinimumChainWork gates that -- - // but they still cost memory/disk). Cap per-peer and drop the peer. IBD-only: post-IBD the - // RandomX check in AcceptBlockHeader already makes forged headers fail RandomX and ban. + // but they still cost memory/disk). Cap per-peer and drop the peer. IBD-only: post-IBD there is + // no header-accept PoW check (removed as bug-prone), so a post-IBD flood is bounded only by + // nMinimumChainWork gating selection -- memory/disk growth there is accepted as low-severity. if (IsInitialBlockDownload()) { CNodeState *hstate = State(pfrom->GetId()); if (hstate != NULL) { hstate->nHeadersProcessed += (int64_t)nCount; - int knownH = std::max(pindexBestHeader ? (int)pindexBestHeader->GetHeight() : 0, + // Cap RELATIVE TO THE VALIDATED ACTIVE-CHAIN HEIGHT (attacker-hard -- advancing it requires + // connecting real PoW blocks), NOT pindexBestHeader: a forward-extending header flood advances + // pindexBestHeader in lockstep with the attacker, so a pindexBestHeader-relative cap never fires. + // The checkpoint height is a fixed floor so honest IBD (blocks still lagging headers) is never capped. + int knownH = std::max((int)chainActive.Height(), Checkpoints::GetTotalBlocksEstimate(chainparams.Checkpoints())); int64_t headerCap = 2 * (int64_t)knownH + 200000; if (hstate->nHeadersProcessed > headerCap) { diff --git a/src/pow.cpp b/src/pow.cpp index 1996307dd..1daf60e61 100644 --- a/src/pow.cpp +++ b/src/pow.cpp @@ -804,20 +804,8 @@ bool CheckRandomXSolution(const CBlockHeader *pblock, int32_t height) // Derive the key (shared helper) and serialize the input (identical bytes to the pool path). std::string rxKey = GetRandomXKey(height); - if (rxKey.empty()) { - // The RandomX key block (keyHeight = ((height-lag)/interval)*interval, looked up on the - // ACTIVE chain) is not yet connected. This happens ONLY at header-accept when headers run - // ahead of our connected block tip (the IBD tail / catch-up) -- block-connect always has it, - // since blocks connect in order and keyHeight <= height-lag < the connected tip. The header - // is NOT invalid; we simply cannot verify it YET. Defer to block-connect (which re-checks - // with the key present) rather than returning an error -- returning an error here makes - // CheckBlockHeader DoS(100)-ban the honest peer that sent a perfectly valid tip header we - // just can't check yet (observed live: a post-reindex node banned the whole fleet and stalled - // ~2000 blocks short of the tip). Flood protection is preserved for synced nodes (key present - // -> real RandomX check) and bounded during catch-up by the per-peer IBD header cap + nMinimumChainWork. - LogPrint("net", "CheckRandomXSolution: RandomX key block for height %d not yet connected; deferring verification to block-connect\n", height); - return true; - } + if (rxKey.empty()) + return error("CheckRandomXSolution(): cannot derive RandomX key for height %d", height); std::vector ssInput = GetRandomXInput(*pblock); char computedHash[RANDOMX_HASH_SIZE]; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 22c9e9860..deb7f3217 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -6437,8 +6437,11 @@ UniValue opreturn_burn(const UniValue& params, bool fHelp, const CPubKey& mypk) // Return the unspent surplus (selected inputs - burn amount - txfee) as change to a // wallet-owned address; without this the entire surplus is silently paid as miner fee. CAmount change = normalInputs - nAmount - txfee; - if ( change > 0 ) - mtx.vout.push_back(CTxOut(change, GetScriptForDestination(myPubkey.GetID()))); + // Only emit change if it clears the dust threshold; a sub-dust output would make the tx + // non-standard (unrelayable). Sub-dust surplus is folded into the fee (standard wallet behavior). + CTxOut changeOut(change, GetScriptForDestination(myPubkey.GetID())); + if ( change > 0 && !changeOut.IsDust(::minRelayTxFee) ) + mtx.vout.push_back(changeOut); ret.push_back(Pair("hex", EncodeHexTx(mtx))); return(ret); }