merge: !11 fix/block-assembly-size into deploy/all-2026-08-24

4 files changed, 267 insertions, 14 deletions

diff --git a/src/main.cpp b/src/main.cpp
index 6b8e0de..018627d 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -4949,7 +4949,6 @@ bool BitcoinMiner(int nThreadId)
             map<uint256, CTxIndex> mapTestPool;
             vector<char> vfAlreadyAdded(mapTransactions.size());
             bool fFoundSomething = true;
-            unsigned int nBlockSize = 0;
             // Assembly must stop below the consensus cap that CheckBlock()
             // enforces, or we mine a block our own node then rejects.
             //
@@ -4960,9 +4959,30 @@ bool BitcoinMiner(int nThreadId)
             // meant solving a block and throwing it away -- with this pool at
             // ~90% of hashrate, that is the network stalling, not just us.
             //
-            // Half the cap, keeping the original margin: the test runs before
-            // a transaction is added, so the loop can overshoot by one tx.
-            while (fFoundSomething && nBlockSize < MAX_BLOCK_SIZE/2)
+            // The budget test then sat only on this outer while, and the
+            // comment claimed that bounded the overshoot to a single
+            // transaction. It did not. The inner for walks all of
+            // mapTransactions with no test of its own, so one pass could append
+            // the entire mempool: the overshoot was bounded by mempool size,
+            // which is to say not bounded at all. Ask per transaction, before
+            // appending, and the overshoot is zero rather than small.
+            //
+            // Seeded with what the block already weighs -- header, tx count,
+            // coinbase -- because CheckBlock() weighs the whole serialized
+            // block and not just the transactions we chose to add to it.
+            unsigned int nBlockSize = ::GetSerializeSize(*pblock, SER_DISK);
+            // Sigops are a budget too, and CheckBlock() enforces one that no
+            // assembler here counted against. Bytes do not bound validation
+            // cost: every OP_CHECKSIG is an elliptic-curve verification, so a
+            // template well inside the size limit can still be refused for
+            // demanding more work than a block is allowed to. Counted with
+            // CTransaction::GetSigOpCount, the function CheckBlock() itself
+            // sums, so the two cannot disagree about what a transaction costs.
+            //
+            // Seeded from the coinbase because CheckBlock() sums over all of
+            // vtx, coinbase included.
+            unsigned int nBlockSigOps = pblock->vtx[0].GetSigOpCount();
+            while (fFoundSomething && nBlockSize < MAX_BLOCK_SIZE_GEN)
             {
                 fFoundSomething = false;
                 unsigned int n = 0;
@@ -4974,6 +4994,29 @@ bool BitcoinMiner(int nThreadId)
                     if (tx.IsCoinBase() || !tx.IsFinal())
                         continue;
 
+                    // Skip, not break: a transaction too big for the room left
+                    // says nothing about the next one, which may still fit.
+                    //
+                    // This is also the only ceiling on a single transaction's
+                    // size anywhere on the path into a block. CheckTransaction
+                    // does not cap it and neither does AcceptTransaction, so
+                    // the mempool will hold one of any size the wire allows;
+                    // without this test a lone 1.5 MB transaction offering a
+                    // fat fee is selected, and every template built from that
+                    // mempool is one CheckBlock() throws away. The fee is never
+                    // paid, because the block never confirms.
+                    //
+                    // Deliberately before ConnectInputs and not after:
+                    // ConnectInputs adds this transaction's fee to nFees by
+                    // reference on success. Passing it and then declining the
+                    // transaction would credit the coinbase for a fee no
+                    // transaction in the block pays, and the block would be
+                    // rejected for a wholly different reason.
+                    unsigned int nTxSize   = ::GetSerializeSize(tx, SER_NETWORK);
+                    unsigned int nTxSigOps = tx.GetSigOpCount();
+                    if (!BlockTemplateFits(nBlockSize, nBlockSigOps, nTxSize, nTxSigOps))
+                        continue;
+
                     // Transaction fee requirements, mainly only needed for flood control
                     // Under 10K (about 80 inputs) is free for first 100 transactions
                     // Base rate is 0.01 per KB
@@ -4985,7 +5028,8 @@ bool BitcoinMiner(int nThreadId)
                     swap(mapTestPool, mapTestPoolTmp);
 
                     pblock->vtx.push_back(tx);
-                    nBlockSize += ::GetSerializeSize(tx, SER_NETWORK);
+                    nBlockSize   += nTxSize;
+                    nBlockSigOps += nTxSigOps;
                     vfAlreadyAdded[n] = true;
                     fFoundSomething = true;
                 }
