mine: ask whether a transaction fits before appending it

The node miner tested its size budget only on the outer while loop. The inner
for walked all of mapTransactions with no test, so a single pass could append
the entire mempool -- the overshoot was bounded by mempool size, not by one
transaction as the comment claimed.

The pool builder did test, but only after the push, which bounds the overshoot
at one transaction. Nothing caps how big one transaction may be: neither
CheckTransaction nor AcceptTransaction limits it, so the mempool will hold one
of any size the wire allows and "one transaction" is worth up to MAX_SIZE. A
lone 1.5 MB transaction offering a large fee is therefore enough to make every
template unmineable, and the fee is never paid because the block never
confirms.

Both now ask before appending and skip what does not fit, so the overshoot is
zero and an oversized transaction costs the template nothing rather than
invalidating it. The budget moves to MAX_BLOCK_SIZE_GEN so the two assemblers
cannot drift on it again -- drift is what produced this.

The test is ordered before ConnectInputs on purpose: ConnectInputs credits
nFees by reference on success, so declining a transaction after calling it
would leave the coinbase claiming a fee the block does not collect.

Assembly only. CheckBlock, CheckTransaction and AcceptTransaction are
untouched: this changes what we produce, not what the network accepts.

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;
                     }
                 }
             }