Skip to content

Commit 625b6f7

Browse files
PastaPastaPastaPastaPastaPasta
committed
Merge #7489: fix: conflict ProRegTx collateral reuse with in-mempool masternode updates
95341f5 refactor: extract removeProTxReferences and cover replacement eviction (pasta) 9b3ca40 fix: conflict ProReg collateral reuse with mempool MN updates (pasta) 4d4f579 test: prove ProReg replacement conflicts with same-MN updates (pasta) Pull request description: ## Issue being fixed or feature implemented A ProRegTx that reuses an external collateral *replaces* the masternode that collateral currently backs — the old masternode ceases to exist and its `proTxHash` becomes unknown. The mempool does not model that. It tracks collateral *spends* (`removeProTxSpentCollateralConflicts`), but reuse is not a spend, so: - A replacement ProRegTx and an update (ProUpServTx / ProUpRegTx / ProUpRevTx) targeting the masternode being replaced can sit in the mempool at the same time. - If both are selected into one block, `BuildNewListFromBlock()` deletes the masternode when it applies the ProRegTx, then fails `bad-protx-hash` on the update. - That aborts block assembly in the miner. The same hazard exists when the replacement ProRegTx arrives in a block rather than through the mempool: the stale update is left behind and can never be mined. ## What was done? Three commits. **1. Mempool admission (`existsProviderTxConflict`)** — the two directions are now symmetric: - A ProRegTx reusing an external collateral conflicts if any mempool transaction still references the incumbent masternode's `proTxHash`. - A ProUpServTx / ProUpRegTx / ProUpRevTx conflicts if a pending ProRegTx already claims its masternode's collateral outpoint. This is first-in-wins with no RBF-style tiebreak. That is a deliberate policy choice: both transactions are individually valid, and the only requirement is that they not coexist. Preferring the incumbent would mean evicting an already-accepted transaction on arrival of a new one, which is a bigger policy change than this problem warrants. **2. Eviction on block connect (`removeProTxConflicts`)** — when a collateral-reusing ProRegTx is mined, any mempool transaction still targeting the replaced masternode is removed. This covers the case where the ProRegTx never passed through this node's mempool, so the admission check above never ran. **3. Refactor** — the removal loop inside `removeProTxSpentCollateralConflicts` is extracted as `CTxMemPool::removeProTxReferences()` and reused by both call sites. No behaviour change; it just avoids a second copy of the iterator-invalidation-safe loop. ### One thing reviewers should look at The eviction in (2) calls `dmnman->GetListAtChainTip()` and needs it to resolve to the list at the *previous* tip, so `GetMNByCollateral()` returns the masternode being replaced rather than its replacement. That holds because `CDeterministicMNManager::tipIndex` is assigned in `UpdatedBlockTip()`, which fires from `ActivateBestChain()` (`validation.cpp:3339-3340`) — after `ConnectTip()` calls `removeForBlock()` (`validation.cpp:3025`). `removeProTxSpentCollateralConflicts()` already depends on this ordering, so the new code is consistent with the file. The difference is that there it is an optimisation, whereas here it is load-bearing: if the ordering changed, `GetMNByCollateral()` would return the replacement masternode and the eviction would silently become a no-op. I have noted the dependency in a comment, but flagging it explicitly since it is the least obvious part of the change. ## How Has This Been Tested? `test_mempool_proreg_replacement_update_conflict` in `src/test/evo_deterministicmns_tests.cpp`, added in the commit *preceding* the fix. It covers both paths: - admission, via `existsProviderTxConflict()` in both directions; - eviction, via `removeForBlock()` with a block containing the replacement ProRegTx. Without the fix the admission assertions fail (2 failures) — the conflicting pair is accepted. With the fix, `evo_dip3_activation_tests` passes (19 cases). Built and run on macOS/arm64 against current `develop`. ## Breaking Changes Mempool policy only; no consensus rules change. Some transaction pairs that were previously accepted together are now mutually exclusive in the mempool. Neither could have been mined together, so nothing that was previously minable is rejected. ## Known follow-up Coverage is C++ unit level only. A functional test exercising the miner path end to end would strengthen this; happy to add one in this PR if reviewers would prefer it before merge. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone Top commit has no ACKs. Tree-SHA512: 07fc7348712ce7e48e40f8234c9a988eb9cac50baed9a85e5229300a1a18ab7caefe46110f348978a44a3943aa7ce068253b4823af51619fc82e02cda7820224
2 parents 97da1e3 + 95341f5 commit 625b6f7

