payout: ask the wallet why a send failed instead of guessing from the balance

A payment that the pool could not fund was being recorded as a fault
against the payment, and 1440 faults erased the debt with the coin still
in the operator's wallet.

The classifier read GetBalance() after the failure and asked whether it
covered `amount + nTransactionFee + CENT`. Every term of that was wrong.
nTransactionFee is 0 on this pool -- it is only ever assigned from the
GUI preferences dialog, which a headless node does not have -- so the
entire reserve was one CENT. The fee it is meant to reserve for is
GetMinFee(fDiscount=true), which is a step function of the serialised
size: 0 below 10,000 bytes and (1 + bytes/1000) * CENT at or above it.
There is no required-fee value between 1 and 11 CENT, so a one-CENT
reserve is never the right number -- below the step nothing needs
reserving, at the step it is short by a factor of eleven.

Worse, the trigger correlates with the failure. A pool wallet close to
broke selects many small inputs BECAUSE it is close to broke, and about
56 signed P2PKH inputs is 10 KB, which a pool that has been paying out
accumulates as change. So: balance of amount + 0.05, the pre-send gate
passes, CreateTransaction builds a transaction over 10 KB, the fee
becomes 0.11, SelectCoins(amount + 0.11) fails, SendMoney returns false
-- and (amount + 0.05) < (amount + 0 + 0.01) is false, so "funded", so a
fault. The condition is stable, so it recurred every 60 s until the
budget ran out and ForfeitPayoutRecipient erased the miner's money.

Three changes.

The wallet now says what it did. CreateTransaction takes an optional
WalletSendResult out-parameter and sets WALLET_SEND_INSUFFICIENT_FUNDS
on the one return that means a funding shortfall -- SelectCoins failing
to cover nValue plus whatever fee that pass is working with, which on
the second and later passes is GetMinFee() of the transaction the first
pass actually built. SendMoney gains a four-argument overload returning
that plus the fee that was required. The three-argument SendMoney stays,
as a separate declaration rather than a default argument, because
gui.cpp declares it itself instead of including main.h; walletcmd.cpp
and the GUI are unchanged. These are wallet functions, not consensus.

ClassifyPayoutSendFailure() then classifies the answer rather than
guessing: locked or short of coin defers, anything else is a fault.
There is no reserve left to get wrong, and the size-dependent fee is
accounted for exactly, because the wallet computed it.

And the give-up branch is gone. 1440 failures is not a decidable state,
it is a count of how long we have been asking, and this pool reached it
by misclassifying a permanent funding shortfall. A payment that keeps
failing now stays in flight, keeps its ledger entry, keeps being
reported as owed in pool_status.json, and keeps retrying once a minute.
A debt that is never written off cannot be written off wrongly. The
remaining call to ForfeitPayoutRecipient is the one decidable case --
the ledger names a string that is not a valid address, so no transaction
can pay it -- which previously only dropped the in-flight record and
left the ledger and the round proof claiming the debt forever.

Tested against a real wallet and the real CreateTransaction, not against
injected values, because the honest question is whether a wallet holding
many small coins reports the state this code assumes. 60 coins of 1 BTF,
balance 60.00, amount 59.95: CreateTransaction returns false with
WALLET_SEND_INSUFFICIENT_FUNDS and a required fee of 0.11 -- and since
GetMinFee returns 0 below 10,000 bytes, a required fee of 11 CENT is
itself the measurement that the transaction crossed the step. The same
test asserts that the old reserve judged that state "funded", and that
the same wallet pays a 3-input transaction with a required fee of 0.00,
which is why no fixed reserve is right for both.

Property: a send that failed because the wallet could not fund it is
deferred, whatever the transaction's size, and no number of failures
erases a debt.

1 files changed, 43 insertions, 4 deletions

diff --git a/src/main.cpp b/src/main.cpp
index 252911b..7b3e555 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -4907,9 +4907,17 @@ bool SelectCoins(int64 nTargetValue, set<CWalletTx*>& setCoinsRet)
 
 
 
