From fd47f589d00771d0dcba30c9014dd1f1f64220ba Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 22 Sep 2026 23:00:23 -0500 Subject: [PATCH 1/3] fix: drop stale operator service updates around an operator key change A ProUpServTx is signed with the operator key at the tip, and block validation applies provider transactions in order without re-checking the signer. A service update signed with the old key that confirms after an operator key change (ProUpRegTx or ProUpSharedRegTx with a new key, or ProUpRevTx) in the same block restores the displaced operator's service and payout fields and revives the masternode the change banned. The mempool let both wait together, so an honest miner could build that block (#7582). Keep such a pair out of the mempool: refuse a service update while a key change for its masternode is pending, evict pending service updates when a key change is accepted (except its ancestors, which are always mined first), and reject packages that would pair them. Move the masternode list tip before re-adding disconnected transactions on a reorg, so a returning key change is recognized as one. Consensus is unchanged. The evicted set follows spenders through mapNextTx to closure rather than through the child links, because during a reorg the links from returning transactions to spenders already in the pool are only rebuilt afterwards by UpdateTransactionsFromBlock; following the child links would leave those spenders behind with inputs the pool no longer has. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/evo/specialtxman.cpp | 2 + src/evo/specialtxman.h | 3 + src/test/evo_deterministicmns_tests.cpp | 234 +++++++++++++++++++++++- src/txmempool.cpp | 93 ++++++++++ src/txmempool.h | 14 ++ src/validation.cpp | 15 ++ 6 files changed, 359 insertions(+), 2 deletions(-) diff --git a/src/evo/specialtxman.cpp b/src/evo/specialtxman.cpp index aef331c99d8d..ce3ab0cdb5c3 100644 --- a/src/evo/specialtxman.cpp +++ b/src/evo/specialtxman.cpp @@ -819,6 +819,8 @@ uint256 CSpecialTxProcessor::GetDeterministicMNListHash(gsl::not_null 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, diff --git a/src/evo/specialtxman.h b/src/evo/specialtxman.h index 1481ad01683a..18f48598a2a6 100644 --- a/src/evo/specialtxman.h +++ b/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 pindex) const; + /** Passthrough to CDeterministicMNManager::UpdatedBlockTip */ + void UpdatedMNListTip(gsl::not_null pindex); + private: bool CheckSpecialTxInner(const CChain* chain, const CTransaction& tx, const CBlockIndex* pindexPrev, SpecialTxRules rules, const CCoinsViewCache& view, const std::optional& indexes, diff --git a/src/test/evo_deterministicmns_tests.cpp b/src/test/evo_deterministicmns_tests.cpp index 1d307803eb9f..7c629669abb6 100644 --- a/src/test/evo_deterministicmns_tests.cpp +++ b/src/test/evo_deterministicmns_tests.cpp @@ -97,7 +97,8 @@ static CMutableTransaction CreateProRegTxExternalCollateral(const ChainstateMana } static CMutableTransaction CreateProUpServTx(const ChainstateManager& chainman, SimpleUTXOMap& utxos, const uint256& proTxHash, const CBLSSecretKey& operatorKey, int port, const CScript& scriptOperatorPayout, const CKey& coinbaseKey, - uint16_t version = ProTxVersion::GetMax(!bls::bls_legacy_scheme, /*is_extended_addr=*/false)) + uint16_t version = ProTxVersion::GetMax(!bls::bls_legacy_scheme, /*is_extended_addr=*/false), + CAmount fee = 0) { CProUpServTx proTx; proTx.nVersion = version; @@ -111,6 +112,8 @@ static CMutableTransaction CreateProUpServTx(const ChainstateManager& chainman, tx.nVersion = 3; tx.nType = TRANSACTION_PROVIDER_UPDATE_SERVICE; const auto spent = FundTransaction(chainman, tx, utxos, GetScriptForDestination(PKHash(coinbaseKey.GetPubKey())), 1 * COIN); + // See CreateProUpRegTx: a zero-fee transaction only gets into blocks built by the test itself + tx.vout.back().nValue -= fee; proTx.inputsHash = CalcTxInputsHash(CTransaction(tx)); proTx.sig = operatorKey.Sign(::SerializeHash(proTx), bls::bls_legacy_scheme); SetTxPayload(tx, proTx); @@ -145,7 +148,8 @@ static CMutableTransaction CreateProUpRegTx(const ChainstateManager& chainman, S return tx; } -static CMutableTransaction CreateProUpRevTx(const ChainstateManager& chainman, SimpleUTXOMap& utxos, const uint256& proTxHash, const CBLSSecretKey& operatorKey, const CKey& coinbaseKey) +static CMutableTransaction CreateProUpRevTx(const ChainstateManager& chainman, SimpleUTXOMap& utxos, const uint256& proTxHash, const CBLSSecretKey& operatorKey, const CKey& coinbaseKey, + CAmount fee = 0) { CProUpRevTx proTx; proTx.nVersion = ProTxVersion::GetMax(!bls::bls_legacy_scheme, /*is_extended_addr=*/false); @@ -155,6 +159,8 @@ static CMutableTransaction CreateProUpRevTx(const ChainstateManager& chainman, S tx.nVersion = 3; tx.nType = TRANSACTION_PROVIDER_UPDATE_REVOKE; const auto spent = FundTransaction(chainman, tx, utxos, GetScriptForDestination(PKHash(coinbaseKey.GetPubKey())), 1 * COIN); + // See CreateProUpRegTx: a zero-fee transaction only gets into blocks built by the test itself + tx.vout.back().nValue -= fee; proTx.inputsHash = CalcTxInputsHash(CTransaction(tx)); proTx.sig = operatorKey.Sign(::SerializeHash(proTx), bls::bls_legacy_scheme); SetTxPayload(tx, proTx); @@ -1583,6 +1589,224 @@ void FuncTestMempoolProRegReplacementUpdateConflict(TestChainSetup& setup) } } +// A ProUpServTx is signed with the operator key at the tip. If an operator key change or revocation +// for the same masternode confirms ahead of it in one block, the service update still applies and +// restores the displaced operator's service fields and revives the masternode the change banned. +// The mempool must never hold such a pair: the key change wins regardless of arrival order. +void FuncTestMempoolStaleServiceUpdate(TestChainSetup& setup) +{ + auto& chainman = *Assert(setup.m_node.chainman.get()); + auto& dmnman = *Assert(setup.m_node.dmnman); + auto& mempool = *Assert(setup.m_node.mempool.get()); + auto tip_index = [&] { return WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()); }; + auto sync_dmn_tip = [&] { dmnman.UpdatedBlockTip(tip_index()); }; + + const CScript coinbase_pk = GetScriptForRawPubKey(setup.coinbaseKey.GetPubKey()); + auto utxos = BuildSimpleUtxoMap(setup.m_coinbase_txns); + constexpr CAmount fee{10000}; + const uint16_t version{ProTxVersion::GetMax(!bls::bls_legacy_scheme, /*is_extended_addr=*/false)}; + + CKey ownerKey; + CBLSSecretKey operatorKey; + auto tx_reg = CreateProRegTx(chainman, utxos, /*port=*/1, GenerateRandomAddress(), setup.coinbaseKey, ownerKey, + operatorKey); + const uint256 proTxHash = tx_reg.GetHash(); + // Every transaction below consumes a whole funding output and the fixture has few mature + // coinbases, so split one into small outputs that need no maturity + constexpr int num_split{20}; + CMutableTransaction tx_split; + const auto split_spent = FundTransaction(chainman, tx_split, utxos, coinbase_pk, 2 * COIN); + BOOST_REQUIRE_EQUAL(tx_split.vout.size(), 2U); + BOOST_REQUIRE(tx_split.vout[1].nValue > num_split * 2 * COIN + fee); + for (int i = 0; i < num_split; ++i) { + tx_split.vout[1].nValue -= 2 * COIN; + tx_split.vout.emplace_back(2 * COIN, coinbase_pk); + } + tx_split.vout[1].nValue -= fee; + SignTransaction(tx_split, split_spent, setup.coinbaseKey); + setup.CreateAndProcessBlock({tx_reg, tx_split}, coinbase_pk); + sync_dmn_tip(); + BOOST_REQUIRE(dmnman.GetListAtChainTip().HasMN(proTxHash)); + for (size_t i = 0; i < tx_split.vout.size(); ++i) { + utxos.emplace(COutPoint(tx_split.GetHash(), i), Coin(tx_split.vout[i], /*nHeightIn=*/0, /*fCoinBaseIn=*/false)); + } + + CBLSSecretKey newOperatorKey; + newOperatorKey.MakeNewKey(); + const CKeyID votingKeyID{ownerKey.GetPubKey().GetID()}; + const CScript payout{GenerateRandomAddress()}; + + auto submit = [&](const CMutableTransaction& tx) { + LOCK(cs_main); + return chainman.ProcessTransaction(MakeTransactionRef(tx)); + }; + auto accepted = [&](const CMutableTransaction& tx) { + const auto result = submit(tx); + BOOST_CHECK_MESSAGE(result.m_result_type == MempoolAcceptResult::ResultType::VALID, + "unexpected rejection: " << result.m_state.GetRejectReason()); + return result.m_result_type == MempoolAcceptResult::ResultType::VALID; + }; + auto in_mempool = [&](const CMutableTransaction& tx) { return mempool.exists(tx.GetHash()); }; + auto reset_mempool = [&] { + LOCK(mempool.cs); + for (const auto& info : mempool.infoAll()) { + mempool.removeRecursive(*info.tx, MemPoolRemovalReason::MANUAL); + } + BOOST_REQUIRE_EQUAL(mempool.size(), 0U); + }; + auto make_serv = [&](int port, const CBLSSecretKey& key) { + return CreateProUpServTx(chainman, utxos, proTxHash, key, port, CScript(), setup.coinbaseKey, version, fee); + }; + auto make_reg = [&](const CBLSSecretKey& operator_key) { + return CreateProUpRegTx(chainman, utxos, proTxHash, ownerKey, operator_key.GetPublicKey(), votingKeyID, payout, + setup.coinbaseKey, version, fee); + }; + + auto spend = [&](const CMutableTransaction& parent) { + SimpleUTXOMap parent_coins{{COutPoint(parent.GetHash(), 0), + Coin(parent.vout[0], /*nHeightIn=*/0, /*fCoinBaseIn=*/false)}}; + CMutableTransaction tx; + tx.vin.emplace_back(COutPoint(parent.GetHash(), 0)); + tx.vout.emplace_back(parent.vout[0].nValue - fee, coinbase_pk); + SignTransaction(tx, parent_coins, setup.coinbaseKey); + return tx; + }; + + for (const bool revoke : {false, true}) { + BOOST_TEST_MESSAGE("key change: " << (revoke ? "revocation" : "rotation")); + auto make_key_change = [&] { + return revoke ? CreateProUpRevTx(chainman, utxos, proTxHash, operatorKey, setup.coinbaseKey, fee) + : make_reg(newOperatorKey); + }; + + // Key change first: the old-key service update is refused + { + const auto tx_change = make_key_change(); + const auto tx_serv = make_serv(/*port=*/2, operatorKey); + BOOST_REQUIRE(accepted(tx_change)); + const auto result = submit(tx_serv); + BOOST_CHECK(result.m_result_type == MempoolAcceptResult::ResultType::INVALID); + BOOST_CHECK_EQUAL(result.m_state.GetRejectReason(), "protx-dup"); + BOOST_CHECK(in_mempool(tx_change)); + reset_mempool(); + } + + // Service update first: the key change evicts it + { + const auto tx_serv = make_serv(/*port=*/3, operatorKey); + const auto tx_change = make_key_change(); + BOOST_REQUIRE(accepted(tx_serv)); + BOOST_REQUIRE(accepted(tx_change)); + BOOST_CHECK(!in_mempool(tx_serv)); + BOOST_CHECK(in_mempool(tx_change)); + reset_mempool(); + } + } + + // A key change spending the service update's output is always mined after it, so both stay + { + const auto tx_serv = make_serv(/*port=*/4, operatorKey); + SimpleUTXOMap serv_coins{{COutPoint(tx_serv.GetHash(), 0), + Coin(tx_serv.vout[0], /*nHeightIn=*/0, /*fCoinBaseIn=*/false)}}; + const auto tx_revoke = CreateProUpRevTx(chainman, serv_coins, proTxHash, operatorKey, setup.coinbaseKey, fee); + BOOST_REQUIRE_EQUAL(tx_revoke.vin.size(), 1U); + BOOST_REQUIRE(tx_revoke.vin[0].prevout.hash == tx_serv.GetHash()); + BOOST_REQUIRE(accepted(tx_serv)); + BOOST_REQUIRE(accepted(tx_revoke)); + BOOST_CHECK(in_mempool(tx_serv)); + BOOST_CHECK(in_mempool(tx_revoke)); + reset_mempool(); + } + + // A registrar update keeping the operator key is not a key change: both stay, in either order + { + const auto tx_serv = make_serv(/*port=*/5, operatorKey); + BOOST_REQUIRE(accepted(tx_serv)); + BOOST_REQUIRE(accepted(make_reg(operatorKey))); + BOOST_CHECK(in_mempool(tx_serv)); + reset_mempool(); + + BOOST_REQUIRE(accepted(make_reg(operatorKey))); + BOOST_CHECK(accepted(make_serv(/*port=*/6, operatorKey))); + reset_mempool(); + } + + // Package members are checked against the pool before any of them is added, so the package + // itself must not pair a key change with an old-key service update, directly or by spending one + { + const auto tx_serv = make_serv(/*port=*/8, operatorKey); + SimpleUTXOMap serv_coins{{COutPoint(tx_serv.GetHash(), 0), + Coin(tx_serv.vout[0], /*nHeightIn=*/0, /*fCoinBaseIn=*/false)}}; + const auto tx_revoke = CreateProUpRevTx(chainman, serv_coins, proTxHash, operatorKey, setup.coinbaseKey, fee); + const auto tx_change = make_reg(newOperatorKey); + auto& chainstate = chainman.ActiveChainstate(); + const Package unrelated{MakeTransactionRef(tx_serv), MakeTransactionRef(tx_change)}; + const Package chained{MakeTransactionRef(tx_serv), MakeTransactionRef(tx_revoke)}; + // Submission only takes a child with its parents, which the unrelated pair is not + for (const auto& [package, test_accept] : {std::pair{unrelated, true}, std::pair{chained, true}, + std::pair{chained, false}}) { + const auto result = WITH_LOCK(cs_main, return ProcessNewPackage(chainstate, mempool, package, test_accept)); + BOOST_CHECK_EQUAL(result.m_state.GetRejectReason(), "protx-dup"); + BOOST_CHECK_EQUAL(mempool.size(), 0U); + } + // A pending service update is spent by a package carrying a key change + BOOST_REQUIRE(accepted(tx_serv)); + const Package spending{MakeTransactionRef(tx_revoke), MakeTransactionRef(spend(tx_revoke))}; + for (const bool test_accept : {true, false}) { + const auto result = WITH_LOCK(cs_main, return ProcessNewPackage(chainstate, mempool, spending, test_accept)); + BOOST_CHECK_EQUAL(result.m_state.GetRejectReason(), "protx-dup"); + BOOST_CHECK_EQUAL(mempool.size(), 1U); + } + reset_mempool(); + } + + // A rotation returned to the mempool by a reorg is still a key change: the mempool checks it + // against the masternode list of the new tip, not of the disconnected block + const auto tx_rotation = make_reg(newOperatorKey); + setup.CreateAndProcessBlock({tx_rotation}, coinbase_pk); + sync_dmn_tip(); + BOOST_REQUIRE(dmnman.GetListAtChainTip().GetMN(proTxHash)->pdmnState->IsBanned()); + BlockValidationState state; + BOOST_REQUIRE(chainman.ActiveChainstate().InvalidateBlock(state, tip_index())); + BOOST_REQUIRE(in_mempool(tx_rotation)); + const auto result = submit(make_serv(/*port=*/7, operatorKey)); + BOOST_CHECK(result.m_result_type == MempoolAcceptResult::ResultType::INVALID); + BOOST_CHECK_EQUAL(result.m_state.GetRejectReason(), "protx-dup"); + reset_mempool(); + + // A reorg returning a block that confirmed a service update and then a rotation, while a pending + // transaction spends the service update, directly or through a transaction the same block + // confirmed: the returning rotation evicts the service update together with every spender, + // although the reorg has not yet linked the pending spender to what it spends + for (const bool intermediate : {false, true}) { + BOOST_TEST_MESSAGE("pending spender " << (intermediate ? "through a confirmed spender" : "direct")); + const CBlockIndex* fork_point{tip_index()}; + const auto tx_serv = make_serv(/*port=*/intermediate ? 10 : 9, operatorKey); + const auto tx_mid = spend(tx_serv); + const auto tx_rot = make_reg(newOperatorKey); + std::vector block_txs{tx_serv}; + if (intermediate) block_txs.push_back(tx_mid); + block_txs.push_back(tx_rot); + setup.CreateAndProcessBlock(block_txs, coinbase_pk); + sync_dmn_tip(); + const auto tx_child = spend(intermediate ? tx_mid : tx_serv); + BOOST_REQUIRE(accepted(tx_child)); + + BlockValidationState reorg_state; + BOOST_REQUIRE(chainman.ActiveChainstate().InvalidateBlock( + reorg_state, WITH_LOCK(cs_main, return chainman.ActiveChain().Next(fork_point)))); + BOOST_CHECK(in_mempool(tx_rot)); + BOOST_CHECK(!in_mempool(tx_serv)); + BOOST_CHECK(!in_mempool(tx_mid)); + BOOST_CHECK(!in_mempool(tx_child)); + { + LOCK2(cs_main, mempool.cs); + mempool.check(chainman.ActiveChainstate().CoinsTip(), chainman.ActiveChain().Height() + 1); + } + reset_mempool(); + } +} + void FuncVerifyDB(TestChainSetup& setup) { auto& chainman = *Assert(setup.m_node.chainman.get()); @@ -3679,6 +3903,12 @@ BOOST_AUTO_TEST_CASE(test_mempool_proreg_replacement_update_conflict) FuncTestMempoolProRegReplacementUpdateConflict(setup); } +BOOST_AUTO_TEST_CASE(test_mempool_stale_service_update) +{ + TestChainV19Setup setup; + FuncTestMempoolStaleServiceUpdate(setup); +} + //This one can be started only with legacy scheme, since inside undo block will switch it back to legacy resulting into an inconsistency BOOST_AUTO_TEST_CASE(verify_db_legacy) { diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 453a180f8444..faf1ccc7ea1c 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1099,6 +1099,93 @@ void CTxMemPool::removeProTxKeyChangedConflicts(const CTransaction &tx, const ui } } +// The masternode whose operator key a registrar update or revocation may change +static std::optional GetOperatorKeyChangeTarget(const CTransaction& tx) +{ + if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) { + if (const auto opt_proTx = GetTxPayload(tx)) return opt_proTx->proTxHash; + } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REVOKE) { + if (const auto opt_proTx = GetTxPayload(tx)) return opt_proTx->proTxHash; + } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_SHARED_REGISTRAR) { + if (const auto opt_proTx = GetTxPayload(tx)) return opt_proTx->proTxHash; + } + return std::nullopt; +} + +void CTxMemPool::CalculateServiceUpdates(const uint256& proTxHash, const setEntries& exclude, setEntries& entries) const +{ + AssertLockHeld(cs); + std::vector stage; + for (auto its = mapProTxRefs.equal_range(proTxHash); its.first != its.second; ++its.first) { + auto txit = mapTx.find(its.first->second); + if (txit == mapTx.end() || txit->GetTx().nType != TRANSACTION_PROVIDER_UPDATE_SERVICE || exclude.count(txit)) { + continue; + } + stage.push_back(txit); + } + // Follow spenders through mapNextTx instead of the child links: while a reorg returns + // transactions to the pool, their links to children already in the pool are only rebuilt + // afterwards (UpdateTransactionsFromBlock), at any depth + while (!stage.empty()) { + const txiter it{stage.back()}; + stage.pop_back(); + if (!entries.insert(it).second) { + continue; + } + const uint256& hash{it->GetTx().GetHash()}; + for (auto next = mapNextTx.lower_bound(COutPoint(hash, 0)); next != mapNextTx.end() && next->first->hash == hash; ++next) { + stage.push_back(mapTx.find(next->second->GetHash())); + } + } +} + +void CTxMemPool::removeProTxStaleServiceUpdates(const CTransaction& tx, const setEntries& ancestors) +{ + AssertLockHeld(cs); + const auto it = mapTx.find(tx.GetHash()); + const auto proTxHash = GetOperatorKeyChangeTarget(tx); + if (it == mapTx.end() || !it->isKeyChangeProTx || !proTxHash) { + return; + } + // An ancestor of the key change is always mined before it, so it cannot undo the change + setEntries stale; + CalculateServiceUpdates(*proTxHash, ancestors, stale); + RemoveStaged(stale, /*updateDescendants=*/false, MemPoolRemovalReason::CONFLICT); +} + +bool CTxMemPool::PackageHasStaleServiceUpdate(const std::vector& txns) const +{ + AssertLockHeld(cs); + std::set targets; + for (const auto& tx : txns) { + if (const auto proTxHash = GetOperatorKeyChangeTarget(*tx)) targets.insert(*proTxHash); + } + if (targets.empty()) { + return false; + } + setEntries stale; + for (const auto& proTxHash : targets) { + CalculateServiceUpdates(proTxHash, /*exclude=*/{}, stale); + } + std::set stale_txids; + for (const auto& entry : stale) { + stale_txids.insert(entry->GetTx().GetHash()); + } + for (const auto& tx : txns) { + if (tx->nType == TRANSACTION_PROVIDER_UPDATE_SERVICE) { + if (const auto opt_proTx = GetTxPayload(*tx); opt_proTx && targets.count(opt_proTx->proTxHash)) { + return true; + } + } + if (stale_txids.count(tx->GetHash()) || std::any_of(tx->vin.begin(), tx->vin.end(), [&](const CTxIn& input) { + return stale_txids.count(input.prevout.hash); + })) { + return true; + } + } + return false; +} + void CTxMemPool::removeProTxVotingPayeeConflicts(const uint256& proTxHash, const CKeyID& keyIDVoting, uint16_t refType) { const CTxDestination voting_dest{PKHash(keyIDVoting)}; @@ -1657,6 +1744,12 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { return true; } } + // This update is signed with the operator key a pending key change replaces. Mined after + // the change, it would restore the previous operator's service fields and revive the + // masternode the change bans. + if (hasKeyChangeInMempool(opt_proTx->proTxHash)) { + return true; + } // Conflict with a replacement ProRegTx that reuses this MN's external collateral. if (auto dmn = m_dmnman.GetListAtChainTip().GetMN(opt_proTx->proTxHash)) { if (mapProTxCollaterals.count(dmn->collateralOutpoint)) { diff --git a/src/txmempool.h b/src/txmempool.h index 08f94c60320e..a7cc818a1a62 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -536,6 +536,18 @@ class CTxMemPool void removeProTxReferences(const uint256& proTxHash) EXCLUSIVE_LOCKS_REQUIRED(cs); void removeProTxSpentCollateralConflicts(const CTransaction &tx) EXCLUSIVE_LOCKS_REQUIRED(cs); void removeProTxKeyChangedConflicts(const CTransaction &tx, const uint256& proTxHash, const uint256& newKeyHash) EXCLUSIVE_LOCKS_REQUIRED(cs); + /** If tx changes or revokes a masternode's operator key, remove that masternode's pending + * ProUpServTxs other than tx's ancestors, together with their descendants. They are signed with + * the key being replaced, and mined after the change they would restore the previous operator's + * service fields. */ + void removeProTxStaleServiceUpdates(const CTransaction& tx, const setEntries& ancestors) EXCLUSIVE_LOCKS_REQUIRED(cs); + /** Does a package pair a registrar update or revocation with a ProUpServTx of the same masternode, + * in the package itself or pending in the pool and spent by the package? Package members are + * checked against the pool before any of them is added, so neither existsProviderTxConflict() + * nor the eviction on admission can separate such a pair. Registrar updates keeping the + * operator key count too: submitpackage is regtest-only, and a multi-transaction + * testmempoolaccept is a dry run, so a false protx-dup there costs nothing. */ + bool PackageHasStaleServiceUpdate(const std::vector& txns) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** Remove pending TXs of refType for proTxHash that pair the P2PKH destination of keyIDVoting with a * share reward script: ProUpShareTxs paying it, or ProUpSharedRegTxs setting it as the voting key. */ void removeProTxVotingPayeeConflicts(const uint256& proTxHash, const CKeyID& keyIDVoting, uint16_t refType) EXCLUSIVE_LOCKS_REQUIRED(cs); @@ -850,6 +862,8 @@ class CTxMemPool * addUnchecked extension for Dash-specific transactions (ProTx). */ void addUncheckedProTx(indexed_transaction_set::iterator& newit, const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs); + /** Add the pending ProUpServTxs of proTxHash that are not in exclude, and their descendants, to entries */ + void CalculateServiceUpdates(const uint256& proTxHash, const setEntries& exclude, setEntries& entries) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** Before calling removeUnchecked for a given transaction, * UpdateForRemoveFromMempool must be called on the entire (dependent) set diff --git a/src/validation.cpp b/src/validation.cpp index 4ad368546a03..592c4afc7d77 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -427,6 +427,12 @@ void Chainstate::MaybeUpdateMempoolForReorg( AssertLockHeld(cs_main); AssertLockHeld(m_mempool->cs); + // The mempool checks provider transactions against the masternode list at the tip, which the + // block tip notifications move only after this. Move it now, so a transaction re-added below is + // not checked against the list of a disconnected block. + if (fAddToMempool && m_chain.Tip()) { + m_special_tx.UpdatedMNListTip(m_chain.Tip()); + } std::vector vHashUpdate; { // disconnectpool is ordered so that the front is the most recently-confirmed @@ -1278,6 +1284,7 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) if (!m_pool.exists(hash)) return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool full"); } + m_pool.removeProTxStaleServiceUpdates(tx, ws.m_ancestors); return true; } @@ -1489,6 +1496,10 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: LOCK(m_pool.cs); if (!CheckPackageAssetUnlockEvictions(txns, package_state)) return PackageMempoolAcceptResult(package_state, {}); + if (m_pool.PackageHasStaleServiceUpdate(txns)) { + package_state.Invalid(PackageValidationResult::PCKG_POLICY, "protx-dup"); + return PackageMempoolAcceptResult(package_state, {}); + } // Do all PreChecks first and fail fast to avoid running expensive script checks when unnecessary. for (Workspace& ws : workspaces) { @@ -1615,6 +1626,10 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // Checked on the whole package before any parent is submitted on its own below: a single // submission of a fresher claimant would already carry out the eviction. if (!CheckPackageAssetUnlockEvictions(package, package_state_quit_early)) return PackageMempoolAcceptResult(package_state_quit_early, {}); + if (m_pool.PackageHasStaleServiceUpdate(package)) { + package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "protx-dup"); + return PackageMempoolAcceptResult(package_state_quit_early, {}); + } // Stores final results that won't change std::map results_final; // Node operators are free to set their mempool policies however they please, nodes may receive From 791ff17c4cf9a6410e02acdb1f6b557f8f86f4e0 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 22 Sep 2026 23:00:23 -0500 Subject: [PATCH 2/3] test: cover stale operator service updates in feature_masternode_shares Run the pending key change checks for a shared masternode, whose ProUpSharedRegTx key change the unit test does not reach: eviction by a later rotation, refusal after a rotation, after one returned by a reorg, and after a revocation. Co-Authored-By: Claude Opus 5.5 (1M context) --- test/functional/feature_masternode_shares.py | 59 ++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/test/functional/feature_masternode_shares.py b/test/functional/feature_masternode_shares.py index cbf4dc05ae5c..e5eace0de0a1 100755 --- a/test/functional/feature_masternode_shares.py +++ b/test/functional/feature_masternode_shares.py @@ -348,6 +348,59 @@ def test_voting_payee_conflict_eviction(self, node, protx_hash): assert_equal(confirmed_address, voting) assert pending_txid not in node.getrawmempool() + def test_stale_service_update(self, node, protx_hash, rotate, port): + """A service update signed with an operator key that a pending key change replaces never shares + the mempool with that change, whichever arrives first. rotate(operator_public, fee_address) + submits an operator key change for protx_hash and returns its txid.""" + def mine(): + self.bump_mocktime(10 * 60 + 1) + return self.generate(node, 1, sync_fun=self.no_op)[0] + + def assert_banned_with(operator): + state = node.protx("info", protx_hash)["state"] + assert_equal(state["pubKeyOperator"], operator) + assert_greater_than(state["PoSeBanHeight"], 0) + + # one per submitted transaction, plus one the refused updates name but never spend + fees = [node.getnewaddress() for _ in range(6)] + node.sendmany("", {address: 1 for address in fees}) + mine() + address = [f"127.0.0.1:{p2p_port(port)}"] + key1, key2, key3 = [node.bls("generate") for _ in range(3)] + # Start from an operator key this test controls + rotate(key1["public"], fees.pop()) + mine() + + self.log.info("A rotation evicts a pending service update signed with the key it replaces") + serv_txid = node.protx("update_service", protx_hash, address, key1["secret"], "", fees.pop()) + rotation_txid = rotate(key2["public"], fees.pop()) + mempool = node.getrawmempool() + assert rotation_txid in mempool + assert serv_txid not in mempool + rotation_block = mine() + assert_banned_with(key2["public"]) + + self.log.info("A rotation returned to the mempool by a reorg still refuses the old key's update") + node.invalidateblock(rotation_block) + assert rotation_txid in node.getrawmempool() + assert_raises_rpc_error(None, "protx-dup", node.protx, "update_service", protx_hash, address, + key1["secret"], "", fees[-1]) + node.reconsiderblock(rotation_block) + assert_equal(node.getbestblockhash(), rotation_block) + + self.log.info("A service update signed with the key a pending rotation replaces is refused") + rotation_txid = rotate(key3["public"], fees.pop()) + assert_raises_rpc_error(None, "protx-dup", node.protx, "update_service", protx_hash, address, + key2["secret"], "", fees[-1]) + assert rotation_txid in node.getblock(mine())["tx"] + + self.log.info("A service update signed with the key a pending revocation clears is refused") + revoke_txid = node.protx("revoke", protx_hash, key3["secret"], 0, fees.pop()) + assert_raises_rpc_error(None, "protx-dup", node.protx, "update_service", protx_hash, address, + key3["secret"], "", fees[-1]) + assert revoke_txid in node.getblock(mine())["tx"] + assert_banned_with("0" * 96) + def test_separate_participant_wallets(self): self.log.info("Eight separate wallets fund and authorize a shared masternode") node = self.nodes[0] @@ -1171,6 +1224,12 @@ def submit_with_sigs(sigs): self.test_pending_registrar_update(node, protx_hash4) self.test_voting_payee_conflict_eviction(node, protx_hash4) + def rotate_shared(operator, fee): + prepared = node.protx("shared_update_registrar_prepare", protx_hash4, operator, "", fee) + sigs = node.protx("shared_sign", prepared["tx"])["signatures"] + return node.protx("shared_combine", prepared["tx"], sigs, True) + self.test_stale_service_update(node, protx_hash4, rotate_shared, port=5) + self.log.info("Shared state survives a node restart") state_before_restart = node.protx("info", protx_hash4)["state"] self.restart_node(0) From df4c41b84053f2afc107b77c2e4b3500d3aeb833 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 22 Sep 2026 23:00:23 -0500 Subject: [PATCH 3/3] docs: add release note for stale operator service updates Co-Authored-By: Claude Opus 5.5 (1M context) --- doc/release-notes-7582.md | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 doc/release-notes-7582.md diff --git a/doc/release-notes-7582.md b/doc/release-notes-7582.md new file mode 100644 index 000000000000..d94b61ecf5f2 --- /dev/null +++ b/doc/release-notes-7582.md @@ -0,0 +1,11 @@ +Mempool policy +-------------- + +- While an operator key change or revocation for a masternode is unconfirmed, the mempool no + longer holds a `ProUpServTx` for that masternode signed with the key being replaced. One + arriving afterwards is rejected with `protx-dup`, and one already pending is evicted when the + key change is accepted, unless the key change descends from it. Such a service update could + otherwise confirm in the same block after the change and restore the previous operator's + service and payout details. Once the change confirms, the new operator signs service updates + with the new key. An operator whose wallet sent an evicted service update should abandon it + with `abandontransaction` to release its inputs. (#7582)