3 files changed

Lines changed: 168 additions & 21 deletions

File tree

‎src/test/evo_deterministicmns_tests.cpp‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1418,6 +1418,112 @@ void FuncTestMempoolDualProregtx(TestChainSetup& setup)
14181418
BOOST_CHECK(testPool.existsProviderTxConflict(CTransaction(tx_reg2)));
14191419
}
14201420

1421+
// A ProRegTx that reuses a confirmed external collateral replaces the live MN at block
1422+
// connect time. An update (ProUpServ/ProUpReg/ProUpRev) for the replaced proTxHash is still
1423+
// mempool-valid against the pre-block tip list, so fee-ordered packaging can put the replacement
1424+
// ProRegTx before the update and make BuildNewListFromBlock return bad-protx-hash, aborting
1425+
// CreateNewBlock/getblocktemplate. The mempool must treat them as conflicts.
1426+
void FuncTestMempoolProRegReplacementUpdateConflict(TestChainSetup& setup)
1427+
{
1428+
auto& chainman = *Assert(setup.m_node.chainman.get());
1429+
auto& dmnman = *Assert(setup.m_node.dmnman);
1430+
auto tip_index = [&] { return WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()); };
1431+
auto tip_height = [&] { return WITH_LOCK(::cs_main, return chainman.ActiveChain().Height()); };
1432+
auto sync_dmn_tip = [&] { dmnman.UpdatedBlockTip(tip_index()); };
1433+
1434+
const CScript coinbase_pk = GetScriptForRawPubKey(setup.coinbaseKey.GetPubKey());
1435+
int nHeight = tip_height();
1436+
auto utxos = BuildSimpleUtxoMap(setup.m_coinbase_txns);
1437+
1438+
CKey ownerKey;
1439+
CKey payoutKey;
1440+
CKey collateralKey;
1441+
CBLSSecretKey operatorKey;
1442+
ownerKey.MakeNewKey(true);
1443+
payoutKey.MakeNewKey(true);
1444+
collateralKey.MakeNewKey(true);
1445+
operatorKey.MakeNewKey();
1446+
1447+
auto scriptPayout = GetScriptForDestination(PKHash(payoutKey.GetPubKey()));
1448+
auto scriptCollateral = GetScriptForDestination(PKHash(collateralKey.GetPubKey()));
1449+
1450+
// Fund and mine an external collateral, then register MN X against it.
1451+
auto tx_collateral = CreateSpendTx(chainman, utxos, scriptCollateral, dmn_types::Regular.collat_amount, setup.coinbaseKey);
1452+
auto block = std::make_shared<CBlock>(setup.CreateBlock({tx_collateral}, coinbase_pk, chainman.ActiveChainstate()));
1453+
BOOST_REQUIRE(chainman.ProcessNewBlock(block, true, nullptr));
1454+
sync_dmn_tip();
1455+
BOOST_CHECK_EQUAL(tip_height(), nHeight + 1);
1456+
1457+
const auto collateralOutpoint = GetCollateralOutpoint(tx_collateral);
1458+
auto tx_reg = CreateProRegTxExternalCollateral(chainman, utxos, /*port=*/1, collateralOutpoint, scriptPayout,
1459+
ownerKey, operatorKey, collateralKey, setup.coinbaseKey);
1460+
const uint256 proTxHash = tx_reg.GetHash();
1461+
block = std::make_shared<CBlock>(setup.CreateBlock({tx_reg}, coinbase_pk, chainman.ActiveChainstate()));
1462+
BOOST_REQUIRE(chainman.ProcessNewBlock(block, true, nullptr));
1463+
sync_dmn_tip();
1464+
BOOST_CHECK_EQUAL(tip_height(), nHeight + 2);
1465+
BOOST_REQUIRE(dmnman.GetListAtChainTip().HasMN(proTxHash));
1466+
BOOST_REQUIRE(dmnman.GetListAtChainTip().GetMNByCollateral(collateralOutpoint) != nullptr);
1467+
BOOST_CHECK_EQUAL(dmnman.GetListAtChainTip().GetMNByCollateral(collateralOutpoint)->proTxHash.ToString(),
1468+
proTxHash.ToString());
1469+
1470+
// Replacement ProRegTx reusing the same external collateral with fresh keys/address.
1471+
CKey ownerKey2;
1472+
CKey payoutKey2;
1473+
CBLSSecretKey operatorKey2;
1474+
ownerKey2.MakeNewKey(true);
1475+
payoutKey2.MakeNewKey(true);
1476+
operatorKey2.MakeNewKey();
1477+
auto scriptPayout2 = GetScriptForDestination(PKHash(payoutKey2.GetPubKey()));
1478+
auto tx_reg_replace = CreateProRegTxExternalCollateral(chainman, utxos, /*port=*/3, collateralOutpoint, scriptPayout2,
1479+
ownerKey2, operatorKey2, collateralKey, setup.coinbaseKey);
1480+
1481+
// Same-block-style update for the MN that the replacement would delete.
1482+
auto tx_up_serv = CreateProUpServTx(chainman, utxos, proTxHash, operatorKey, /*port=*/2, CScript(), setup.coinbaseKey);
1483+
1484+
// Prove the in-block hazard itself: replacement first, then update of the deleted proTxHash.
1485+
{
1486+
LOCK(cs_main);
1487+
CBlock hazard_block;
1488+
hazard_block.vtx.emplace_back(MakeTransactionRef(CMutableTransaction())); // dummy coinbase slot
1489+
hazard_block.vtx.emplace_back(MakeTransactionRef(tx_reg_replace));
1490+
hazard_block.vtx.emplace_back(MakeTransactionRef(tx_up_serv));
1491+
1492+
BlockValidationState state;
1493+
CDeterministicMNList mn_list;
1494+
BOOST_CHECK(!chainman.ActiveChainstate().ChainHelper().special_tx->BuildNewListFromBlock(
1495+
hazard_block, tip_index(), chainman.ActiveChainstate().CoinsTip(), /*debugLogs=*/false, state, mn_list));
1496+
BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-protx-hash");
1497+
}
1498+
1499+
// Mempool acceptance must refuse the second of these once one is present.
1500+
CTxMemPool testPool{MemPoolOptionsForTest(setup.m_node)};
1501+
TestMemPoolEntryHelper entry;
1502+
1503+
{
1504+
LOCK2(cs_main, testPool.cs);
1505+
// Replacement already in mempool => update for the MN it replaces is a conflict.
1506+
testPool.addUnchecked(entry.FromTx(tx_reg_replace));
1507+
BOOST_CHECK_EQUAL(testPool.size(), 1U);
1508+
BOOST_CHECK(testPool.existsProviderTxConflict(CTransaction(tx_up_serv)));
1509+
testPool.removeRecursive(CTransaction(tx_reg_replace), MemPoolRemovalReason::MANUAL);
1510+
BOOST_CHECK_EQUAL(testPool.size(), 0U);
1511+
1512+
// Update already in mempool => replacement ProRegTx reusing that MN's collateral conflicts.
1513+
testPool.addUnchecked(entry.FromTx(tx_up_serv));
1514+
BOOST_CHECK_EQUAL(testPool.size(), 1U);
1515+
BOOST_CHECK(testPool.existsProviderTxConflict(CTransaction(tx_reg_replace)));
1516+
1517+
// existsProviderTxConflict only gates our own acceptance; it cannot stop a miner from
1518+
// confirming the replacement. Once that block arrives, removeForBlock must evict the
1519+
// now-unmineable update, otherwise it lingers and stalls our own block assembly.
1520+
std::vector<CTransactionRef> connected{MakeTransactionRef(CMutableTransaction()),
1521+
MakeTransactionRef(tx_reg_replace)};
1522+
testPool.removeForBlock(connected, tip_height() + 1);
1523+
BOOST_CHECK_EQUAL(testPool.size(), 0U);
1524+
}
1525+
}
1526+
14211527
void FuncVerifyDB(TestChainSetup& setup)
14221528
{
14231529
auto& chainman = *Assert(setup.m_node.chainman.get());
@@ -3144,6 +3250,12 @@ BOOST_AUTO_TEST_CASE(test_mempool_dual_proregtx_basic)
31443250
FuncTestMempoolDualProregtx(setup);
31453251
}
31463252

