1 files changed, 367 insertions, 74 deletions

diff --git a/src/rpc.cpp b/src/rpc.cpp
index 403cb14..06de7f2 100644
--- a/src/rpc.cpp
+++ b/src/rpc.cpp
@@ -1700,8 +1700,11 @@ static void ForgetPayment(const PayingRecord& r)
 
         // When the last record for a round is gone the round is no longer in
         // flight, so a later flush may explode it again if the ledger still
-        // shows recipients owed (a give-up, for instance, leaves the entry
-        // standing).
+        // shows recipients owed. That is the intended behaviour and it is now
+        // the only way a stalled payment ever leaves this list: a payment that
+        // keeps failing is retried forever rather than given up on, so its
+        // record stays here, and the flush that would re-explode it is stopped
+        // by PaymentInFlightLocked.
         bool stillInFlight = false;
         for (const auto& e : gPaying)
             if (SamePayoutRound(e.blockHeight, e.blockHash,
@@ -1776,9 +1779,16 @@ static void RecordPayoutTxid(const PayingRecord& r, const std::string& txid)
         LogPrint("payout", "[payout] block %d fully settled\n", r.blockHeight);
 }
 
-// Write a recipient off. Only reached after a full budget of failures that
-// were NOT the wallet being locked or short of coin -- see the classification
-// in PayoutThreadFn -- against a block still on the main chain.
+// Write a recipient off.
+//
+// Only ever called for a state that is DECIDABLE -- currently one: the ledger
+// names an address that is not a valid address, so no transaction paying it
+// can be constructed by this or any other build. It is not called, and must
+// not be, because a payment has failed some number of times. "The wallet said
+// no N times" is not a decidable state, it is a description of how long we
+// have been asking; a debt written off on the strength of it is a debt the
+// operator keeps, and the arithmetic that decided WHY the wallet said no was
+// wrong for a year (see PayoutThreadFn).
 //
 // The amount is removed from the entry rather than marked paid, so the money
 // is not reported as sent. Once nothing is left owed the entry retires. The
@@ -1810,6 +1820,60 @@ static void ForfeitPayoutRecipient(const PayingRecord& r, const std::string& rea
                            r.address, reason);
 }
 
+// What a failed send means for the payment's retry budget.
+//
+// A deferral is "the wallet's condition is temporarily wrong": wait, and do
+// not hold it against this payment. A fault is "the wallet is funded and
+// unlocked and still would not do it", which is worth logging loudly and
+// retrying with backoff.
+//
+// This used to be decided by re-reading GetBalance() after the failure and
+// comparing it against `amount + nTransactionFee + CENT`. Every term of that
+// was wrong:
+//
+//   * nTransactionFee is 0 in production. It is only ever assigned from the
+//     GUI preferences dialog (gui.cpp), which a headless pool node does not
+//     have, so the whole reserve was the CENT.
+//
+//   * The fee the reserve is for is GetMinFee(fDiscount=true), a STEP
+//     function of the serialised size: 0 below 10,000 bytes, and
+//     (1 + bytes/1000) * CENT at or above it -- so 11 * CENT at the step.
+//     There is no required-fee value between 1 and 11 CENT. A one-CENT
+//     reserve is therefore never the right number: below the step nothing
+//     needs reserving, and at the step it is short by a factor of eleven.
+//
+//   * A pool wallet close to broke selects many small inputs BECAUSE it is
+//     close to broke, and ~56 signed P2PKH inputs is 10 KB. The trigger and
+//     the failure are positively correlated: the guess is wrong precisely
+//     when the pool is nearly out of money.
+//
+// Concretely, with a balance of amount + 0.05: the pre-send gate passes,
+// CreateTransaction builds a >10 KB transaction, the fee becomes 0.11,
+// SelectCoins(amount + 0.11) fails, SendMoney returns false -- and the old
+// arithmetic said (amount + 0.05) < (amount + 0.01) is false, so "funded",
+// so a fault. The condition is stable, so it recurred every 60 s, and 1440 of
+// them erased the debt with the coin still in the operator's wallet.
+//
+// So stop guessing. The wallet is asked to say what it did, and this only
+// classifies the answer. WALLET_SEND_INSUFFICIENT_FUNDS is coin selection
+// failing to cover amount plus whatever fee that size of transaction
+// required, whatever that turned out to be -- no reserve to get wrong.
+enum PayoutFailureClass {
+    PAYOUT_FAILURE_DEFER = 0, // conditions are wrong; wait for them to change
+    PAYOUT_FAILURE_FAULT      // funded and unlocked, and it still would not go
+};
+
+static PayoutFailureClass ClassifyPayoutSendFailure(WalletSendResult result,
+                                                    bool fWalletLocked)
+{
+    // A locked wallet cannot sign. Nothing about this payment is wrong.
+    if (fWalletLocked)
+        return PAYOUT_FAILURE_DEFER;
+    if (result == WALLET_SEND_INSUFFICIENT_FUNDS)
+        return PAYOUT_FAILURE_DEFER;
+    return PAYOUT_FAILURE_FAULT;
+}
+
 // Payout execution thread: works through gPaying, calls SendMoney.
 // Runs independently of the event loop so slow wallet operations
 // (SendMoney takes cs_main) never stall miner I/O.