diff --git a/src/main.h b/src/main.h
index 382dd9c..30e25ef 100644
--- a/src/main.h
+++ b/src/main.h
@@ -33,6 +33,35 @@ static const unsigned int MAX_BLOCK_SIGOPS = 20000;
 // limit (32 MB), not a block rule. Bitcoin 0.1.0 predated the later 1 MB block
 // cap, so this tree inherited no real block-size consensus limit.
 static const unsigned int MAX_BLOCK_SIZE = 1000000;
+// How much of that cap a template we build may fill. Half, which is what both
+// assemblers already aimed at -- this only gives the number a name and one
+// place to live. They had already drifted on how it was enforced, which is the
+// bug this exists to stop repeating.
+//
+// NOT a consensus rule. CheckBlock() decides what the network accepts; this
+// decides what we are willing to produce, and it has to stay under that.
+static const unsigned int MAX_BLOCK_SIZE_GEN = MAX_BLOCK_SIZE / 2;
+
+// Room for one more transaction in a template that currently weighs
+// nBlockSize bytes and spends nBlockSigOps signature operations?
+//
+// Asked BEFORE the transaction is appended, never after. A template that has
+// already overshot is one CheckBlock() rejects, and asking afterwards only
+// measures the waste. Both assemblers call this, so neither can drift into a
+// different idea of what fits.
+//
+// Size gets a margin (half the cap); sigops do not. There is no cheap way to
+// spend the sigop budget usefully and no reason to reserve headroom in it, so
+// assembly holds itself to the same number CheckBlock() enforces -- and to the
+// same comparison. CheckBlock() fails on `> MAX_BLOCK_SIGOPS`, so a block
+// sitting exactly on the limit is valid and this has to accept it too.
+// Assembly and validation disagreeing by one is how the size bug started.
+inline bool BlockTemplateFits(unsigned int nBlockSize, unsigned int nBlockSigOps,
+                              unsigned int nTxSize, unsigned int nTxSigOps)
+{
+    return (nBlockSize + nTxSize <= MAX_BLOCK_SIZE_GEN &&
+            nBlockSigOps + nTxSigOps <= MAX_BLOCK_SIGOPS);
+}
 static const int64 COIN = 100000000;
 // Total Bitflash emission: identical to Bitcoin (21 million).
 // MAX_MONEY guards against the value overflow bug (CVE-2010-5139), which in
diff --git a/src/rpc.cpp b/src/rpc.cpp
index 1d53fb2..3827cd5 100644
--- a/src/rpc.cpp
+++ b/src/rpc.cpp
@@ -303,16 +303,29 @@ static bool RebuildJob()
                 map<uint256, CTxIndex> pool;
                 vector<char> vfAlreadyAdded(mapTransactions.size());
                 bool fFoundSomething = true;
-                unsigned int sz = 0;
-                // The cap is MAX_BLOCK_SIZE/2, not MAX_SIZE/2. MAX_SIZE is the
-                // 32 MB serialisation ceiling, so half of it is 16 MB -- this
-                // builder would happily assemble a template that its own
+                // The cap is MAX_BLOCK_SIZE_GEN, not MAX_SIZE/2. MAX_SIZE is
+                // the 32 MB serialisation ceiling, so half of it is 16 MB --
+                // this builder would happily assemble a template that its own
                 // CheckBlock() rejects at the 1 MB consensus cap. This is the
                 // pool's job builder, so a block over the cap is not one
                 // wasted solve, it is every miner on the pool working a
