payout: say why a grandfathered pair is safe, since it is not the orphan check

The comment on FindRecordedPoolPayout justified the legacy fallback by
saying that where two pre-upgrade rounds share a height, the losing
one's ledger entry is voided by the orphan check before it can be paid.

That check is PayoutBlockStillOnChain(), which returns true for a zero
hash -- and every pre-upgrade entry has a zero hash, because they were
written before payouts recorded one. So it is a no-op for exactly the
population the fallback exists to serve. The stated reason for a
safety property was not the reason.

The property does hold, by a different mechanism. SamePayoutRound()
treats two zero-hash entries at the same height as the same round, so
the pair is indistinguishable to every consumer, not just to this one:
FlushMaturePayouts explodes one and skips the other as already queued,
PaymentInFlightLocked refuses a second record for that (round, address),
and RecordPayoutTxid settles against the first match. The pair presents
as one debt and can be paid at most once. The cost of that -- only one
of a genuine same-height pair would ever be paid -- is now written down
too, rather than left as an implication.

Both halves are asserted rather than reasoned about: the self-test
checks that the orphan check passes a zero hash, and that two identical
zero-hash entries at one height produce exactly one in-flight record.

Property: the comment describes the mechanism that is actually
load-bearing, and a test fails if either half of it stops being true.

1 files changed, 53 insertions, 5 deletions

diff --git a/src/rpc.cpp b/src/rpc.cpp
index 7b45fea..7ea0597 100644
--- a/src/rpc.cpp
+++ b/src/rpc.cpp
@@ -1379,11 +1379,32 @@ static void MarkPoolRoundForfeited(const std::string& id,
 //
 // What is left unresolved by construction is two pre-upgrade rounds at the
 // SAME height paying the same address the same amount, since the comment
-// carries only the height. That needs a reorg, and the losing round's ledger
-// entry is voided by the orphan check before it can be paid, so the pair
-// cannot both be live. Where the evidence genuinely runs out the fallback
-// simply does not match, and the payment is made -- paying a debt we cannot
-// prove was settled is the correct default for a pool.
+// carries only the height. That needs a reorg.
+//
+// This used to say the losing twin's ledger entry is voided by the orphan
+// check before it can be paid. That is wrong: PayoutBlockStillOnChain()
+// returns true for a zero hash, and EVERY pre-upgrade entry has a zero hash,
+// so the orphan check is a no-op for exactly the population this fallback
+// serves.
+//
+// The outcome is still safe, by a different mechanism. SamePayoutRound()
+// treats two zero-hash entries at the same height as the SAME round, so the
+// pair is indistinguishable everywhere, not only here: FlushMaturePayouts
+// explodes one of them and skips the other as already queued,
+// PaymentInFlightLocked refuses a second record for that (round, address), and
+// RecordPayoutTxid settles against the first match. The pair therefore
+// presents as ONE debt and can be paid at most once -- which is the direction
+// that matters for a fallback whose failure mode is treating one payment as
+// proof of two.
+//
+// The cost of that mechanism, stated plainly: only one of a genuine
+// same-height pair would ever be paid. It takes a reorg AND both rounds
+// predating hash keying, and no file this tree has written since can produce
+// one, so it is a property of history rather than a live risk.
+//
+// Where the evidence genuinely runs out the fallback simply does not match,
+// and the payment is made -- paying a debt we cannot prove was settled is the
+// correct default for a pool.
 enum PayoutProbeResult {
     PAYOUT_PROBE_NONE = 0,   // the wallet holds no record of this payment
     PAYOUT_PROBE_STRUCTURED, // matched this build's pool_payout_* metadata
@@ -3594,6 +3615,33 @@ int RunPayoutLedgerSelfTest()
               PayoutRoundIdentifiable(7, 0),
               "an entry with neither hash nor height is not adjudicable");
 
+        // What actually makes a grandfathered same-height pair safe, since the
+        // comment on FindRecordedPoolPayout used to claim it was the orphan
+        // check. It is not: the orphan check passes every zero-hash entry, and
+        // every pre-upgrade entry has a zero hash.
+        check(PayoutBlockStillOnChain(0),
+              "the orphan check passes every grandfathered entry, so it cannot"
+              " be what voids a losing twin");
+        PayoutTestResetState();
+        remove(ledgerPath.c_str());
+        remove(payingPath.c_str());
+        {
+            std::lock_guard<std::mutex> lk(gPayoutLedgerMutex);
+            PendingPayout twin;
+            twin.blockHeight = 300; twin.blockHash = 0; twin.matureAtHeight = 300;
+            twin.amounts[addrA] = 1 * COIN;
+            gPendingPayouts.push_back(twin);
+            gPendingPayouts.push_back(twin);   // the losing side of a reorg
+        }
+        nBestHeight = 5000;
+        FlushMaturePayouts();
+        {
+            std::lock_guard<std::mutex> lk(gPayoutExecMutex);
+            check(gPaying.size() == 1,
+                  "two grandfathered rounds at one height are one round, so the"
+                  " pair presents as one debt and is paid at most once");
+        }
+
         // End to end: recording a payment against one grandfathered round must
         // not settle the other's debt.
         PayoutTestResetState();