payout: require a confirmation before reading a pre-upgrade comment as payment

FindRecordedPoolPayout's legacy branch matched any wallet transaction
carrying this height's payout comment and an output of exactly this
amount to this address, and never asked whether that transaction was in
the chain. A pre-upgrade payout that never confirmed is evidence of
NON-payment, and it was being read as payment: the recipient is dropped
from the in-flight list, a txid is recorded against the ledger entry,
and the debt retires having never been settled. The miner is not paid
and nothing on disk says they were owed.

That is not hypothetical here. This tree has had a 280-block reorg (see
the note in FlushMaturePayouts). A payout invalidated by one is still
sitting in mapWallet with its comment intact, and the first boot after
this upgrade -- the boot where the binary is swapped after an unclean
shutdown with payments in flight -- is precisely when the legacy probe
runs against it.

The structured branch is deliberately left alone. This process sent
those transactions moments earlier, and a send that failed leaves
nothing behind: SendMoney calls RollbackTransactionSpent, which calls
EraseFromWallet. Depth there would only reject payments that really were
made.

The check cannot be done inside the wallet scan.
GetDepthInMainChain() reads mapBlockIndex and pindexBest, which belong
to cs_main, and cs_main is taken before cs_mapWallet everywhere else in
this tree -- CreateTransaction, ConnectBlock -- so testing it under
cs_mapWallet would invert the lock order in the payout thread. Comment
matches are collected in the scan and adjudicated afterwards under
cs_main then cs_mapWallet, in that order. That also fixes a second
thing: the old code stopped at the first comment match, so one
unconfirmed transaction could hide a confirmed one behind it.

A rejection logs the txid, height, amount and depth, and says the
recipient will be paid, because this is the one decision where being
wrong is unrecoverable in the direction that costs a miner money.

Property: a payment is only ever inferred from a pre-upgrade comment
when the transaction carrying it is in the main chain.

1 files changed, 155 insertions, 9 deletions

diff --git a/src/rpc.cpp b/src/rpc.cpp
index 06de7f2..b0436ba 100644
--- a/src/rpc.cpp
+++ b/src/rpc.cpp
@@ -1339,7 +1339,16 @@ static PayoutProbeResult FindRecordedPoolPayout(const std::string& id,
         want << OP_DUP << OP_HASH160 << h160 << OP_EQUALVERIFY << OP_CHECKSIG;
     const std::string comment = PayoutComment(blockHeight);
 
-    std::string legacyTxid;
+    // Candidate legacy matches, adjudicated after cs_mapWallet is released.
+    //
+    // The depth check cannot happen inside the scan: GetDepthInMainChain()
+    // reads mapBlockIndex and pindexBest, which belong to cs_main, and cs_main
+    // is taken BEFORE cs_mapWallet everywhere else in this tree
+    // (CreateTransaction, ConnectBlock). Taking it the other way round here
+    // would be a lock-order inversion in the payout thread. There is at most a
+    // handful of these: a candidate has to carry this exact height's comment
+    // and an output of exactly this amount to this address.
+    std::vector<uint256> legacyCandidates;
 
     // One pass, not two: this scan runs under cs_mapWallet, which the whole
     // node contends on, and the merge is already paying for it once per record.
@@ -1365,27 +1374,63 @@ static PayoutProbeResult FindRecordedPoolPayout(const std::string& id,
                 return PAYOUT_PROBE_STRUCTURED;
             }
 
-            if (!fHaveScript || !legacyTxid.empty())
+            if (!fHaveScript)
                 continue;
             // The comment is written only by the payout path, so its presence
             // already identifies the transaction as one of ours.
             map<string, string>::const_iterator ci = wtx.mapValue.find("comment");
             if (ci == wtx.mapValue.end() || ci->second != comment)
                 continue;
-            std::string txid = wtx.GetHash().GetHex();
-            if (excluded.count(txid))
+            if (excluded.count(wtx.GetHash().GetHex()))
                 continue;   // already credited to a different debt
             foreach (const CTxOut& txout, wtx.vout)
                 if (txout.nValue == amount && txout.scriptPubKey == want) {
-                    legacyTxid = txid;
+                    legacyCandidates.push_back(it->first);
                     break;
                 }
         }
     }
 