-bool CreateTransaction(CScript scriptPubKey, int64 nValue, CWalletTx& wtxNew, int64& nFeeRequiredRet)
+// presultRet, when given, says WHY this returned false: the caller cannot work
+// it out afterwards. "Coin selection came up short" and "the wallet could not
+// build the transaction" are indistinguishable from the outside -- both are
+// just `false` -- and the difference decides whether a pool defers a payment
+// or counts it against the payment's retry budget. See WalletSendResult.
+bool CreateTransaction(CScript scriptPubKey, int64 nValue, CWalletTx& wtxNew, int64& nFeeRequiredRet,
+                       WalletSendResult* presultRet)
 {
     nFeeRequiredRet = 0;
+    if (presultRet)
+        *presultRet = WALLET_SEND_FAILED;
     CRITICAL_BLOCK(cs_main)
     {
         // txdb must be opened before the mapWallet lock
@@ -4940,10 +4948,21 @@ bool CreateTransaction(CScript scriptPubKey, int64 nValue, CWalletTx& wtxNew, in
                 // fixed this in 107d9e288.
                 int64 nTotalValue = nValue + nFee;
 
-                // Choose coins to use
+                // Choose coins to use.
+                //
+                // Failing here is a funding shortfall and nothing else: there
+                // is not enough spendable coin for nValue plus the fee this
+                // pass is working with. On the second and later passes that
+                // fee is GetMinFee() of the transaction the first pass built,
+                // so this is also how a wallet that can afford the amount but
+                // not the 11 * CENT charged at 10,000 bytes reports itself.
                 set<CWalletTx*> setCoins;
                 if (!SelectCoins(nTotalValue, setCoins))
+                {
+                    if (presultRet)
+                        *presultRet = WALLET_SEND_INSUFFICIENT_FUNDS;
                     return false;
+                }
                 int64 nValueIn = 0;
                 foreach(CWalletTx* pcoin, setCoins)
                     nValueIn += pcoin->GetCredit();
@@ -5013,6 +5032,8 @@ bool CreateTransaction(CScript scriptPubKey, int64 nValue, CWalletTx& wtxNew, in
             }
         }
     }
+    if (presultRet)
+        *presultRet = WALLET_SEND_OK;
     return true;
 }
 
@@ -5075,13 +5096,24 @@ bool CommitTransactionSpent(const CWalletTx& wtxNew)
 
 
 
-bool SendMoney(CScript scriptPubKey, int64 nValue, CWalletTx& wtxNew)
+// outcomeRet reports what happened rather than only whether it worked. A
+// caller that has to decide what a failure MEANS -- the pool payout thread --
+// cannot reconstruct it from a balance reading afterwards, because the fee
+// that was actually required depends on the size of a transaction that was
+// never built. See WalletSendResult.
+bool SendMoney(CScript scriptPubKey, int64 nValue, CWalletTx& wtxNew,
+               SendMoneyOutcome& outcomeRet)
 {
+    outcomeRet.result       = WALLET_SEND_FAILED;
+    outcomeRet.nFeeRequired = 0;
     CRITICAL_BLOCK(cs_main)
     {
         int64 nFeeRequired;
-        if (!CreateTransaction(scriptPubKey, nValue, wtxNew, nFeeRequired))
+        WalletSendResult createResult = WALLET_SEND_FAILED;
+        if (!CreateTransaction(scriptPubKey, nValue, wtxNew, nFeeRequired, &createResult))
         {
+            outcomeRet.result       = createResult;
+            outcomeRet.nFeeRequired = nFeeRequired;
             string strError;
             if (nValue + nFeeRequired > GetBalance())
                 // Not an oversized transaction -- this branch is simply "the
@@ -5119,6 +5151,13 @@ bool SendMoney(CScript scriptPubKey, int64 nValue, CWalletTx& wtxNew)
         }
         wtxNew.RelayWalletTransaction();
     }
+    outcomeRet.result = WALLET_SEND_OK;
     MainFrameRepaint();
     return true;
 }
+
+bool SendMoney(CScript scriptPubKey, int64 nValue, CWalletTx& wtxNew)
+{
+    SendMoneyOutcome ignored;
+    return SendMoney(scriptPubKey, nValue, wtxNew, ignored);
+}