From f8c0b6d9b477739e0699c4972adba68c5ea20000 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Sun, 23 Aug 2026 07:44:48 +0000 Subject: 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. --- src/rpc.cpp | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-) (limited to 'src/rpc.cpp') 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 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 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(); + // "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(); 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(); if (obj.contains("deferrals") && obj["deferrals"].is_number()) r.deferrals = obj["deferrals"].get(); + // 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(); // 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); -- cgit v1.2.3