wallet: stop paying the recipient the transaction fee on a fee retry (#74)

CreateTransaction kept the payee amount in nValue and then did

    int64 nValueOut = nValue;
    nValue += nFee;

with both lines inside the retry loop. The first pass is fine. When the fee
turns out to be too low the loop starts over, and on the second pass
nValueOut is read from an nValue that already carries the fee -- so the
recipient is sent the amount plus the fee, out of the sender's pocket, and
it compounds on every further retry.

Reachable, but it takes two things at once, which is why it has sat here:
the user must have set a transaction fee in Options (with the default of
zero, nValue += nFee adds nothing on the first pass and nothing is
corrupted), and the transaction must be big enough for GetMinFee to demand
more than that -- over 10 KB, so a wallet paying from many small inputs.

Keep nValue meaning what the payee asked for and carry nValue + nFee in a
separate nTotalValue, the way Bitcoin fixed it in 107d9e288.

Verified by building the same transaction on both versions, with a
synthetic wallet of 200 coins of 50, a configured fee of 0.05, and a
request of 5,000 that needs 101 inputs and 28 KB -- enough for GetMinFee to
demand 0.12 and force the retry:

    before   payee gets 5,000.05   change 49.83
    after    payee gets 5,000.00   change 49.88

Two other fixes from the same era were considered and left out. Satoshi's
CENT floor on the change output (496823249) exists to avoid a dust output
that his fee rules then penalised; this fee schedule has no such rule, so
adopting it would only donate sub-cent change to the miner. The
1-confirmation rule (a790fa46f) is a spending-policy change, not a bug fix,
and deserves its own decision.

1 files changed, 20 insertions, 6 deletions

diff --git a/src/main.cpp b/src/main.cpp
index c3fb33b..5610de9 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -3437,22 +3437,36 @@ bool CreateTransaction(CScript scriptPubKey, int64 nValue, CWalletTx& wtxNew, in
                 wtxNew.vout.clear();
                 if (nValue < 0)
                     return false;
-                int64 nValueOut = nValue;
-                nValue += nFee;
+
+                // nValue is the amount the payee asked for and must stay that.
+                //
+                // This used to read "nValueOut = nValue; nValue += nFee;", and
+                // both lines are inside the retry loop. The first pass was
+                // fine. But when the fee turned out to be too low the loop
+                // starts over, and on the second pass nValueOut is read from
+                // an nValue that already had the fee added -- so the payee is
+                // sent the amount plus the fee, and it compounds on every
+                // further retry, with the sender covering it.
+                //
+                // The retry fires whenever GetMinFee exceeds the fee we
+                // guessed, which for this fee schedule means any transaction
+                // over 10 KB: a wallet paying from many small inputs. Bitcoin
+                // fixed this in 107d9e288.
+                int64 nTotalValue = nValue + nFee;
 
                 // Choose coins to use
                 set<CWalletTx*> setCoins;
-                if (!SelectCoins(nValue, setCoins))
+                if (!SelectCoins(nTotalValue, setCoins))
                     return false;
                 int64 nValueIn = 0;
                 foreach(CWalletTx* pcoin, setCoins)
                     nValueIn += pcoin->GetCredit();
 
                 // Fill vout[0] to the payee
-                wtxNew.vout.push_back(CTxOut(nValueOut, scriptPubKey));
+                wtxNew.vout.push_back(CTxOut(nValue, scriptPubKey));
 
                 // Fill vout[1] back to self with any change
-                if (nValueIn > nValue)
+                if (nValueIn > nTotalValue)
                 {
                     // Use the same key as one of the coins
                     vector<unsigned char> vchPubKey;
@@ -3467,7 +3481,7 @@ bool CreateTransaction(CScript scriptPubKey, int64 nValue, CWalletTx& wtxNew, in
                     // Fill vout[1] to ourself
                     CScript scriptPubKey;
                     scriptPubKey << vchPubKey << OP_CHECKSIG;
-                    wtxNew.vout.push_back(CTxOut(nValueIn - nValue, scriptPubKey));
+                    wtxNew.vout.push_back(CTxOut(nValueIn - nTotalValue, scriptPubKey));
                 }
 
                 // Fill vin