-    if (!legacyTxid.empty()) {
-        txidRet = legacyTxid;
-        return PAYOUT_PROBE_LEGACY;
+    // A legacy match must have CONFIRMED.
+    //
+    // The structured path does not need this and deliberately does not have
+    // it: this process sent that transaction moments ago, and a send that
+    // failed leaves nothing behind, because SendMoney calls
+    // RollbackTransactionSpent, which calls EraseFromWallet.
+    //
+    // The legacy path is the opposite case in every respect. Those
+    // transactions are days or weeks old, so whether they confirmed is a
+    // settled fact rather than an open question, and a pre-upgrade payout that
+    // never confirmed is evidence of NON-payment about to be read as payment.
+    // This chain has had a 280-block reorg (see FlushMaturePayouts): a payout
+    // invalidated by it is still sitting in mapWallet with its comment intact,
+    // and the first boot after this upgrade is exactly when it gets matched.
+    // The cost of being wrong here is a miner who is never paid and no record
+    // that they were owed.
+    CRITICAL_BLOCK(cs_main)
+    CRITICAL_BLOCK(cs_mapWallet)
+    {
+        for (const uint256& hash : legacyCandidates) {
+            map<uint256, CWalletTx>::const_iterator it = mapWallet.find(hash);
+            if (it == mapWallet.end())
+                continue;   // erased under us; it is not evidence of anything
+            int depth = it->second.GetDepthInMainChain();
+            if (depth < 1) {
+                // Loud: this is the difference between a miner having been
+                // paid and a miner about to be paid twice, decided on a fact
+                // that is easy to check and impossible to recover afterwards.
+                LogPrint("payout", "[payout] wallet tx %s carries the payout"
+                         " comment for height %d and an output of exactly %s to"
+                         " %s, but it is NOT in the main chain (depth %d) --"
+                         " refusing to read it as payment; this recipient will"
+                         " be paid\n",
+                         hash.GetHex().c_str(), blockHeight,
+                         FormatMoney(amount).c_str(), address.c_str(), depth);
+                continue;
+            }
+            txidRet = hash.GetHex();
+            return PAYOUT_PROBE_LEGACY;
+        }
     }
     return PAYOUT_PROBE_NONE;
 }
@@ -2997,13 +3042,47 @@ static CScript PayoutTestScriptFor(const std::string& address)
     return sc;
 }
 
+// Block index entries fabricated by PayoutTestAddWalletTx, so they can be
+// freed and unhooked when the test finishes. gPayoutTestChainTip is the end of
+// that chain: blocks are linked onto it rather than each replacing the tip,
+// because IsInMainChain() is "pnext is set, or I am the tip".
+static std::vector<CBlockIndex*> gPayoutTestBlockIndexes;
+static CBlockIndex*              gPayoutTestChainTip = NULL;
+
+static void PayoutTestClearBlockIndex()
+{
+    CRITICAL_BLOCK(cs_main)
+    {
+        for (CBlockIndex* p : gPayoutTestBlockIndexes) {
+            for (std::map<uint256, CBlockIndex*>::iterator it = mapBlockIndex.begin();
+                 it != mapBlockIndex.end(); ) {
+                if (it->second == p) mapBlockIndex.erase(it++);
+                else ++it;
+            }
+            delete p;
+        }
+        gPayoutTestBlockIndexes.clear();
+        gPayoutTestChainTip = NULL;
+        pindexBest = NULL;
+    }
+}
+
 // Put a transaction in the wallet map that looks exactly like one this pool
 // sent. `nonce` only exists to keep the hashes distinct.
