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.