@@ -1867,9 +1931,24 @@ static void PayoutThreadFn(void*)
 
             uint160 h160;
             if (!AddressToHash160(r.address, h160)) {
-                // Should not happen -- bad addresses are filtered in QueuePayouts
-                LogPrint("payout", "[payout] SKIPPED bad address '%s'\n",
-                         r.address.c_str());
+                // Should not happen -- bad addresses are filtered in
+                // QueuePayouts. If one is here anyway it is decidable and
+                // permanent: there is no transaction that pays a string which
+                // is not an address, so retrying is not waiting for anything.
+                //
+                // This used to only ForgetPayment(), which drops the in-flight
+                // record and leaves the ledger entry claiming the debt and
+                // pool_rounds.json advertising the recipient as unpaid
+                // forever, with the next flush re-exploding it. Write it off
+                // properly and say why.
+                const std::string reason =
+                    strprintf("'%s' is not a valid address, so no transaction"
+                              " can pay it", r.address.c_str());
+                LogPrint("payout", "[payout] SKIPPED bad address '%s' --"
+                         " %s owed for block %d cannot be paid\n",
+                         r.address.c_str(), FormatMoney(r.amount).c_str(),
+                         r.blockHeight);
+                ForfeitPayoutRecipient(r, reason);
                 ForgetPayment(r);
                 continue;
             }
@@ -1926,13 +2005,17 @@ static void PayoutThreadFn(void*)
             // the operator's wallet and nothing on disk saying it was owed.
             //
             // nTransactionFee is the part of the fee that can be known before