3253+
BOOST_AUTO_TEST_CASE(test_mempool_proreg_replacement_update_conflict)
3254+
{
3255+
TestChainV19Setup setup;
3256+
FuncTestMempoolProRegReplacementUpdateConflict(setup);
3257+
}
3258+
31473259
//This one can be started only with legacy scheme, since inside undo block will switch it back to legacy resulting into an inconsistency
31483260
BOOST_AUTO_TEST_CASE(verify_db_legacy)
31493261
{

‎src/txmempool.cpp‎

Lines changed: 52 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -937,39 +937,41 @@ void CTxMemPool::removeProTxCollateralConflicts(const CTransaction &tx, const CO
937937
}
938938
}
939939

940+
void CTxMemPool::removeProTxReferences(const uint256& proTxHash)
941+
{
942+
// Can't use equal_range here as every call to removeRecursive might invalidate iterators
943+
AssertLockHeld(cs);
944+
while (true) {
945+
auto it = mapProTxRefs.find(proTxHash);
946+
if (it == mapProTxRefs.end()) {
947+
break;
948+
}
949+
auto conflictIt = mapTx.find(it->second);
950+
if (conflictIt != mapTx.end()) {
951+
removeRecursive(conflictIt->GetTx(), MemPoolRemovalReason::CONFLICT);
952+
} else {
953+
// Should not happen as we track referencing TXs in addUnchecked/removeUnchecked.
954+
// But lets be on the safe side and not run into an endless loop...
955+
LogPrint(BCLog::MEMPOOL, "%s: ERROR: found invalid TX ref in mapProTxRefs, proTxHash=%s, txHash=%s\n", __func__, proTxHash.ToString(), it->second.ToString());
956+
mapProTxRefs.erase(it);
957+
}
958+
}
959+
}
960+
940961
void CTxMemPool::removeProTxSpentCollateralConflicts(const CTransaction &tx)
941962
{
942963
// Remove TXs that refer to a MN for which the collateral was spent
943-
auto removeSpentCollateralConflict = [&](const uint256& proTxHash) EXCLUSIVE_LOCKS_REQUIRED(cs) {
944-
// Can't use equal_range here as every call to removeRecursive might invalidate iterators
945-
AssertLockHeld(cs);
946-
while (true) {
947-
auto it = mapProTxRefs.find(proTxHash);
948-
if (it == mapProTxRefs.end()) {
949-
break;
950-
}
951-
auto conflictIt = mapTx.find(it->second);
952-
if (conflictIt != mapTx.end()) {
953-
removeRecursive(conflictIt->GetTx(), MemPoolRemovalReason::CONFLICT);
954-
} else {
955-
// Should not happen as we track referencing TXs in addUnchecked/removeUnchecked.
956-
// But lets be on the safe side and not run into an endless loop...
957-
LogPrint(BCLog::MEMPOOL, "%s: ERROR: found invalid TX ref in mapProTxRefs, proTxHash=%s, txHash=%s\n", __func__, proTxHash.ToString(), it->second.ToString());
958-
mapProTxRefs.erase(it);
959-
}
960-
}
961-
};
962964
auto mnList = m_dmnman.GetListAtChainTip();
963965
for (const auto& in : tx.vin) {
964966
auto collateralIt = mapProTxCollaterals.find(in.prevout);
965967
if (collateralIt != mapProTxCollaterals.end()) {
966968
// These are not yet mined ProRegTxs
967-
removeSpentCollateralConflict(collateralIt->second);
969+
removeProTxReferences(collateralIt->second);
968970
}
969971
auto dmn = mnList.GetMNByCollateral(in.prevout);
970972
if (dmn) {
971973
// These are updates referring to a mined ProRegTx
972-
removeSpentCollateralConflict(dmn->proTxHash);
974+
removeProTxReferences(dmn->proTxHash);
973975
}
974976
}
975977
}
@@ -1027,6 +1029,13 @@ void CTxMemPool::removeProTxConflicts(const CTransaction &tx)
10271029
}
10281030
if (!proTx.collateralOutpoint.hash.IsNull()) {
10291031
removeProTxCollateralConflicts(tx, proTx.collateralOutpoint);
1032+
// A ProRegTx reusing an external collateral replaces the MN that collateral
1033+
// currently backs, so that MN ceases to exist. Drop any mempool update that
1034+
// still targets its proTxHash; such an update can never be mined afterwards.
1035+
// removeProTxSpentCollateralConflicts only covers collateral *spends*, not reuse.
1036+
if (auto dmn = m_dmnman.GetListAtChainTip().GetMNByCollateral(proTx.collateralOutpoint)) {
1037+
removeProTxReferences(dmn->proTxHash);
1038+
}
10301039
} else {
10311040
removeProTxCollateralConflicts(tx, COutPoint(tx_hash, proTx.collateralOutpoint.n));
10321041
}
@@ -1450,6 +1459,14 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const {
14501459
// there is another tx that spends the collateral
14511460
return true;
14521461
}
1462+
// A replacement ProRegTx deletes the live MN backed by this collateral. Any
1463+
// in-mempool update that still targets that MN's proTxHash would then fail
1464+
// BuildNewListFromBlock with bad-protx-hash if both were mined in one block.
1465+
if (auto dmn = m_dmnman.GetListAtChainTip().GetMNByCollateral(proTx.collateralOutpoint)) {
1466+
if (mapProTxRefs.find(dmn->proTxHash) != mapProTxRefs.end()) {
1467+
return true;
1468+
}
1469+
}
14531470
}
14541471
return false;
14551472
} else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_SERVICE) {
@@ -1470,6 +1487,12 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const {
14701487
return true;
14711488
}
14721489
}
1490+
// Conflict with a replacement ProRegTx that reuses this MN's external collateral.
1491+
if (auto dmn = m_dmnman.GetListAtChainTip().GetMN(opt_proTx->proTxHash)) {
1492+
if (mapProTxCollaterals.count(dmn->collateralOutpoint)) {
1493+
return true;
1494+
}
1495+
}
14731496
} else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) {
14741497
const auto opt_proTx = GetTxPayload<CProUpRegTx>(tx);
14751498
if (!opt_proTx) {
@@ -1484,6 +1507,10 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const {
14841507
LogPrint(BCLog::MEMPOOL, "%s: ERROR: Masternode is not in the list, proTxHash: %s\n", __func__, proTx.proTxHash.ToString());
14851508
return true; // i.e. failed to find validated ProTx == conflict
14861509
}
1510+
// Conflict with a replacement ProRegTx that reuses this MN's external collateral.
1511+
if (mapProTxCollaterals.count(dmn->collateralOutpoint)) {
1512+
return true;
1513+
}
14871514
// only allow one operator key change in the mempool
14881515
if (dmn->pdmnState->pubKeyOperator != proTx.pubKeyOperator) {
14891516
if (hasKeyChangeInMempool(proTx.proTxHash)) {
@@ -1506,6 +1533,10 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const {
15061533
LogPrint(BCLog::MEMPOOL, "%s: ERROR: Masternode is not in the list, proTxHash: %s\n", __func__, proTx.proTxHash.ToString());
15071534
return true; // i.e. failed to find validated ProTx == conflict
15081535
}
1536+
// Conflict with a replacement ProRegTx that reuses this MN's external collateral.
1537+
if (mapProTxCollaterals.count(dmn->collateralOutpoint)) {
1538+
return true;
1539+
}
15091540
// only allow one operator key change in the mempool
15101541
if (dmn->pdmnState->pubKeyOperator.Get() != CBLSPublicKey()) {
15111542
if (hasKeyChangeInMempool(proTx.proTxHash)) {

‎src/txmempool.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,10 @@ class CTxMemPool
650650
void removeProTxPubKeyConflicts(const CTransaction &tx, const CBLSLazyPublicKey &pubKey) EXCLUSIVE_LOCKS_REQUIRED(cs);
651651
void removeProTxPlatformNodeIDConflicts(const CTransaction &tx, const uint160 &platformNodeID) EXCLUSIVE_LOCKS_REQUIRED(cs);
652652
void removeProTxCollateralConflicts(const CTransaction &tx, const COutPoint &collateralOutpoint) EXCLUSIVE_LOCKS_REQUIRED(cs);
653+
/** Remove every mempool TX that refers to proTxHash. Used when the MN it names ceases to exist
654+
* (collateral spent, or collateral reused by a replacement ProRegTx), since such TXs can never
655+
* be mined afterwards and would abort block assembly with "bad-protx-hash". */
656+
void removeProTxReferences(const uint256& proTxHash) EXCLUSIVE_LOCKS_REQUIRED(cs);
653657
void removeProTxSpentCollateralConflicts(const CTransaction &tx) EXCLUSIVE_LOCKS_REQUIRED(cs);
654658
void removeProTxKeyChangedConflicts(const CTransaction &tx, const uint256& proTxHash, const uint256& newKeyHash) EXCLUSIVE_LOCKS_REQUIRED(cs);
655659
void removeProTxConflicts(const CTransaction &tx) EXCLUSIVE_LOCKS_REQUIRED(cs);

0 commit comments

Comments
 (0)