payout: keep one retry clock, the one that is used, and persist it

There were two, and each was broken in the opposite direction.

PendingPayout::retryAfter was declared, serialised into pending_payouts.json,
loaded back, and tested at the flush gate -- and the only assignment to it
anywhere in the tree was `= 0`. The gate could never fire. PendingPayout's
`retries` and `deferrals` were the same: carried, never written. Dead fields
next to live ones in a structure this one is easy to reach for by mistake, and
a gate that looks like backoff but is not is worse than no gate at all.

PayingRecord::retryAfter is the clock the payout thread actually uses -- and
SavePayingLocked did not write it. Every restart therefore reset every
backing-off payment to "ready now" and put the entire in-flight set into the
very next 500 ms batch instead of once a minute. On a node restarting in a
loop that is a continuous retry storm against a wallet that has already said
no, each pass taking cs_main.

The dead copy is deleted, along with the flush gate that read it and the one
`retryAfter = 0` in RecordPayoutTxid. The live one is now persisted and
restored. Retry state stays where the failure happens -- per (round,
recipient), which is the granularity SendMoney works at.

On disk, both directions still read:
  - pending_payouts.json no longer has "retryAfter" written into it, and no
    longer reads one. Files that carry it (everything the merged build wrote,
    always as 0) load unchanged, the key simply ignored. A build that expects
    it defaults an absent key to 0, which is the only value it has ever held.
  - paying_payouts.json gains "retryAfter". Older builds ignore unknown keys;
    this build defaults an absent one to 0, which is exactly the behaviour a
    file written without it used to get.

1 files changed, 27 insertions, 16 deletions

diff --git a/src/rpc.cpp b/src/rpc.cpp
index 6f6d101..d655ad5 100644
--- a/src/rpc.cpp
+++ b/src/rpc.cpp
@@ -352,11 +352,13 @@ struct PendingPayout {
     // lets an entry stay on disk until every address in `amounts` is paid,
     // instead of being erased the moment the batch is handed off.
     std::map<std::string, std::string> paidTxids; // address -> txid
-    int                          retries = 0;    // exec-thread only: SendMoney attempts so far
-    int                          deferrals = 0;  // exec-thread only: waits for spendable coin
-    // Upstream's `nextAttempt` is this field under another name; ours is kept
-    // so the retry/deferral policy above stays coherent with it.
-    int64                        retryAfter = 0; // exec-thread only: unix time, 0 = ready now
+    // Retry and backoff state does NOT live here. It used to be declared here
+    // as well as on PayingRecord, and the copy on this side was never written:
+    // `retryAfter` was declared, serialised, loaded and tested at the flush
+    // gate, and the only assignment to it anywhere was `= 0`, so the gate could
+    // not fire. `retries` and `deferrals` were the same story. The real state
+    // is per (round, recipient) -- which is the granularity a payment can fail
+    // at -- and it lives on PayingRecord, which is now what persists it.
 };
 static std::vector<PendingPayout> gPendingPayouts;
 static std::mutex                 gPayoutLedgerMutex;
@@ -597,7 +599,11 @@ static VersionedBlob SerializePendingPayoutsLocked()
         obj["blockHeight"]    = pp.blockHeight;
         obj["blockHash"]      = pp.blockHash.ToString();
         obj["matureAtHeight"] = pp.matureAtHeight;
-        obj["retryAfter"]     = pp.retryAfter;
+        // No retryAfter: it was never written with anything but zero, and the
+        // retry clock that is actually used is on PayingRecord. Files that
+        // still carry the key load fine (it is ignored), and a build that
+        // expects it defaults an absent one to 0, which means "ready now" --
+        // the same answer it always got.
         nlohmann::json amounts = nlohmann::json::object();
         for (const auto& kv : pp.amounts)
             amounts[kv.first] = kv.second;
@@ -655,10 +661,9 @@ static void LoadPendingPayouts()
             // still COINBASE_MATURITY.
             if (pp.blockHeight == 0 && pp.matureAtHeight > COINBASE_MATURITY)
                 pp.blockHeight = pp.matureAtHeight - COINBASE_MATURITY;
-            // Absent in every file written before this merge; default 0 means
-            // "ready now", which is the right answer for a restored entry.
-            if (obj.contains("retryAfter") && obj["retryAfter"].is_number())
-                pp.retryAfter = obj["retryAfter"].get<int64>();
+            // "retryAfter" is deliberately not read. Files written by the
+            // merged build carry it, always as 0, and there is nowhere left to
+            // put it -- the retry clock is PayingRecord's.
             for (auto it = obj["amounts"].begin(); it != obj["amounts"].end(); ++it)
                 pp.amounts[it.key()] = it.value().get<int64>();
             if (obj.contains("paidTxids") && obj["paidTxids"].is_object())
@@ -777,6 +782,11 @@ static VersionedBlob SerializePayingLocked()
         obj["amount"]         = r.amount;
         obj["retries"]        = r.retries;
         obj["deferrals"]      = r.deferrals;
+        // The live retry clock, which used not to be written at all: every
+        // restart handed every backing-off payment a retryAfter of 0 and put
+        // the whole set into the very next 500 ms batch, so a node restarted
+        // in a loop retried continuously instead of once a minute.
+        obj["retryAfter"]     = r.retryAfter;
         arr.push_back(obj);
     }
     VersionedBlob v;
@@ -818,6 +828,10 @@ static void LoadPaying()
                 r.retries = obj["retries"].get<int>();
             if (obj.contains("deferrals") && obj["deferrals"].is_number())
                 r.deferrals = obj["deferrals"].get<int>();
+            // Absent in files written before this change; 0 means ready now,
+            // which is exactly the behaviour those files used to get.
+            if (obj.contains("retryAfter") && obj["retryAfter"].is_number())
+                r.retryAfter = obj["retryAfter"].get<int64>();
             // Collapse duplicates as they are read. A build without the
             // ReconcilePayouts seeding below wrote a second (and after another
             // restart, a third) byte-identical record for every unpaid
@@ -834,8 +848,9 @@ static void LoadPaying()
                     break;
                 }
             if (dup) {
-                dup->retries   = std::max(dup->retries, r.retries);
-                dup->deferrals = std::max(dup->deferrals, r.deferrals);
+                dup->retries    = std::max(dup->retries, r.retries);
+                dup->deferrals  = std::max(dup->deferrals, r.deferrals);
+                dup->retryAfter = std::max(dup->retryAfter, r.retryAfter);
                 LogPrint("payout", "[payout] collapsing duplicate in-flight"
                          " record for %s to %s -- one payment, one record\n",
                          FormatMoney(r.amount).c_str(), r.address.c_str());
@@ -1505,12 +1520,9 @@ static void FlushMaturePayouts()
         }
         if (voided) ledgerSnap = SerializePendingPayoutsLocked();
 
-        int64 now = GetTime();
         for (const PendingPayout& pp : gPendingPayouts) {
             if (nBestHeight < pp.matureAtHeight)
                 continue;
-            if (pp.retryAfter > now)
-                continue;
             if (PayoutFullyPaid(pp))
                 continue;
             // An entry with neither a hash nor a height cannot be told apart
@@ -1658,7 +1670,6 @@ static void RecordPayoutTxid(const PayingRecord& r, const std::string& txid)
                                  r.blockHeight, r.blockHash))
                 continue;
             it->paidTxids[r.address] = txid;
-            it->retryAfter = 0;
             if (PayoutFullyPaid(*it)) {
                 LogPrint("payout", "[payout] payout for block %d complete --"
                          " removing from ledger\n", it->blockHeight);