-            // the transaction exists. The rest is GetMinFee(), a function of
-            // the serialised size and therefore of how many inputs SelectCoins
-            // ends up picking, which cannot be predicted from here -- so this
-            // gate deliberately does NOT try to reserve it. Refusing to attempt
-            // a payment the wallet could actually have made would be its own
-            // way of not paying a miner. That residual is handled after the
-            // fact, by classifying the failure below.
+            // the transaction exists, and in production it is 0 -- it is only
+            // ever assigned from the GUI preferences dialog, which a pool node
+            // does not have -- so including it here changes nothing on this
+            // pool and is kept only because a GUI operator can set it. The
+            // rest is GetMinFee(), a function of the serialised size and
+            // therefore of how many inputs SelectCoins ends up picking, which
+            // cannot be predicted from here -- so this gate deliberately does
+            // NOT try to reserve it. Refusing to attempt a payment the wallet
+            // could actually have made would be its own way of not paying a
+            // miner. That residual is handled after the fact, by asking the
+            // wallet why it refused -- see ClassifyPayoutSendFailure.
             int64 nSpendable = GetBalance();
             if (nSpendable < r.amount + nTransactionFee) {
                 r.deferrals++;
@@ -1977,7 +2060,8 @@ static void PayoutThreadFn(void*)
             wtx.mapValue["pool_payout_address"] = r.address;
             wtx.mapValue["pool_payout_amount"]  = strprintf("%lld", r.amount);
 
-            if (SendMoney(sc, r.amount, wtx)) {
+            SendMoneyOutcome outcome;
+            if (SendMoney(sc, r.amount, wtx, outcome)) {
                 // The wallet holds the transaction before this returns, so a
                 // crash between here and the ledger write is recoverable: the
                 // probe above finds it by id+address+amount and does not
@@ -1989,74 +2073,67 @@ static void PayoutThreadFn(void*)
                          FormatMoney(r.amount).c_str(), r.address.c_str(),
                          wtx.GetHash().ToString().substr(0, 12).c_str());
             } else {
-                // Why did it fail? The answer decides whether this costs a
-                // retry, and the retry budget is what eventually erases a
-                // miner's money -- so guessing here is how a pool keeps coin
-                // it owes.
+                // Why did it fail? The answer used to decide whether the
+                // payment eventually got erased, and it was decided by
+                // guessing from a balance snapshot -- see
+                // ClassifyPayoutSendFailure for the arithmetic and what it
+                // cost. The wallet now says what it did, and nothing here
+                // erases anything.
                 //
-                // Re-read the conditions rather than trusting the ones checked
-                // before the attempt: SendMoney takes cs_main and can block for
-                // a long time, so the balance and the lock state may both have
-                // moved underneath it.
-                //
-                // Short of coin covers the size-dependent part of the fee that
-                // the gate above deliberately does not reserve: if the balance
-                // no longer covers amount + configured fee + one fee tier, the
-                // failure is a funding shortfall, not a fault.
-                int64 nAfter    = GetBalance();
-                bool  fLocked   = IsWalletLocked();
-                bool  fUnfunded = nAfter < r.amount + nTransactionFee + CENT;
-
-                if (fLocked || fUnfunded) {
-                    // Conditions are temporarily wrong. Wait; do not spend the
-                    // budget that leads to a permanent write-off.
+                // The lock state is re-read rather than trusted from before
+                // the attempt: SendMoney takes cs_main and can block for a
+                // long time, so it may have moved underneath us. The funding
+                // answer does not need re-reading -- it is a statement about
+                // the attempt that was actually made.
+                bool fLocked = IsWalletLocked();
+                int64 nAfter = GetBalance();
+
+                if (ClassifyPayoutSendFailure(outcome.result, fLocked)
+                        == PAYOUT_FAILURE_DEFER) {
+                    // Conditions are temporarily wrong. Wait.
                     r.deferrals++;
                     r.retryAfter = GetTime() + 60;
                     if (r.deferrals == 1 || r.deferrals % 60 == 0)
                         LogPrint("payout", "[payout] deferring %s to %s -- send"
-                                 " failed with %s (%s spendable, %s needed with"
-                                 " fee headroom); waiting %d min so far\n",
+                                 " failed with %s (%s spendable; the wallet"
+                                 " needed a fee of %s for a transaction this"
+                                 " size); waiting %d min so far\n",
                                  FormatMoney(r.amount).c_str(), r.address.c_str(),
                                  fLocked ? "the wallet locked" : "too little coin",
                                  FormatMoney(nAfter).c_str(),
-                                 FormatMoney(r.amount + nTransactionFee + CENT).c_str(),
+                                 FormatMoney(outcome.nFeeRequired).c_str(),
                                  r.deferrals);
                     ReschedulePayment(r);
-                } else if (r.retries < 1440) {
-                    // A genuine fault: funded, unlocked, block on the main
-                    // chain, and the wallet still would not build or accept
-                    // the transaction. Retry with backoff.
+                } else {
+                    // Funded, unlocked, block on the main chain, and the
+                    // wallet still would not build, sign or accept the
+                    // transaction. That is worth shouting about; it is not
+                    // grounds to keep the money.
+                    //
+                    // There is deliberately no give-up branch. It used to
+                    // forfeit the debt after 1440 of these, which is not a
+                    // decidable state -- it is a count of how long we have
+                    // been asking, and it was reached by misclassifying a
+                    // funding shortfall as a fault. A debt that is never
+                    // written off cannot be written off wrongly. The payment
+                    // stays in flight, keeps its ledger entry, keeps being
+                    // reported in pool_status.json as owed, and keeps
+                    // retrying once a minute until the operator or the wallet
+                    // makes it possible.
                     r.retries++;
                     r.retryAfter = GetTime() + 60;
                     ReschedulePayment(r);
-                    LogPrint("payout", "[payout] FAILED to pay %s to %s"
-                             " (%s spendable, wallet unlocked) -- retry %d/1440"
-                             " in 60s\n",
-                             FormatMoney(r.amount).c_str(), r.address.c_str(),
-                             FormatMoney(nAfter).c_str(), r.retries);
-                } else {
-                    // 1440 failures that were each individually impossible to
-                    // explain by maturity, funding, the wallet being locked, or
-                    // a reorg. That is the only state in which this payment can
-                    // be called impossible rather than merely blocked.
-                    const std::string reason = strprintf(
-                        "wallet refused this payment %d times while funded"
-                        " (%s spendable), unlocked, and block %d on the main"
-                        " chain", r.retries, FormatMoney(nAfter).c_str(),
-                        r.blockHeight);
-                    LogPrint("payout", "[payout] GIVING UP on %s to %s: %s"
-                             " -- payment forfeited\n",
-                             FormatMoney(r.amount).c_str(), r.address.c_str(),
-                             reason.c_str());
-                    // Write the debt off in the ledger as well as dropping the
-                    // in-flight record. Under the survives-until-fully-paid
-                    // model the entry would otherwise never complete, and every
-                    // later flush would explode it again -- a forfeit that
-                    // silently becomes an infinite retry. The round proof is
-                    // told too, so the public record does not keep claiming the
-                    // address is owed.
-                    ForfeitPayoutRecipient(r, reason);
-                    ForgetPayment(r);
+                    // Every minute at first, then hourly: a fault that is not
+                    // going away must not turn the log into one line a minute
+                    // forever, and must not go quiet either.
+                    if (r.retries <= 5 || r.retries % 60 == 0)
+                        LogPrint("payout", "[payout] FAILED to pay %s to %s"
+                                 " (%s spendable, wallet unlocked, block %d on"
+                                 " the main chain) -- attempt %d, retrying in"
+                                 " 60s; this debt stands until it is paid\n",
+                                 FormatMoney(r.amount).c_str(), r.address.c_str(),
+                                 FormatMoney(nAfter).c_str(), r.blockHeight,
+                                 r.retries);
                 }
             }
         }
@@ -2850,13 +2927,19 @@ static void PayoutTestRemoveDir(const std::string& dir)
     }
     RemoveDirectoryA(dir.c_str());
 #else
