3 files changed, 81 insertions, 14 deletions

diff --git a/src/main.cpp b/src/main.cpp
index 7b3e555..64eb290 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -4470,7 +4470,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.
             //
@@ -4481,9 +4480,19 @@ 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);
+            while (fFoundSomething && nBlockSize < MAX_BLOCK_SIZE_GEN)
             {
                 fFoundSomething = false;
                 unsigned int n = 0;
@@ -4495,6 +4504,28 @@ 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);
+                    if (!BlockTemplateFits(nBlockSize, nTxSize))
+                        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
@@ -4506,7 +4537,7 @@ bool BitcoinMiner(int nThreadId)
                     swap(mapTestPool, mapTestPoolTmp);
 
                     pblock->vtx.push_back(tx);
-                    nBlockSize += ::GetSerializeSize(tx, SER_NETWORK);
+                    nBlockSize += nTxSize;
                     vfAlreadyAdded[n] = true;
                     fFoundSomething = true;
                 }
diff --git a/src/main.h b/src/main.h
index 382dd9c..1f452b5 100644
--- a/src/main.h
+++ b/src/main.h
@@ -33,6 +33,26 @@ 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?
+//
+// 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.
+inline bool BlockTemplateFits(unsigned int nBlockSize, unsigned int nTxSize)
+{
+    return (nBlockSize + nTxSize <= MAX_BLOCK_SIZE_GEN);
+}
 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 a4b53ca..7c5ef6c 100644
--- a/src/rpc.cpp
+++ b/src/rpc.cpp
@@ -267,16 +267,24 @@ 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);
+                while (fFoundSomething && sz < MAX_BLOCK_SIZE_GEN)
                 {
                     fFoundSomething = false;
                     unsigned int n = 0;
@@ -287,16 +295,24 @@ 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);
+                        if (!BlockTemplateFits(sz, nTxSize))
+                            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;
                         vfAlreadyAdded[n] = true;
                         fFoundSomething = true;
-                        if (sz > MAX_BLOCK_SIZE / 2) break;
                     }
                 }
             }