From 7aaaa7a684e33591125963df1eb4da6da2fa6495 Mon Sep 17 00:00:00 2001 From: Bitflash Date: Tue, 4 Aug 2026 15:14:46 -0300 Subject: rpc: build the pool block template the way the solo miner does (#141) Two bugs in RebuildJob(), the operator's Stratum template builder. It is a separate path from BitcoinMiner() and had drifted from it. A single pass over mapTransactions is not enough. That map is keyed by hash and carries no dependency order, so a transaction spending another still-unconfirmed transaction's output -- change from a recent send, usually -- fails ConnectInputs whenever its parent sits later in map order. One pass drops it, and map order does not change between rebuilds, so every later rebuild drops it at the same point. That is deterministic exclusion rather than bad luck: where pool operators mine most blocks, a transaction of that shape can stay unconfirmed for as long as its parent is. Now it retries until a pass adds nothing, which is what the solo builder has always done. And the fees reached nobody. nFees was declared inside the scan loop, so each transaction reset it and the running total was discarded, while the coinbase line passed a hardcoded 0. An operator collected fees into a block and paid none of them out. The block stayed valid, since paying less than allowed always is, so nothing ever complained. Both the diagnosis and the fix are Vic-Nas's, from his fork of this code. He deleted the fork, so this brings the work back rather than letting it go with it. His description of the failure was precise enough that finding the same two defects here took one read of our RebuildJob. --- src/rpc.cpp | 61 ++++++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 48 insertions(+), 13 deletions(-) (limited to 'src/rpc.cpp') diff --git a/src/rpc.cpp b/src/rpc.cpp index 41c0328..d73b09c 100644 --- a/src/rpc.cpp +++ b/src/rpc.cpp @@ -167,29 +167,64 @@ static bool RebuildJob() CBlock block; block.vtx.push_back(txNew); + // Fill the block the same way BitcoinMiner() does, because this is the + // same job: a block that gets mined and submitted for real. + // + // It used to differ in two ways, and both were bugs. + // + // One pass over mapTransactions is not enough. That map is keyed by + // hash, so it has no dependency order, and a transaction spending the + // still-unconfirmed output of another (change from a recent send, most + // commonly) fails ConnectInputs whenever its parent happens to sit + // later in map order. A single pass then drops it -- and because map + // order does not change between rebuilds, every later rebuild drops it + // again at the same point. Deterministic exclusion, not bad luck: if + // pool operators mine most of the blocks, a transaction of that shape + // can stay unconfirmed for as long as its parent does. + // + // Diagnosis and fix both come from Vic-Nas, who found it in his fork + // of this code and described it exactly. The fork has since been + // deleted, so this carries it back rather than losing it. + int64 nFees = 0; { CTxDB txdb("r"); CRITICAL_BLOCK(cs_mapTransactions) { map pool; + vector vfAlreadyAdded(mapTransactions.size()); + bool fFoundSomething = true; unsigned int sz = 0; - for (auto& kv : mapTransactions) { - CTransaction& tx = kv.second; - if (tx.IsCoinBase() || !tx.IsFinal()) continue; - int64 nFees = 0; - map tmp(pool); - if (!tx.ConnectInputs(txdb, tmp, CDiskTxPos(1,1,1), 0, nFees, - false, true, tx.GetMinFee(block.vtx.size() < 100))) - continue; - pool = tmp; - block.vtx.push_back(tx); - sz += ::GetSerializeSize(tx, SER_NETWORK); - if (sz > MAX_SIZE / 2) break; + while (fFoundSomething && sz < MAX_SIZE / 2) + { + fFoundSomething = false; + unsigned int n = 0; + for (map::iterator mi = mapTransactions.begin(); + mi != mapTransactions.end(); ++mi, ++n) + { + if (vfAlreadyAdded[n]) + continue; + CTransaction& tx = (*mi).second; + if (tx.IsCoinBase() || !tx.IsFinal()) continue; + map tmp(pool); + if (!tx.ConnectInputs(txdb, tmp, CDiskTxPos(1,1,1), 0, nFees, + false, true, tx.GetMinFee(block.vtx.size() < 100))) + continue; + swap(pool, tmp); + block.vtx.push_back(tx); + sz += ::GetSerializeSize(tx, SER_NETWORK); + vfAlreadyAdded[n] = true; + fFoundSomething = true; + } } } } - block.vtx[0].vout[0].nValue = block.GetBlockValue(pindexPrev->nHeight + 1, 0); + // And the fees actually go to the coinbase now. nFees was declared + // inside the scan loop, so every transaction reset it and the total was + // thrown away, while this line passed a hardcoded 0. A pool operator + // collected fees into a block and paid none of them to itself. Solo + // mining has always passed nFees here; only this builder did not. + block.vtx[0].vout[0].nValue = block.GetBlockValue(pindexPrev->nHeight + 1, nFees); block.hashPrevBlock = pindexPrev->GetBlockHash(); block.hashMerkleRoot = block.BuildMerkleTree(); block.nTime = max((unsigned int)(pindexPrev->GetMedianTimePast() + 1), -- cgit v1.2.3