From 3ec75952d9142bb5aa58307fc5a07db264ecdb24 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Sun, 23 Aug 2026 11:44:16 +0000 Subject: payout: require a confirmation before reading a pre-upgrade comment as payment FindRecordedPoolPayout's legacy branch matched any wallet transaction carrying this height's payout comment and an output of exactly this amount to this address, and never asked whether that transaction was in the chain. A pre-upgrade payout that never confirmed is evidence of NON-payment, and it was being read as payment: the recipient is dropped from the in-flight list, a txid is recorded against the ledger entry, and the debt retires having never been settled. The miner is not paid and nothing on disk says they were owed. That is not hypothetical here. This tree has had a 280-block reorg (see the note in FlushMaturePayouts). A payout invalidated by one is still sitting in mapWallet with its comment intact, and the first boot after this upgrade -- the boot where the binary is swapped after an unclean shutdown with payments in flight -- is precisely when the legacy probe runs against it. The structured branch is deliberately left alone. This process sent those transactions moments earlier, and a send that failed leaves nothing behind: SendMoney calls RollbackTransactionSpent, which calls EraseFromWallet. Depth there would only reject payments that really were made. The check cannot be done inside the wallet scan. GetDepthInMainChain() reads mapBlockIndex and pindexBest, which belong to cs_main, and cs_main is taken before cs_mapWallet everywhere else in this tree -- CreateTransaction, ConnectBlock -- so testing it under cs_mapWallet would invert the lock order in the payout thread. Comment matches are collected in the scan and adjudicated afterwards under cs_main then cs_mapWallet, in that order. That also fixes a second thing: the old code stopped at the first comment match, so one unconfirmed transaction could hide a confirmed one behind it. A rejection logs the txid, height, amount and depth, and says the recipient will be paid, because this is the one decision where being wrong is unrecoverable in the direction that costs a miner money. Property: a payment is only ever inferred from a pre-upgrade comment when the transaction carrying it is in the main chain. --- src/rpc.cpp | 164 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 155 insertions(+), 9 deletions(-) (limited to 'src/rpc.cpp') diff --git a/src/rpc.cpp b/src/rpc.cpp index 06de7f2..b0436ba 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -1339,7 +1339,16 @@ static PayoutProbeResult FindRecordedPoolPayout(const std::string& id, want << OP_DUP << OP_HASH160 << h160 << OP_EQUALVERIFY << OP_CHECKSIG; const std::string comment = PayoutComment(blockHeight); - std::string legacyTxid; + // Candidate legacy matches, adjudicated after cs_mapWallet is released. + // + // The depth check cannot happen inside the scan: GetDepthInMainChain() + // reads mapBlockIndex and pindexBest, which belong to cs_main, and cs_main + // is taken BEFORE cs_mapWallet everywhere else in this tree + // (CreateTransaction, ConnectBlock). Taking it the other way round here + // would be a lock-order inversion in the payout thread. There is at most a + // handful of these: a candidate has to carry this exact height's comment + // and an output of exactly this amount to this address. + std::vector legacyCandidates; // One pass, not two: this scan runs under cs_mapWallet, which the whole // node contends on, and the merge is already paying for it once per record. @@ -1365,27 +1374,63 @@ static PayoutProbeResult FindRecordedPoolPayout(const std::string& id, return PAYOUT_PROBE_STRUCTURED; } - if (!fHaveScript || !legacyTxid.empty()) + if (!fHaveScript) continue; // The comment is written only by the payout path, so its presence // already identifies the transaction as one of ours. map::const_iterator ci = wtx.mapValue.find("comment"); if (ci == wtx.mapValue.end() || ci->second != comment) continue; - std::string txid = wtx.GetHash().GetHex(); - if (excluded.count(txid)) + if (excluded.count(wtx.GetHash().GetHex())) continue; // already credited to a different debt foreach (const CTxOut& txout, wtx.vout) if (txout.nValue == amount && txout.scriptPubKey == want) { - legacyTxid = txid; + legacyCandidates.push_back(it->first); break; } } } - if (!legacyTxid.empty()) { - txidRet = legacyTxid; - return PAYOUT_PROBE_LEGACY; + // A legacy match must have CONFIRMED. + // + // The structured path does not need this and deliberately does not have + // it: this process sent that transaction moments ago, and a send that + // failed leaves nothing behind, because SendMoney calls + // RollbackTransactionSpent, which calls EraseFromWallet. + // + // The legacy path is the opposite case in every respect. Those + // transactions are days or weeks old, so whether they confirmed is a + // settled fact rather than an open question, and a pre-upgrade payout that + // never confirmed is evidence of NON-payment about to be read as payment. + // This chain has had a 280-block reorg (see FlushMaturePayouts): a payout + // invalidated by it is still sitting in mapWallet with its comment intact, + // and the first boot after this upgrade is exactly when it gets matched. + // The cost of being wrong here is a miner who is never paid and no record + // that they were owed. + CRITICAL_BLOCK(cs_main) + CRITICAL_BLOCK(cs_mapWallet) + { + for (const uint256& hash : legacyCandidates) { + map::const_iterator it = mapWallet.find(hash); + if (it == mapWallet.end()) + continue; // erased under us; it is not evidence of anything + int depth = it->second.GetDepthInMainChain(); + if (depth < 1) { + // Loud: this is the difference between a miner having been + // paid and a miner about to be paid twice, decided on a fact + // that is easy to check and impossible to recover afterwards. + LogPrint("payout", "[payout] wallet tx %s carries the payout" + " comment for height %d and an output of exactly %s to" + " %s, but it is NOT in the main chain (depth %d) --" + " refusing to read it as payment; this recipient will" + " be paid\n", + hash.GetHex().c_str(), blockHeight, + FormatMoney(amount).c_str(), address.c_str(), depth); + continue; + } + txidRet = hash.GetHex(); + return PAYOUT_PROBE_LEGACY; + } } return PAYOUT_PROBE_NONE; } @@ -2997,13 +3042,47 @@ static CScript PayoutTestScriptFor(const std::string& address) return sc; } +// Block index entries fabricated by PayoutTestAddWalletTx, so they can be +// freed and unhooked when the test finishes. gPayoutTestChainTip is the end of +// that chain: blocks are linked onto it rather than each replacing the tip, +// because IsInMainChain() is "pnext is set, or I am the tip". +static std::vector gPayoutTestBlockIndexes; +static CBlockIndex* gPayoutTestChainTip = NULL; + +static void PayoutTestClearBlockIndex() +{ + CRITICAL_BLOCK(cs_main) + { + for (CBlockIndex* p : gPayoutTestBlockIndexes) { + for (std::map::iterator it = mapBlockIndex.begin(); + it != mapBlockIndex.end(); ) { + if (it->second == p) mapBlockIndex.erase(it++); + else ++it; + } + delete p; + } + gPayoutTestBlockIndexes.clear(); + gPayoutTestChainTip = NULL; + pindexBest = NULL; + } +} + // Put a transaction in the wallet map that looks exactly like one this pool // sent. `nonce` only exists to keep the hashes distinct. +// +// fConfirmed decides what GetDepthInMainChain() will answer for it, which is +// what the legacy probe now gates on. A confirmed one is given a block index +// entry of its own: an empty merkle branch hashes to the transaction itself, +// so a block whose merkle root IS that hash accepts it at index 0, and +// pointing pindexBest at that block puts it in the main chain one deep. An +// unconfirmed one keeps hashBlock 0, which is depth 0 -- the shape a payout +// left behind by a reorg has. static std::string PayoutTestAddWalletTx(const std::string& comment, const std::string& address, int64 amount, int nonce, - const std::string& structuredId) + const std::string& structuredId, + bool fConfirmed = true) { CWalletTx wtx; wtx.vin.resize(1); @@ -3020,6 +3099,32 @@ static std::string PayoutTestAddWalletTx(const std::string& comment, wtx.mapValue["pool_payout_amount"] = strprintf("%lld", amount); } uint256 hash = wtx.GetHash(); + if (fConfirmed) { + // A block hash distinct from the transaction's own. + uint256 blockHash = hash; + ((unsigned char*)&blockHash)[0] ^= 0xff; + CRITICAL_BLOCK(cs_main) + { + CBlockIndex* pindex = new CBlockIndex(); + pindex->hashMerkleRoot = hash; + // Linked onto the end of the fabricated chain rather than simply + // becoming the new tip: IsInMainChain() is "pnext is set, or I am + // the tip", so a second confirmed transaction would otherwise take + // the first one out of the chain again. + pindex->pprev = gPayoutTestChainTip; + pindex->nHeight = gPayoutTestChainTip ? gPayoutTestChainTip->nHeight + 1 : 1; + if (gPayoutTestChainTip) + gPayoutTestChainTip->pnext = pindex; + gPayoutTestChainTip = pindex; + std::map::iterator mi = + mapBlockIndex.insert(std::make_pair(blockHash, pindex)).first; + pindex->phashBlock = &mi->first; + pindexBest = pindex; + gPayoutTestBlockIndexes.push_back(pindex); + } + wtx.hashBlock = blockHash; + wtx.nIndex = 0; + } CRITICAL_BLOCK(cs_mapWallet) mapWallet[hash] = wtx; return hash.GetHex(); @@ -3497,6 +3602,46 @@ int RunPayoutLedgerSelfTest() check(FindRecordedPoolPayout(id, height, addrB, amount, none, found) == PAYOUT_PROBE_STRUCTURED && found == structuredTxid, "structured payout metadata is matched in preference to a comment"); + + // A comment-matching transaction that never made it into the chain is + // evidence of NON-payment. This tree has seen a 280-block reorg; a + // pre-upgrade payout invalidated by one is still in mapWallet with its + // comment intact, and the first boot after the upgrade is exactly when + // it would be matched. + CRITICAL_BLOCK(cs_mapWallet) + mapWallet.clear(); + PayoutTestClearBlockIndex(); + const std::string addrC = PayoutTestAddress(0x33); + PayoutTestAddWalletTx(comment, addrC, amount, 4, "", false); + check(FindRecordedPoolPayout(id, height, addrC, amount, none, found) + == PAYOUT_PROBE_NONE, + "a pre-upgrade payout that never confirmed is not read as" + " payment"); + + // The same transaction, confirmed, is. + CRITICAL_BLOCK(cs_mapWallet) + mapWallet.clear(); + const std::string confirmedTxid = + PayoutTestAddWalletTx(comment, addrC, amount, 4, "", true); + check(FindRecordedPoolPayout(id, height, addrC, amount, none, found) + == PAYOUT_PROBE_LEGACY && found == confirmedTxid, + "the same payout, confirmed, is"); + + // And an unconfirmed one does not shadow a confirmed one that would + // have matched: the probe used to stop at the first comment match. + const std::string alsoConfirmed = + PayoutTestAddWalletTx(comment, addrC, amount, 6, "", true); + PayoutTestAddWalletTx(comment, addrC, amount, 5, "", false); + std::set spentFirst; + spentFirst.insert(confirmedTxid); + check(FindRecordedPoolPayout(id, height, addrC, amount, spentFirst, found) + == PAYOUT_PROBE_LEGACY && found == alsoConfirmed, + "an unconfirmed transaction does not hide a confirmed one behind" + " it"); + + CRITICAL_BLOCK(cs_mapWallet) + mapWallet.clear(); + PayoutTestClearBlockIndex(); } // -- 9. On-disk compatibility with the live production ledger ------------ @@ -3908,6 +4053,7 @@ int RunPayoutLedgerSelfTest() PayoutTestResetState(); CRITICAL_BLOCK(cs_mapWallet) mapWallet.clear(); + PayoutTestClearBlockIndex(); // Out of the scratch directory before removing it, and out of the key maps // too: the keys this test generated are process-local, but leaving them in // the keystore would let a later test think a script was its own. -- cgit v1.2.3