+    // One level of recursion, because CreateTransaction() opens a Berkeley DB
+    // environment and BDB puts its logs in a "database" subdirectory. Without
+    // this the scratch directory cannot be removed and the test leaves one
+    // behind on every run, which is what d5b3503 was for.
     DIR* d = opendir(dir.c_str());
     if (d) {
         struct dirent* ent;
         while ((ent = readdir(d)) != NULL) {
             std::string name = ent->d_name;
             if (name == "." || name == "..") continue;
-            unlink((dir + "/" + name).c_str());
+            const std::string path = dir + "/" + name;
+            if (unlink(path.c_str()) != 0)
+                PayoutTestRemoveDir(path);
         }
         closedir(d);
     }
@@ -2942,6 +3025,48 @@ static std::string PayoutTestAddWalletTx(const std::string& comment,
     return hash.GetHex();
 }
 
+// A real key, in the keystore, so a test wallet's outputs are IsMine() and can
+// actually be signed -- both are needed for CreateTransaction to build the
+// large transaction the fee schedule is being tested against. Inserted
+// straight into the key maps rather than through AddKey(), which would also
+// write it to a wallet.dat.
+static std::string PayoutTestAddSpendableKey()
+{
+    CKey key;
+    key.MakeNewKey();
+    std::vector<unsigned char> vchPubKey = key.GetPubKey();
+    CRITICAL_BLOCK(cs_mapKeys)
+    {
+        mapKeys[vchPubKey]                = key.GetPrivKey();
+        mapPubKeys[Hash160(vchPubKey)]    = vchPubKey;
+    }
+    return Hash160ToAddress(Hash160(vchPubKey));
+}
+
+// Give the wallet `n` spendable outputs of `each`, one transaction apiece --
+// which is the granularity SelectCoins works at, and therefore the number of
+// inputs the transaction spending them will carry.
+//
+// Deliberately not coinbases: a null prevout would make CWalletTx::GetCredit
+// return 0 until COINBASE_SPENDABLE_DEPTH, and there is no chain here.
+static void PayoutTestFundWallet(int n, int64 each, const std::string& address)
+{
+    const CScript sc = PayoutTestScriptFor(address);
+    CRITICAL_BLOCK(cs_mapWallet)
+    {
+        for (int i = 0; i < n; i++) {
+            CWalletTx wtx;
+            wtx.vin.resize(1);
+            wtx.vin[0].prevout.hash = (uint64)(i + 1);
+            wtx.vin[0].prevout.n    = 0;
+            wtx.vout.resize(1);
+            wtx.vout[0].nValue       = each;
+            wtx.vout[0].scriptPubKey = sc;
+            mapWallet[wtx.GetHash()] = wtx;
+        }
+    }
+}
+
 // Defined below with the event loop; the status-file publication test drives
 // it directly.
 static void RefreshStats(int blocksFoundThisSession, uint64 roundShareTotal);
