fix: mine a pending ProUpServTx before a key change of its masternode

doc/release-notes-7857.md

@@ -0,0 +1,13 @@
+Mining and mempool policy
+-------------------------
+
+- When a block template includes an operator key change or revocation for a masternode, it now
+  places the masternode's pending `ProUpServTx` signed with the key being replaced before it. Such
+  a service update could otherwise confirm after the change in the same block and restore the
+  previous operator's service and payout details. A service update that cannot be placed before
+  the change, or that does not fit in the block with it, is left out of that block. (#7857)
+- The mempool now rejects with `protx-dup` a `ProUpServTx` that spends, directly or through other
+  pending transactions, an output of a pending operator key change or revocation, which it can only
+  confirm after, or of a pending asset lock, asset unlock or MNHF signal, which could keep it out of
+  a block. A service update funded from such outputs can be sent once they confirm, signed with the
+  operator key that is current then. (#7857)

src/evo/specialtxman.cpp

@@ -819,6 +819,8 @@ uint256 CSpecialTxProcessor::GetDeterministicMNListHash(gsl::not_null<const CBlo
     return SerializeHash(m_dmnman.GetListForBlock(pindex));
 }
 
+void CSpecialTxProcessor::UpdatedMNListTip(gsl::not_null<const CBlockIndex*> pindex) { m_dmnman.UpdatedBlockTip(pindex); }
+
 bool CSpecialTxProcessor::ProcessSpecialTxsInBlock(Chainstate& chainstate, const CChain& chain, const CBlock& block,
                                                    const CBlockIndex* pindex, SpecialTxRules rules,
                                                    const CCoinsViewCache& view, CAmount blockSubsidy, bool fJustCheck,

src/evo/specialtxman.h

@@ -107,6 +107,9 @@ class CSpecialTxProcessor
     /** Return a canonical hash of the deterministic MN list derived at a block. */
     uint256 GetDeterministicMNListHash(gsl::not_null<const CBlockIndex*> pindex) const;
 
+    /** Passthrough to CDeterministicMNManager::UpdatedBlockTip */
+    void UpdatedMNListTip(gsl::not_null<const CBlockIndex*> pindex);
+
 private:
     bool CheckSpecialTxInner(const CChain* chain, const CTransaction& tx, const CBlockIndex* pindexPrev,
                              SpecialTxRules rules, const CCoinsViewCache& view, const std::optional<CRangesSet>& indexes,

src/node/miner.cpp

@@ -30,6 +30,7 @@
 #include <evo/cbtx.h>
 #include <evo/creditpool.h>
 #include <evo/mnhftx.h>
+#include <evo/providertx.h>
 #include <evo/deterministicmns.h>
 #include <evo/simplifiedmns.h>
 #include <evo/specialtxman.h>
@@ -553,6 +554,16 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
     // This map with signals is used only to find duplicates
     auto signals = m_chainstate.m_special_tx.m_mnhfman->GetSignalsStage(pindexPrev);
     const SpecialTxRules special_tx_rules{GetSpecialTxRules(pindexPrev, m_chainstate.m_chainman)};
+    // Masternodes whose operator key a transaction already in the block changes or revokes. A
+    // ProUpServTx of theirs must not follow: one left out of the key change's package, whether it
+    // could not be put first, did not fit or did not pass TestPackageTransactions() then (which can
+    // change while the template is built), may come up later.
+    std::set<uint256> keyChangedMNs;
+    const auto serviceUpdateTarget = [](const CTransaction& tx) -> std::optional<uint256> {
+        if (tx.nType != TRANSACTION_PROVIDER_UPDATE_SERVICE) return std::nullopt;
+        const auto opt_proTx = GetTxPayload<CProUpServTx>(tx);
+        return opt_proTx ? std::make_optional(opt_proTx->proTxHash) : std::nullopt;
+    };
 
     // mapModifiedTx will store sorted packages after they are modified
     // because some of their txs are already in the block
@@ -659,6 +670,50 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
         onlyUnconfirmed(ancestors);
         ancestors.insert(iter);
 
+        // A pending ProUpServTx signed with the operator key that a key change in this package
+        // replaces must not follow the key change in the block: it would restore the previous
+        // operator's service fields. Mine such updates first. One that cannot be in this block now,
+        // whose package block assembly could not reliably put first (the mempool rejects those on
+        // entry, but a reorg can return such ancestors later), or whose package has an update of a
+        // masternode whose key already changed in the block, is left out: keyChangedMNs keeps it out
+        // of the rest of the block. If those pulled in do not fit, they are all left out instead of
+        // holding the key change back, which pending updates could otherwise do forever.
+        CTxMemPool::setEntries serviceUpdates;
+        for (const auto& entry : ancestors) {
+            for (const auto& update : mempool.GetServiceUpdatesBeforeKeyChange(entry)) {
+                if (inBlock.count(update) || serviceUpdates.count(update)) {
+                    continue;
+                }
+                auto updatePackage{mempool.AssumeCalculateMemPoolAncestors(__func__, *update,
+                                                                           CTxMemPool::Limits::NoLimits(),
+                                                                           /*fSearchForParents=*/false)};
+                onlyUnconfirmed(updatePackage);
+                updatePackage.insert(update);
+                const bool followsKeyChange{std::any_of(updatePackage.begin(), updatePackage.end(),
+                                                        [&](const CTxMemPool::txiter& it) {
+                                                            const auto target{serviceUpdateTarget(it->GetTx())};
+                                                            return target && keyChangedMNs.count(*target);
+                                                        })};
+                if (!followsKeyChange && TestPackageTransactions(updatePackage) &&
+                    !mempool.IsUnorderableServiceUpdate(update->GetTx(), updatePackage)) {
+                    serviceUpdates.insert(updatePackage.begin(), updatePackage.end());
+                }
+            }
+        }
+        if (!serviceUpdates.empty()) {
+            uint64_t extraSize{0};
+            unsigned int extraSigOps{0};
+            for (const auto& entry : serviceUpdates) {
+                if (!ancestors.count(entry)) {
+                    extraSize += entry->GetTxSize();
+                    extraSigOps += entry->GetSigOpCount();
+                }
+            }
+            if (!TestPackage(packageSize + extraSize, packageSigOps + extraSigOps)) {
+                serviceUpdates.clear();
+            }
+        }
+
         // Test if all tx's are Final and safe
         if (!TestPackageTransactions(ancestors)) {
             if (fUsingModified) {
@@ -668,15 +723,38 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
             continue;
         }
 
-        // Package can be added. Sort the entries in a valid order.
+        // Package can be added. Sort the entries in a valid order, the service updates first.
         std::vector<CTxMemPool::txiter> sortedEntries;
-        SortForBlock(ancestors, sortedEntries);
+        SortForBlock(serviceUpdates, sortedEntries);
+        CTxMemPool::setEntries rest;
+        for (const auto& entry : ancestors) {
+            if (!serviceUpdates.count(entry)) {
+                rest.insert(entry);
+            }
+        }
+        std::vector<CTxMemPool::txiter> sortedRest;
+        SortForBlock(rest, sortedRest);
+        sortedEntries.insert(sortedEntries.end(), sortedRest.begin(), sortedRest.end());
+        ancestors.insert(serviceUpdates.begin(), serviceUpdates.end());
 
         auto packageSignals = signals;
+        std::vector<uint256> packageKeyChangedMNs;
         std::vector<CTransactionRef> creditPoolTransactions;
         bool validPackage{true};
         for (const auto& entry : sortedEntries) {
             const auto& tx = entry->GetTx();
+            if (const auto target = serviceUpdateTarget(tx);
+                target && (keyChangedMNs.count(*target) ||
+                           std::find(packageKeyChangedMNs.begin(), packageKeyChangedMNs.end(), *target) !=
+                               packageKeyChangedMNs.end())) {
+                LogPrintf("%s: package tx %s skipped, it would follow a key change of masternode %s\n", __func__,
+                          tx.GetHash().ToString(), target->ToString());
+                validPackage = false;
+                break;
+            }
+            if (const auto target = mempool.GetKeyChangeTarget(*entry)) {
+                packageKeyChangedMNs.push_back(*target);
+            }
             if (std::optional<uint8_t> signal = extractEHFSignal(tx); signal != std::nullopt) {
                 if (!packageSignals.emplace(*signal, 0).second) {
                     LogPrintf("%s: package tx %s skipped due to duplicate EHF signal %d\n", __func__,
@@ -723,6 +801,7 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
         // This transaction will make it in; reset the failed counter.
         nConsecutiveFailed = 0;
         signals = std::move(packageSignals);
+        keyChangedMNs.insert(packageKeyChangedMNs.begin(), packageKeyChangedMNs.end());
 
         for (size_t i = 0; i < sortedEntries.size(); ++i) {
             AddToBlock(sortedEntries[i]);