-                // template the network will reject. The inner break bounds the
-                // overshoot to a single transaction within a pass.
-                while (fFoundSomething && sz < MAX_BLOCK_SIZE / 2)
+                // template the network will reject.
+                //
+                // The inner break used to bound the overshoot to one
+                // transaction per pass. One transaction is still too many:
+                // nothing caps how big a single transaction may be, so that
+                // bound was worth up to MAX_SIZE. Asking whether each one fits
+                // before appending it bounds the overshoot at zero instead.
+                //
+                // Seeded with the header, tx count and coinbase, because
+                // CheckBlock() weighs the whole serialized block.
+                unsigned int sz = ::GetSerializeSize(block, SER_DISK);
+                // And the sigop budget, which CheckBlock() enforces and this
+                // builder never counted against. Same counting function
+                // CheckBlock() sums, seeded from the coinbase because it sums
+                // over all of vtx.
+                unsigned int nSigOps = block.vtx[0].GetSigOpCount();
+                while (fFoundSomething && sz < MAX_BLOCK_SIZE_GEN)
                 {
                     fFoundSomething = false;
                     unsigned int n = 0;
@@ -323,16 +336,26 @@ static bool RebuildJob()
                             continue;
                         CTransaction& tx = (*mi).second;
                         if (tx.IsCoinBase() || !tx.IsFinal()) continue;
+                        // Skip rather than stop: one transaction too big for
+                        // the room left does not mean the next is. Ordered
+                        // before ConnectInputs because ConnectInputs credits
+                        // nFees by reference on success, and a transaction we
+                        // then decline would leave the coinbase claiming a fee
+                        // this block does not collect.
+                        unsigned int nTxSize   = ::GetSerializeSize(tx, SER_NETWORK);
+                        unsigned int nTxSigOps = tx.GetSigOpCount();
+                        if (!BlockTemplateFits(sz, nSigOps, nTxSize, nTxSigOps))
+                            continue;
                         map<uint256, CTxIndex> 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);
+                        sz      += nTxSize;
+                        nSigOps += nTxSigOps;
                         vfAlreadyAdded[n] = true;
                         fFoundSomething = true;
-                        if (sz > MAX_BLOCK_SIZE / 2) break;
                     }
                 }
             }
diff --git a/src/selftest.cpp b/src/selftest.cpp
index 9f7106f..d11fdd8 100644
--- a/src/selftest.cpp
+++ b/src/selftest.cpp
@@ -1235,6 +1235,74 @@ static CBlock MakeSizedConsensusBlock(size_t nScriptBytes)
     return block;
 }
 