@@ -2967,11 +3092,32 @@ int RunPayoutLedgerSelfTest()
 #else
     unsigned long pid = (unsigned long)getpid();
 #endif
-    std::string dir = strprintf("payout_selftest_%d_%lu", (int)GetTime(), pid);
+    // Absolute, and the process moves into it.
+    //
+    // Both are needed by the CreateTransaction section: Berkeley DB resolves
+    // its log directory relative to the environment home, so a relative
+    // datadir makes dbenv.open() fail with DB_RUNRECOVERY; and BDB opens
+    // db.log relative to the working directory, which is src/ during `make
+    // tests`. Moving in puts it inside the scratch directory, where it dies
+    // with everything else.
+    std::string cwd = ".";
+    {
+#ifdef _WIN32
+        char buf[MAX_PATH];
+        if (GetCurrentDirectoryA(MAX_PATH, buf) > 0) cwd = buf;
+#else
+        char buf[4096];
+        if (getcwd(buf, sizeof(buf)) != NULL) cwd = buf;
+#endif
+    }
+    std::string dir = cwd + "/" +
+                      strprintf("payout_selftest_%d_%lu", (int)GetTime(), pid);
 #ifdef _WIN32
     CreateDirectoryA(dir.c_str(), NULL);
+    SetCurrentDirectoryA(dir.c_str());
 #else
     mkdir(dir.c_str(), 0700);
+    if (chdir(dir.c_str()) != 0) { /* stay put; the paths are absolute anyway */ }
 #endif
     strSetDataDir = dir;
     fprintf(stdout, "  scratch datadir: %s\n", dir.c_str());
