selftest: cover the assembly half of the block limits

consensus-limits only ever asked what CheckBlock() refuses. Nothing asked what
the assemblers produce, which is why a miner able to overshoot the cap by an
entire mempool sat here unnoticed through both the deployed and candidate
builds.

Adds the other half: a template built from a 3 MB mempool stays inside the
budget and passes CheckSizeLimits(); a transaction too big for the budget is
skipped while the ones that fit are still taken; sigop-heavy transactions stop
selection at MAX_BLOCK_SIGOPS while the block is still small. Plus the rule's
edges, including that sigops exactly on the limit fit -- CheckBlock() fails on
`>`, so assembly must accept what validation accepts.

The test drives BlockTemplateFits the way the assemblers drive it rather than
calling them: BitcoinMiner needs a CTxDB, a wallet key and cs_main, and
RebuildJob is static in rpc.cpp. What that proves is the shared rule and the
property it exists to give, which is where the defect was -- the budget was
right and the enforcement was not.

Checked against the bug: with the fit test neutered, eight of these fail,
including both templates that CheckBlock() would have rejected. With the sigop
comparison stricter than CheckBlock's by one, the boundary case fails.

1 files changed, 157 insertions, 0 deletions

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)
     {