+// A distinct transaction of a chosen weight and verification cost. nUnique
+// only keeps the txids apart, so a mempool built from these is not a pile of
+// duplicates a block could never carry anyway.
+static CTransaction MakeSizedTx(size_t nPadBytes, unsigned int nSigOps, int nUnique)
+{
+    CTransaction tx;
+    tx.vin.push_back(CTxIn(COutPoint(), CScript() << (int64)nUnique << 1));
+    tx.vout.push_back(CTxOut(0, CScript()));
+    for (unsigned int i = 0; i < nSigOps; i++)
+        tx.vout[0].scriptPubKey << OP_CHECKSIG;
+    tx.vout[0].scriptPubKey.insert(tx.vout[0].scriptPubKey.end(), nPadBytes, 0);
+    return tx;
+}
+
+static CBlock MakeCoinbaseOnlyTemplate()
+{
+    CTransaction txNew;
+    txNew.vin.resize(1);
+    txNew.vin[0].prevout.SetNull();
+    txNew.vin[0].scriptSig << 1 << 1 << 1;
+    txNew.vout.resize(1);
+    txNew.vout[0].scriptPubKey << OP_CHECKSIG;
+
+    CBlock block;
+    block.vtx.push_back(txNew);
+    block.nVersion = 1;
+    block.nTime = GetAdjustedTime();
+    block.nBits = bnProofOfWorkLimit.GetCompact();
+    block.nNonce = 0;
+    return block;
+}
+
+// The sum CheckBlock() takes, spelled the same way.
+static unsigned int BlockSigOpCount(const CBlock& block)
+{
+    unsigned int n = 0;
+    for (int i = 0; i < (int)block.vtx.size(); i++)
+        n += block.vtx[i].GetSigOpCount();
+    return n;
+}
+
+// Drive the selection rule the way both assemblers drive it: ask
+// BlockTemplateFits before appending, skip what does not fit, never stop early
+// on one that does not.
+//
+// This mirrors the loops in BitcoinMiner() and RebuildJob() rather than calling
+// them -- one needs a CTxDB, a wallet key and cs_main, the other is static in
+// rpc.cpp, so neither is reachable from here. What that leaves proven is the
+// rule the two of them share and the property it is meant to guarantee, which
+// is where the defect actually lived: the budget was right and the enforcement
+// was not.
+static void SelectIntoTemplate(CBlock& block, const vector<CTransaction>& vMempool)
+{
+    unsigned int nBlockSize   = ::GetSerializeSize(block, SER_DISK);
+    unsigned int nBlockSigOps = block.vtx[0].GetSigOpCount();
+    for (int i = 0; i < (int)vMempool.size(); i++)
+    {
+        const CTransaction& tx = vMempool[i];
+        unsigned int nTxSize   = ::GetSerializeSize(tx, SER_NETWORK);
+        unsigned int nTxSigOps = tx.GetSigOpCount();
+        if (!BlockTemplateFits(nBlockSize, nBlockSigOps, nTxSize, nTxSigOps))
+            continue;
+        block.vtx.push_back(tx);
+        nBlockSize   += nTxSize;
+        nBlockSigOps += nTxSigOps;
+    }
+}
+
 static int RunConsensusLimitsSelfTest()
 {
     fflush(stdout);
@@ -1258,6 +1326,95 @@ static int RunConsensusLimitsSelfTest()
                        "the test block sits between 1 MB and the old 32 MB cap") ? 0 : 1;
         nFail += Check(!oversized.CheckSizeLimits(),
                        "a block above 1 MB is outside the consensus size limit") ? 0 : 1;
