fix: keep a pending ProUpServTx across a registrar update that keeps the operator key

src/test/evo_deterministicmns_tests.cpp

@@ -1340,7 +1340,7 @@ void FuncTestMempoolProTxKeyChangedConflictChain(TestChainSetup& setup)
     CKey ownerKey;
     CBLSSecretKey operatorKey;
     // Only the resulting proTxHash matters here; the registration never has to be mined because
-    // none of the paths under test consult the masternode list for a ProUpServ payload.
+    // a pending ProUpServ of a masternode missing from the list is evicted by any operator key change.
     auto tx_reg = CreateProRegTx(chainman, utxos, 1, scriptPayout, setup.coinbaseKey, ownerKey, operatorKey);
     const uint256 proTxHash = tx_reg.GetHash();
 
@@ -1538,6 +1538,43 @@ void FuncTestMempoolProRegReplacementUpdateConflict(TestChainSetup& setup)
         testPool.removeForBlock(connected, tip_height() + 1);
         BOOST_CHECK_EQUAL(testPool.size(), 0U);
     }
+
+    // The replacement is not the only confirmed transaction that can leave updates of the MN
+    // unmineable: so does a ProUpReg changing the operator key, and a spend of the collateral.
+    auto tx_up_reg = CreateProUpRegTx(chainman, utxos, proTxHash, ownerKey, operatorKey.GetPublicKey(),
+                                      ownerKey.GetPubKey().GetID(), scriptPayout, setup.coinbaseKey);
+    auto tx_up_reg_new_key = CreateProUpRegTx(chainman, utxos, proTxHash, ownerKey, operatorKey2.GetPublicKey(),
+                                              ownerKey.GetPubKey().GetID(), scriptPayout, setup.coinbaseKey);
+    auto tx_up_rev = CreateProUpRevTx(chainman, utxos, proTxHash, operatorKey, setup.coinbaseKey);
+
+    CMutableTransaction tx_spend_other;
+    tx_spend_other.vin.emplace_back(COutPoint(tx_collateral.GetHash(), collateralOutpoint.n + 1));
+    tx_spend_other.vout.emplace_back(0, CScript() << OP_RETURN);
+
+    CMutableTransaction tx_spend_collateral;
+    tx_spend_collateral.vin.emplace_back(collateralOutpoint);
+    tx_spend_collateral.vout.emplace_back(0, CScript() << OP_RETURN);
+
+    {
+        LOCK2(cs_main, testPool.cs);
+        // ProUpServ and ProUpRev are signed by the operator key, so they stay valid across a
+        // confirmed ProUpReg that keeps it, and across a spend of anything but the collateral.
+        testPool.addUnchecked(entry.FromTx(tx_up_serv));
+        testPool.addUnchecked(entry.FromTx(tx_up_rev));
+        testPool.removeForBlock({MakeTransactionRef(tx_up_reg), MakeTransactionRef(tx_spend_other)}, tip_height() + 1);
+        BOOST_CHECK_EQUAL(testPool.size(), 2U);
+
+        testPool.removeForBlock({MakeTransactionRef(tx_up_reg_new_key)}, tip_height() + 1);
+        BOOST_CHECK_EQUAL(testPool.size(), 0U);
+
+        // Spending the collateral removes the MN, and no update of any kind can be mined after that.
+        testPool.addUnchecked(entry.FromTx(tx_up_serv));
+        testPool.addUnchecked(entry.FromTx(tx_up_reg));
+        testPool.addUnchecked(entry.FromTx(tx_up_rev));
+        BOOST_CHECK_EQUAL(testPool.size(), 3U);
+        testPool.removeForBlock({MakeTransactionRef(tx_spend_collateral)}, tip_height() + 1);
+        BOOST_CHECK_EQUAL(testPool.size(), 0U);
+    }
 }
 
 void FuncVerifyDB(TestChainSetup& setup)

src/txmempool.cpp

@@ -643,6 +643,10 @@ void CTxMemPool::addUncheckedProTx(indexed_transaction_set::iterator& newit, con
     } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_SERVICE) {
         auto proTx = *Assert(GetTxPayload<CProUpServTx>(tx));
         mapProTxRefs.emplace(proTx.proTxHash, tx_hash);
+        // Without a masternode to take the key from, any registrar update evicts it
+        if (auto dmn = m_dmnman.GetListAtChainTip().GetMN(proTx.proTxHash)) {
+            newit->validForProTxKey = ::SerializeHash(dmn->pdmnState->pubKeyOperator);
+        }
         for (const auto& entry : proTx.netInfo->GetEntries()) {
             mapProTxAddresses.emplace(entry, tx_hash);
         }

test/functional/feature_masternode_shares.py

@@ -1198,6 +1198,10 @@ def submit_with_sigs(sigs):
             assert_equal(node.protx("info", protx_hash4, node.getbestblockhash())["state"], state_at_snapshot)
 
         self.log.info("A reorg across a dissolution restores the masternode and its shares")
+        registrar_fee, share_fee = node.getnewaddress(), node.getnewaddress()
+        node.sendmany("", {registrar_fee: 1, share_fee: 1})
+        self.bump_mocktime(10 * 60 + 1)
+        self.generate(node, 1, sync_fun=self.no_op)
         info_before_dissolve = node.protx("info", protx_hash4)
         dissolve_txid4 = node.protx("shared_dissolve", protx_hash4, 0, DISSOLVE_FEE)
         self.bump_mocktime(10 * 60 + 1)
@@ -1210,9 +1214,18 @@ def submit_with_sigs(sigs):
         # the disconnected dissolution returns to the mempool: the restored masternode makes it
         # valid again, so it is not lost by the reorg
         assert dissolve_txid4 in node.getrawmempool()
+        # the restored masternode can be updated again, but the dissolution block was mined without
+        # these updates: once it is reconnected they can never be mined and must not stay behind
+        prepared = node.protx("shared_update_registrar_prepare", protx_hash4, "", node.getnewaddress(), registrar_fee)
+        registrar_sigs = node.protx("shared_sign", prepared["tx"])["signatures"]
+        registrar = node.protx("shared_combine", prepared["tx"], registrar_sigs)
+        pending_txids = {node.sendrawtransaction(registrar),
+                         node.protx("shared_update_share", protx_hash4, 0, node.getnewaddress(), share_fee)}
+        assert pending_txids.issubset(node.getrawmempool())
         node.reconsiderblock(dissolve_block)
         assert_raises_rpc_error(None, None, node.protx, "info", protx_hash4)
         assert dissolve_txid4 not in node.getrawmempool()
+        assert pending_txids.isdisjoint(node.getrawmempool())
         assert_equal(node.masternodelist(), {})
 
         self.log.info("Every participant's principal was refunded by the dissolutions")