+//
+// fConfirmed decides what GetDepthInMainChain() will answer for it, which is
+// what the legacy probe now gates on. A confirmed one is given a block index
+// entry of its own: an empty merkle branch hashes to the transaction itself,
+// so a block whose merkle root IS that hash accepts it at index 0, and
+// pointing pindexBest at that block puts it in the main chain one deep. An
+// unconfirmed one keeps hashBlock 0, which is depth 0 -- the shape a payout
+// left behind by a reorg has.
 static std::string PayoutTestAddWalletTx(const std::string& comment,
                                          const std::string& address,
                                          int64 amount,
                                          int nonce,
-                                         const std::string& structuredId)
+                                         const std::string& structuredId,
+                                         bool fConfirmed = true)
 {
     CWalletTx wtx;
     wtx.vin.resize(1);
@@ -3020,6 +3099,32 @@ static std::string PayoutTestAddWalletTx(const std::string& comment,
         wtx.mapValue["pool_payout_amount"]  = strprintf("%lld", amount);
     }
     uint256 hash = wtx.GetHash();
+    if (fConfirmed) {
+        // A block hash distinct from the transaction's own.
+        uint256 blockHash = hash;
+        ((unsigned char*)&blockHash)[0] ^= 0xff;
+        CRITICAL_BLOCK(cs_main)
+        {
+            CBlockIndex* pindex = new CBlockIndex();
+            pindex->hashMerkleRoot = hash;
+            // Linked onto the end of the fabricated chain rather than simply
+            // becoming the new tip: IsInMainChain() is "pnext is set, or I am
+            // the tip", so a second confirmed transaction would otherwise take
+            // the first one out of the chain again.
+            pindex->pprev   = gPayoutTestChainTip;
+            pindex->nHeight = gPayoutTestChainTip ? gPayoutTestChainTip->nHeight + 1 : 1;
+            if (gPayoutTestChainTip)
+                gPayoutTestChainTip->pnext = pindex;
+            gPayoutTestChainTip = pindex;
+            std::map<uint256, CBlockIndex*>::iterator mi =
+                mapBlockIndex.insert(std::make_pair(blockHash, pindex)).first;
+            pindex->phashBlock = &mi->first;
+            pindexBest = pindex;
+            gPayoutTestBlockIndexes.push_back(pindex);
+        }
+        wtx.hashBlock = blockHash;
+        wtx.nIndex    = 0;
+    }
     CRITICAL_BLOCK(cs_mapWallet)
         mapWallet[hash] = wtx;
     return hash.GetHex();
@@ -3497,6 +3602,46 @@ int RunPayoutLedgerSelfTest()
         check(FindRecordedPoolPayout(id, height, addrB, amount, none, found)
                   == PAYOUT_PROBE_STRUCTURED && found == structuredTxid,
               "structured payout metadata is matched in preference to a comment");
+
+        // A comment-matching transaction that never made it into the chain is
+        // evidence of NON-payment. This tree has seen a 280-block reorg; a
+        // pre-upgrade payout invalidated by one is still in mapWallet with its
+        // comment intact, and the first boot after the upgrade is exactly when
+        // it would be matched.
+        CRITICAL_BLOCK(cs_mapWallet)
+            mapWallet.clear();
+        PayoutTestClearBlockIndex();
+        const std::string addrC = PayoutTestAddress(0x33);
+        PayoutTestAddWalletTx(comment, addrC, amount, 4, "", false);
+        check(FindRecordedPoolPayout(id, height, addrC, amount, none, found)
+                  == PAYOUT_PROBE_NONE,
+              "a pre-upgrade payout that never confirmed is not read as"
+              " payment");
+
+        // The same transaction, confirmed, is.
+        CRITICAL_BLOCK(cs_mapWallet)
+            mapWallet.clear();
+        const std::string confirmedTxid =
+            PayoutTestAddWalletTx(comment, addrC, amount, 4, "", true);
+        check(FindRecordedPoolPayout(id, height, addrC, amount, none, found)
+                  == PAYOUT_PROBE_LEGACY && found == confirmedTxid,
+              "the same payout, confirmed, is");
+
+        // And an unconfirmed one does not shadow a confirmed one that would
+        // have matched: the probe used to stop at the first comment match.
+        const std::string alsoConfirmed =
+            PayoutTestAddWalletTx(comment, addrC, amount, 6, "", true);
+        PayoutTestAddWalletTx(comment, addrC, amount, 5, "", false);
+        std::set<std::string> spentFirst;
+        spentFirst.insert(confirmedTxid);
+        check(FindRecordedPoolPayout(id, height, addrC, amount, spentFirst, found)
+                  == PAYOUT_PROBE_LEGACY && found == alsoConfirmed,
+              "an unconfirmed transaction does not hide a confirmed one behind"
+              " it");
+
+        CRITICAL_BLOCK(cs_mapWallet)
+            mapWallet.clear();
+        PayoutTestClearBlockIndex();
     }
 
     // -- 9. On-disk compatibility with the live production ledger ------------
@@ -3908,6 +4053,7 @@ int RunPayoutLedgerSelfTest()
     PayoutTestResetState();
     CRITICAL_BLOCK(cs_mapWallet)
         mapWallet.clear();
+    PayoutTestClearBlockIndex();
     // 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.