+
+        // The other half of the cap. Everything above asks what validation
+        // refuses; nothing asked what assembly produces, which is why a miner
+        // that could overshoot the cap by an entire mempool went unnoticed.
+        nFail += Check(MAX_BLOCK_SIZE_GEN < MAX_BLOCK_SIZE,
+                       "the assembly budget stays under the consensus cap") ? 0 : 1;
+
+        // The rule itself, at its edges. The sigop comparison has to match
+        // CheckBlock(), which fails on `> MAX_BLOCK_SIGOPS` and so accepts a
+        // block sitting exactly on the limit.
+        nFail += Check(BlockTemplateFits(MAX_BLOCK_SIZE_GEN - 100, 0, 100, 0),
+                       "a transaction that exactly fills the size budget fits") ? 0 : 1;
+        nFail += Check(!BlockTemplateFits(MAX_BLOCK_SIZE_GEN - 100, 0, 101, 0),
+                       "one byte past the size budget does not fit") ? 0 : 1;
+        nFail += Check(BlockTemplateFits(0, 0, 0, MAX_BLOCK_SIGOPS),
+                       "sigops exactly on the limit fit, as CheckBlock() allows") ? 0 : 1;
+        nFail += Check(!BlockTemplateFits(0, 0, 0, MAX_BLOCK_SIGOPS + 1),
+                       "one sigop past the limit does not fit") ? 0 : 1;
+
+        // A mempool several times the consensus cap, in ordinary-sized
+        // transactions. The old miner tested its budget only on the outer
+        // loop, so a single pass appended all of this.
+        {
+            CBlock block = MakeCoinbaseOnlyTemplate();
+            vector<CTransaction> vMempool;
+            for (int i = 0; i < 300; i++)
+                vMempool.push_back(MakeSizedTx(10000, 1, i));
+            SelectIntoTemplate(block, vMempool);
+            block.hashMerkleRoot = block.BuildMerkleTree();
+
+            nFail += Check(block.vtx.size() > 1,
+                           "a full mempool still fills a template") ? 0 : 1;
+            nFail += Check(::GetSerializeSize(block, SER_DISK) <= MAX_BLOCK_SIZE_GEN,
+                           "a template built from a 3 MB mempool respects the budget") ? 0 : 1;
+            nFail += Check(block.CheckSizeLimits(),
+                           "that template is one CheckBlock() would accept on size") ? 0 : 1;
+        }
+
+        // One transaction too big for the budget. Nothing on the path into the
+        // mempool caps transaction size, so this is a transaction a peer can
+        // simply send. It must be skipped and the rest still taken -- skipped,
+        // not stopped on.
+        {
+            CBlock block = MakeCoinbaseOnlyTemplate();
+            vector<CTransaction> vMempool;
+            vMempool.push_back(MakeSizedTx(1500000, 1, 9999));
+            for (int i = 0; i < 10; i++)
+                vMempool.push_back(MakeSizedTx(1000, 1, i));
+
+            unsigned int nHugeSize = ::GetSerializeSize(vMempool[0], SER_NETWORK);
+            nFail += Check(nHugeSize > MAX_BLOCK_SIZE && nHugeSize <= MAX_SIZE,
+                           "the outsized transaction is one the mempool would hold") ? 0 : 1;
+
+            SelectIntoTemplate(block, vMempool);
+            block.hashMerkleRoot = block.BuildMerkleTree();
+
+            bool fTookHuge = false;
+            for (int i = 0; i < (int)block.vtx.size(); i++)
+                if (block.vtx[i].GetHash() == vMempool[0].GetHash())
+                    fTookHuge = true;
+
+            nFail += Check(!fTookHuge,
+                           "a transaction larger than the budget is left out") ? 0 : 1;
+            nFail += Check(block.vtx.size() == 11,
+                           "the ten that do fit are taken anyway") ? 0 : 1;
+            nFail += Check(block.CheckSizeLimits(),
+                           "one outsized transaction cannot invalidate a template") ? 0 : 1;
+        }
+
+        // Transactions that are cheap in bytes and expensive to verify. Size
+        // alone does not bound what a block costs, and CheckBlock() refuses on
+        // the count regardless of how small the block is.
+        {
+            CBlock block = MakeCoinbaseOnlyTemplate();
+            vector<CTransaction> vMempool;
+            for (int i = 0; i < 100; i++)
+                vMempool.push_back(MakeSizedTx(0, 1000, i));
+            SelectIntoTemplate(block, vMempool);
+            block.hashMerkleRoot = block.BuildMerkleTree();
+
+            nFail += Check(block.vtx.size() > 1,
+                           "sigop-heavy transactions are still selected") ? 0 : 1;
+            nFail += Check(BlockSigOpCount(block) <= MAX_BLOCK_SIGOPS,
+                           "selection stops at the sigop cap CheckBlock() enforces") ? 0 : 1;
+            nFail += Check(::GetSerializeSize(block, SER_DISK) < MAX_BLOCK_SIZE_GEN,
+                           "and stops on sigops while still well inside the size budget") ? 0 : 1;
+            nFail += Check(block.CheckSizeLimits(),
+                           "the sigop-bounded template is within the size limit too") ? 0 : 1;
+        }
     }
     catch (const std::exception& e)
     {