@@ -3494,7 +3640,141 @@ int RunPayoutLedgerSelfTest()
         strPoolRoundsFile = savedRoundsFile;
     }
 
-    // -- 11. No payout mutex is held across the status-file write ------------
+    // -- 11. A send that failed for want of coin is not a fault --------------
+    //
+    // The defect this replaces: the classifier decided "was this a funding
+    // problem?" by re-reading GetBalance() and allowing amount +
+    // nTransactionFee + CENT. nTransactionFee is 0 on a headless node, and
+    // GetMinFee() is a step function -- 0 below 10,000 bytes, 11 * CENT at it
+    // -- so the allowance was either unnecessary or eleven times too small,
+    // and the ledger's own retry budget then wrote the debt off after 1440
+    // repeats of the same stable condition.
+    //
+    // This is tested against a real wallet and the real CreateTransaction, not
+    // against injected values: the honest question is not "does my enum
+    // switch work" but "does a wallet holding many small coins actually report
+    // the state I think it does". 60 outputs of 1 BTF give ~180 signed P2PKH
+    // bytes each, which is where the fee schedule steps. Nothing is committed
+    // or broadcast -- CreateTransaction only builds.
+    {
+        PayoutTestResetState();
+        CRITICAL_BLOCK(cs_mapWallet)
+            mapWallet.clear();
+
+        // CreateTransaction opens CTxDB("r"), which throws if blkindex.dat is
+        // not there. Create it in the scratch datadir first.
+        bool dbOk = true;
+        try {
+            CTxDB txdb("cr");
+        } catch (const std::exception&) {
+            dbOk = false;
+        }
+        check(dbOk, "a scratch block index exists for CreateTransaction to open");
+
+        const std::string walletAddr = PayoutTestAddSpendableKey();
+        const std::string payee      = PayoutTestAddress(0x77);
+        const CScript     payeeScript = PayoutTestScriptFor(payee);
+
+        // -- the >= 10 KB case: 60 small coins, balance = amount + 0.05 ------
+        PayoutTestFundWallet(60, 1 * COIN, walletAddr);
+        const int64 balance = GetBalance();
+        check(balance == 60 * COIN,
+              "the test wallet holds 60 separately spendable coins");
+
+        const int64 amount = balance - 5 * CENT;
+        // The pre-send gate in PayoutThreadFn passes: the balance covers the
+        // amount and the only fee that can be known in advance.
+        check(balance >= amount + nTransactionFee,
+              "the pre-send gate would let this payment through");
+
+        CWalletTx        wtx;
+        SendMoneyOutcome outcome;
+        int64            feeRequired = 0;
+        WalletSendResult result      = WALLET_SEND_OK;
+        bool             built       = true;
+        if (dbOk) {
+            try {
+                built = CreateTransaction(payeeScript, amount, wtx, feeRequired,
+                                          &result);
+            } catch (const std::exception&) {
+                built = true;   // report as a failure below
+            }
+        }
+        check(dbOk && !built,
+              "a wallet of many small coins cannot fund amount + the fee its"
+              " own size demands");
+        check(dbOk && result == WALLET_SEND_INSUFFICIENT_FUNDS,
+              "the wallet reports that as a funding shortfall, not as a"
+              " failure to build");
+        // GetMinFee(fDiscount) returns 0 below 10,000 bytes and
+        // (1 + bytes/1000) * CENT at or above, so a required fee of 11 CENT or
+        // more IS the statement that the transaction crossed 10 KB. Measured
+        // through the real serialiser rather than asserted.
+        check(dbOk && feeRequired >= 11 * CENT,
+              "the transaction it built crossed 10,000 bytes, so the fee"
+              " stepped from 0 to at least 11 CENT");
+
+        // The defect, stated as arithmetic. The old classifier asked whether
+        // the balance covered amount + nTransactionFee + CENT; it did, so it
+        // called a permanent funding shortfall a fault and started counting
+        // towards a write-off.
+        check(!(balance < amount + nTransactionFee + CENT),
+              "the reserve this replaces would have judged this send a fault");
+        check(balance < amount + feeRequired,
+              "while the fee actually required was more than the balance had");
+
+        outcome.result       = result;
+        outcome.nFeeRequired = feeRequired;
+        check(dbOk && ClassifyPayoutSendFailure(outcome.result, false)
+                          == PAYOUT_FAILURE_DEFER,
+              "a >= 10 KB send that ran out of coin is deferred, not counted"
+              " against the payment");
+
+        // -- the < 10 KB case: no reserve is needed at all -------------------
+        //
+        // The other half of the arithmetic. Below the step the required fee is
+        // 0, so a reserve of any size is a reason to refuse a payment the
+        // wallet could have made -- which is its own way of not paying a
+        // miner.
+        CRITICAL_BLOCK(cs_mapWallet)
+            mapWallet.clear();
+        PayoutTestFundWallet(3, 1 * COIN, walletAddr);
+        const int64 smallAmount = 3 * COIN - 5 * CENT;
+        CWalletTx   smallTx;
+        int64       smallFee    = -1;
+        WalletSendResult smallResult = WALLET_SEND_FAILED;
+        bool        smallBuilt  = false;
+        if (dbOk) {
+            try {
+                smallBuilt = CreateTransaction(payeeScript, smallAmount, smallTx,
+                                               smallFee, &smallResult);
+            } catch (const std::exception&) {
+                smallBuilt = false;
+            }
+        }
+        check(dbOk && smallBuilt && smallResult == WALLET_SEND_OK,
+              "the same balance pays a small transaction with no headroom at"
+              " all");
+        check(dbOk && smallFee == 0,
+              "because below 10,000 bytes the required fee is zero");
+
+        // -- and the classification of every other answer --------------------
+        check(ClassifyPayoutSendFailure(WALLET_SEND_INSUFFICIENT_FUNDS, false)
+                  == PAYOUT_FAILURE_DEFER &&
+              ClassifyPayoutSendFailure(WALLET_SEND_INSUFFICIENT_FUNDS, true)
+                  == PAYOUT_FAILURE_DEFER &&
+              ClassifyPayoutSendFailure(WALLET_SEND_FAILED, true)
+                  == PAYOUT_FAILURE_DEFER,
+              "short of coin or unable to sign is always a deferral");
+        check(ClassifyPayoutSendFailure(WALLET_SEND_FAILED, false)
+                  == PAYOUT_FAILURE_FAULT,
+              "funded and unlocked and still refused is a fault");
+
+        CRITICAL_BLOCK(cs_mapWallet)
+            mapWallet.clear();
+    }
+
+    // -- 12. No payout mutex is held across the status-file write ------------
     //
     // RefreshStats runs on the event loop -- the thread that owns every
     // miner's socket -- and used to hold gStatsMutex and gPayoutLedgerMutex at
@@ -3628,6 +3908,19 @@ int RunPayoutLedgerSelfTest()
     PayoutTestResetState();
     CRITICAL_BLOCK(cs_mapWallet)
         mapWallet.clear();
+    // Out of the scratch directory before removing it, and out of the key maps
+    // too: the keys this test generated are process-local, but leaving them in
+    // the keystore would let a later test think a script was its own.
+    CRITICAL_BLOCK(cs_mapKeys)
+    {
+        mapKeys.clear();
+        mapPubKeys.clear();
+    }
+#ifdef _WIN32
+    SetCurrentDirectoryA(cwd.c_str());
+#else
+    if (chdir(cwd.c_str()) != 0) { /* nothing useful to do about it */ }
+#endif
     PayoutTestRemoveDir(dir);
     strSetDataDir = savedDataDir;
     nBestHeight = savedBestHeight;