From f094101976e7f56f048b951422a1b2878f6cace8 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 18 Jul 2026 13:11:18 +0300 Subject: [PATCH 1/5] fix: enforce operator key uniqueness across BLS schemes Operator key uniqueness was enforced per BLS encoding rather than per key. mnUniquePropertyMap is keyed by GetUniquePropertyHash(), which serializes its argument, and a BLS key serializes differently under the legacy and basic schemes. So one public key sits in one of two possible slots, and CheckProRegTx's duplicate check -- which consults that map -- could not see the same key presented under the other encoding. A ProRegTx never proves ownership of the operator key, so anyone could re-register an existing masternode's operator public key for the price of a collateral. Rather than re-key the map canonically, which would apply retroactively -- the map is derived, not stored: Unserialize() clears it and rebuilds via AddMN, so a historical cross-scheme pair would make AddMN throw and nodes fail to sync -- probe both encodings at each point where a key can be claimed. There are exactly two schemes, so this is two O(1) lookups rather than a scan, and the consensus rules are gated on v24, leaving historical reconstruction untouched. Checking only the confirmed list is not enough, so probes are placed at every point a key is claimed: - CheckProRegTx and CheckProUpRegTx, against the previous block's list. - RebuildListFromBlock, against the list as rebuilt so far, since per-transaction checks run against pindexPrev and are blind to each other within a block. This rejects cleanly: AddMN()/UpdateMN() report duplicates by throwing, and that throw would escape block-template assembly. - AcceptToMemoryPool, so two in-flight transactions cannot claim one key under different schemes. This one is deliberately NOT gated on v24. Block assembly does not revalidate special transactions cumulatively -- it checks each candidate against the tip, where neither key is yet present -- so a pair admitted before activation is never evicted and would still be selected together afterwards, leaving an honest miner unable to build any template at all. Keeping the pair out of the mempool is what actually closes that, and mempool policy is allowed to be stricter than consensus: a node rejecting the second transaction still accepts a block containing it, so no chain can split over it. The registrar probes run only when the operator key is actually changing, at every layer. An update that keeps its own key cannot create a duplicate, and probing it anyway would let a cross-scheme pair formed before activation permanently block the affected masternode's registrar updates unless it rotated its key -- making an old squat more harmful rather than less. Pairs that already exist before activation are tolerated: nothing rehashes the map, and they remain upgradable because leaving LegacyBLS rotates the key anyway. Co-Authored-By: Claude Opus 4.8 --- src/evo/deterministicmns.h | 27 +++++++++++++++++++ src/evo/specialtxman.cpp | 40 ++++++++++++++++++++++++++++ src/txmempool.cpp | 53 ++++++++++++++++++++++++++++++++++++++ src/txmempool.h | 17 ++++++++++++ src/validation.cpp | 15 +++++++++++ 5 files changed, 152 insertions(+) diff --git a/src/evo/deterministicmns.h b/src/evo/deterministicmns.h index 3a684ffa78df..61c4e357c77e 100644 --- a/src/evo/deterministicmns.h +++ b/src/evo/deterministicmns.h @@ -443,6 +443,33 @@ class CDeterministicMNList return GetMN(p->first); } + /** + * Is this operator public key already held by a masternode other than `self`, under *either* BLS + * encoding? + * + * mnUniquePropertyMap is keyed by GetUniquePropertyHash(), which serializes its argument, and a + * BLS key serializes differently under the legacy and basic schemes. So one public key sits in + * one of two possible slots and a single lookup sees only one of them, which is why operator key + * uniqueness is otherwise enforced per encoding rather than per key. There are exactly two + * schemes, so probing both is O(1) rather than a scan. + * + * Both slots are probed even when one resolves to `self`: where a cross-scheme duplicate pair + * already exists, returning early on a self-match would miss the other member. + * + * Pass uint256() as `self` to exclude nothing. + */ + [[nodiscard]] bool HasOperatorKeyUnderAnyScheme(const CBLSPublicKey& pubkey, const uint256& self) const + { + for (const bool legacy_scheme : {true, false}) { + CBLSLazyPublicKey wrapped; + wrapped.Set(pubkey, legacy_scheme); + if (!HasUniqueProperty(wrapped)) continue; + const auto holder = GetUniquePropertyMN(wrapped); + if (holder && holder->proTxHash != self) return true; + } + return false; + } + // Compare two masternode lists for equality, ignoring non-deterministic members. // Non-deterministic members (nTotalRegisteredCount, internalId) can differ between // nodes due to different sync histories, but don't affect consensus validity. diff --git a/src/evo/specialtxman.cpp b/src/evo/specialtxman.cpp index bb3578524d81..b9abf6b0cbbb 100644 --- a/src/evo/specialtxman.cpp +++ b/src/evo/specialtxman.cpp @@ -384,6 +384,15 @@ bool CSpecialTxProcessor::RebuildListFromBlock(const CBlock& block, gsl::not_nul } dmn->pdmnState = dmnState; + // CheckProRegTx ran against pindexPrev, so transactions in this same block are invisible + // to each other and two of them could claim one operator key under different encodings. + // Re-probe the list as rebuilt so far. AddMN() reports a duplicate by throwing, which + // would escape block assembly, so reject cleanly here instead. + if (is_v24_deployed && + newList.HasOperatorKeyUnderAnyScheme(dmn->pdmnState->pubKeyOperator.Get(), /*self=*/uint256())) { + return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-key"); + } + newList.AddMN(dmn); if (debugLogs) { @@ -504,6 +513,17 @@ bool CSpecialTxProcessor::RebuildListFromBlock(const CBlock& block, gsl::not_nul newState->payouts.clear(); } + // As in the registration path: CheckProUpRegTx ran against pindexPrev, so a second + // transaction in this same block claiming the same key under the other encoding is + // invisible to it. Same key-change scoping as that check -- an update keeping its own key + // cannot create a duplicate. UpdateMN() reports duplicates by throwing, which would + // escape block assembly, so reject cleanly here instead. + if (is_v24_deployed && !(opt_proTx->pubKeyOperator == dmn->pdmnState->pubKeyOperator) && + newList.HasOperatorKeyUnderAnyScheme(opt_proTx->pubKeyOperator.Get(), + /*self=*/opt_proTx->proTxHash)) { + return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-key"); + } + newList.UpdateMN(opt_proTx->proTxHash, newState); if (debugLogs) { @@ -1126,6 +1146,15 @@ bool CheckProRegTx(const CTransaction& tx, gsl::not_null pin return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key"); } + // The check above only sees the operator key under the encoding this payload happens to use, + // so it misses a key an existing masternode holds under the other one. A ProRegTx never + // proves ownership of the operator key, so that gap lets anyone claim a masternode's key. + // Nothing is excluded here: a duplicate key is never allowed, even for a ProTx replacing an + // existing masternode. + if (is_v24_active && mnList.HasOperatorKeyUnderAnyScheme(opt_ptx->pubKeyOperator.Get(), /*self=*/uint256())) { + return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key"); + } + // never allow duplicate platformNodeIds for EvoNodes if (opt_ptx->nType == MnType::Evo) { if (mnList.HasUniqueProperty(opt_ptx->platformNodeID)) { @@ -1287,6 +1316,17 @@ bool CheckProUpRegTx(const CTransaction& tx, gsl::not_null p } } + // As above, but for the key under its other encoding, which the check above cannot see. Only + // when the key is actually changing: an update that keeps its own key cannot create a new + // duplicate, and probing it anyway would let a cross-scheme pair formed before activation + // permanently block that masternode's registrar updates unless it rotated its key -- making an + // old squat more harmful rather than less. + if (DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_V24) && + !(opt_ptx->pubKeyOperator == dmn->pdmnState->pubKeyOperator) && + mnList.HasOperatorKeyUnderAnyScheme(opt_ptx->pubKeyOperator.Get(), /*self=*/opt_ptx->proTxHash)) { + return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key"); + } + if (!DeploymentDIP0003Enforced(pindexPrev->nHeight, Params().GetConsensus())) { if (dmn->pdmnState->keyIDOwner != opt_ptx->keyIDVoting) { return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-key-not-same"); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 8524d6a07e71..adf7a379e27f 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1375,6 +1375,59 @@ TxMempoolInfo CTxMemPool::info(const uint256& hash) const return GetInfo(i); } +bool CTxMemPool::existsProviderTxCrossSchemeConflict(const CTransaction& tx) const +{ + LOCK(cs); + + // Probe both encodings of `pubkey` and report a conflict if any in-flight transaction other than + // `self_protx`'s own already claims it. mapProTxBlsPubKeyHashes maps the scheme-sensitive key + // hash to a transaction hash, not to a masternode, so resolving ownership means decoding the + // conflicting transaction's payload. + auto probe = [&](const CBLSLazyPublicKey& key, const uint256& self_protx) EXCLUSIVE_LOCKS_REQUIRED(cs) { + AssertLockHeld(cs); + const CBLSPublicKey& pubkey{key.Get()}; + if (!pubkey.IsValid()) return false; + for (const bool legacy_scheme : {true, false}) { + CBLSLazyPublicKey wrapped; + wrapped.Set(pubkey, legacy_scheme); + auto it = mapProTxBlsPubKeyHashes.find(wrapped.GetHash()); + if (it == mapProTxBlsPubKeyHashes.end()) continue; + auto txit = mapTx.find(it->second); + if (txit == mapTx.end()) continue; + if (txit->GetTx().GetHash() == tx.GetHash()) continue; // ourselves + if (!self_protx.IsNull() && txit->GetTx().nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) { + // The same masternode's own in-flight registrar update is not a conflict. + if (const auto other = GetTxPayload(txit->GetTx()); + other && other->proTxHash == self_protx) { + continue; + } + } + return true; + } + return false; + }; + + if (tx.nType == TRANSACTION_PROVIDER_REGISTER) { + const auto opt_proTx = GetTxPayload(tx); + if (!opt_proTx) return true; // can't decode payload == conflict, as elsewhere here + return probe(opt_proTx->pubKeyOperator, uint256()); + } + if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) { + const auto opt_proTx = GetTxPayload(tx); + if (!opt_proTx) return true; + // Only probe when the operator key is actually changing, matching the consensus checks. An + // update keeping its own key cannot create a duplicate, and probing it anyway would let one + // member of an existing cross-scheme pair block the other's unrelated registrar updates. + auto dmnman = Assert(m_dmnman.load(std::memory_order_acquire)); + if (auto dmn = dmnman->GetListAtChainTip().GetMN(opt_proTx->proTxHash); + dmn && opt_proTx->pubKeyOperator == dmn->pdmnState->pubKeyOperator) { + return false; + } + return probe(opt_proTx->pubKeyOperator, opt_proTx->proTxHash); + } + return false; +} + bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { auto dmnman = Assert(m_dmnman.load(std::memory_order_acquire)); diff --git a/src/txmempool.h b/src/txmempool.h index e785f025bd39..2afedc986d0c 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -827,6 +827,23 @@ class CTxMemPool */ bool existsProviderTxConflict(const CTransaction &tx) const; + /** + * Does another in-flight transaction already claim this transaction's operator key under the + * *other* BLS encoding? + * + * mapProTxBlsPubKeyHashes is keyed by CBLSLazyPublicKey::GetHash(), which is scheme-sensitive, + * so existsProviderTxConflict() above compares operator keys per encoding rather than per key + * and cannot see this. + * + * This is mempool policy and is intentionally invoked unconditionally (not gated on the + * deployment that makes cross-scheme reuse a consensus error). Gating it would leave a pair + * admitted before activation resident afterwards: block assembly does not revalidate special + * transactions cumulatively, so both would still be selected and the resulting block rejected, + * stalling an honest miner. Policy may be stricter than consensus here, since a node that rejects + * the second transaction still accepts a block containing it. + */ + bool existsProviderTxCrossSchemeConflict(const CTransaction& tx) const; + size_t DynamicMemoryUsage() const; /** Adds a transaction to the unbroadcast set */ diff --git a/src/validation.cpp b/src/validation.cpp index bd99df8bc8d6..1355de1d04cd 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -993,6 +993,21 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_CONFLICT, "protx-dup"); } + // The check above compares operator keys per BLS encoding, so it cannot see a second in-flight + // transaction claiming the same key under the other one. Keeping such a pair out of the mempool + // matters because block assembly does not revalidate special transactions cumulatively: it checks + // each candidate against the tip, where neither key is yet present, so both would be selected and + // the whole block then rejected -- leaving an honest miner unable to produce one at all. + // + // Deliberately NOT gated on the deployment that makes such a pair a consensus error. A pair + // admitted before activation is never evicted by it, so a gated check would leave exactly that + // stall reachable across the boundary. This is mempool policy, which is allowed to be stricter + // than consensus: a node that rejects the second transaction still accepts a block containing it, + // so no chain can split over this. + if (m_pool.existsProviderTxCrossSchemeConflict(tx)) { + return state.Invalid(TxValidationResult::TX_CONFLICT, "protx-dup"); + } + return true; } From 71e7980b796d8d36f5d6e2e7da2cb01dbbc866ea Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 18 Jul 2026 13:19:38 +0300 Subject: [PATCH 2/5] fix: allow legacy->basic operator key migration without key rotation Adopts the maintainer's preferred approach from #7472 (re-encode the operator key on a version change) instead of forcing a key rotation to leave LegacyBLS, while keeping the cross-scheme uniqueness guards this branch added so the re-key cannot collide and stall block production. A masternode operator can keep the same BLS private key across the legacy->basic migration; only the serialized encoding of the public key changes. Rather than rejecting a same-key ProUpServTx/ProUpRegTx and forcing a rotation (which also PoSe-bans the masternode), SetStateVersion() now re-encodes the stored key to the scheme its version implies, and UpdateUniqueProperty() re-keys the scheme-dependent unique-property map when the encoding changes. The RPCs build a BasicBLS migration payload for a legacy masternode instead of erroring. Because #7472's re-encode collides -- and UpdateMN() throws out of block assembly -- when a squatter already holds the same key under the other encoding (the live per-encoding registration hole), migration is guarded: CheckProUpServTx, CheckProUpRegTx and RebuildListFromBlock reject a migration that would collide with another masternode's key under either scheme (bad-protx-dup-key), and only when the key actually changes or the version crosses the scheme boundary, so a grandfathered cross-scheme pair's non-migrating routine update is not blocked. CDeterministicMNStateDiff also has to capture the re-encoding: its field comparison used CBLSLazyPublicKey::operator==, which ignores the scheme, so a same-key migration produced a diff that omitted the key. A node reconstructing the list from evoDB diffs then kept the old encoding while an online-built list had the new one -- a reconstruction split that full-snapshot serialization does not reveal. The diff now compares the scheme-dependent hash. Co-Authored-By: Claude Opus 4.8 --- src/evo/deterministicmns.h | 11 +++- src/evo/dmnstate.h | 10 ++++ src/evo/specialtxman.cpp | 102 ++++++++++++++++++++++++++++--------- src/node/miner.cpp | 17 +++++++ src/node/miner.h | 4 +- src/rpc/evo.cpp | 12 +++++ 6 files changed, 128 insertions(+), 28 deletions(-) diff --git a/src/evo/deterministicmns.h b/src/evo/deterministicmns.h index 61c4e357c77e..05dc5bfdf3e5 100644 --- a/src/evo/deterministicmns.h +++ b/src/evo/deterministicmns.h @@ -578,7 +578,16 @@ class CDeterministicMNList template [[nodiscard]] bool UpdateUniqueProperty(const CDeterministicMN& dmn, const T& oldValue, const T& newValue) { - if (oldValue == newValue) { + // A BLS operator key can keep the same point while its serialized encoding (legacy<->basic) + // changes on a version transition. The map is keyed by GetUniquePropertyHash(), so only that + // hash reveals the entry must be re-keyed to the new scheme; CBLSLazyPublicKey::operator== + // compares the point and ignores the scheme, so it would wrongly short-circuit. Compare the + // serialized hashes for BLS keys and the plain value for every other unique property. + if constexpr (std::is_same_v, CBLSLazyPublicKey>) { + if (GetUniquePropertyHash(oldValue) == GetUniquePropertyHash(newValue)) { + return true; + } + } else if (oldValue == newValue) { return true; } static const T nullValue{}; diff --git a/src/evo/dmnstate.h b/src/evo/dmnstate.h index 7513e6080934..5d92dd899fdf 100644 --- a/src/evo/dmnstate.h +++ b/src/evo/dmnstate.h @@ -236,6 +236,16 @@ class CDeterministicMNStateDiff member.get(state) = member.get(b); fields |= member.mask; } + } else if constexpr (BaseType::mask == Field_pubKeyOperator) { + // CBLSLazyPublicKey::operator== compares the underlying key and ignores its BLS + // encoding, but a scheme migration re-encodes the same key (legacy->basic). That must + // be captured in the diff -- GetHash() is scheme-dependent -- or a diff-reconstructed + // list keeps the old encoding while an online-built list has the new one, and the two + // diverge (mnUniquePropertyMap included). + if (member.get(a).GetHash() != member.get(b).GetHash()) { + member.get(state) = member.get(b); + fields |= member.mask; + } } else { if (member.get(a) != member.get(b)) { member.get(state) = member.get(b); diff --git a/src/evo/specialtxman.cpp b/src/evo/specialtxman.cpp index b9abf6b0cbbb..2c0770ed4bcb 100644 --- a/src/evo/specialtxman.cpp +++ b/src/evo/specialtxman.cpp @@ -41,6 +41,13 @@ static bool AddNetInfoEntries(const std::shared_ptr& net_info, return true; } +// Raising a masternode's state version out of the legacy BLS scheme re-encodes its operator key and +// moves it to a new scheme-dependent unique-property slot; the collision guards key off this. +static bool IsSchemeMigration(int old_version, int new_version) +{ + return old_version == ProTxVersion::LegacyBLS && new_version > ProTxVersion::LegacyBLS; +} + static bool SetStateVersion(CDeterministicMNState& state_mn, uint16_t nVersion, MnType nType, BlockValidationState& state) { @@ -51,6 +58,19 @@ static bool SetStateVersion(CDeterministicMNState& state_mn, uint16_t nVersion, state_mn.payouts = LegacyPayoutAsList(state_mn.scriptPayout); state_mn.scriptPayout.clear(); } + + // Keep the operator key's BLS encoding a deterministic function of nVersion, matching the SML and + // on-disk serialization, so the stored key and the (scheme-dependent) unique-property index use + // the same scheme on every node whether the list was built online or reloaded from a snapshot. + // Set() rather than SetLegacy(): the latter only flips the flag and leaves the cached + // serialization in the old encoding, so a reloaded node would decode a different key. This runs + // before the early return because callers pre-set nVersion, so the version may already match here + // while the key still needs re-encoding. + if (state_mn.pubKeyOperator != CBLSLazyPublicKey()) { + const CBLSPublicKey& pubkey{state_mn.pubKeyOperator.Get()}; + state_mn.pubKeyOperator.Set(pubkey, nVersion == ProTxVersion::LegacyBLS); + } + if (state_mn.nVersion == nVersion && state_mn.netInfo->CanStorePlatform() == needs_extended) { return true; } @@ -470,6 +490,17 @@ bool CSpecialTxProcessor::RebuildListFromBlock(const CBlock& block, gsl::not_nul } } + // Migrating a legacy masternode to the basic scheme re-encodes its stored key + // (SetStateVersion), moving it to the basic-scheme slot. Per-transaction checks ran + // against pindexPrev, so re-check against the list as rebuilt so far: if another + // masternode holds this key under either encoding, the re-key in UpdateMN() would throw + // out of block assembly. + if (is_v24_deployed && IsSchemeMigration(current_version, target_version) && + newList.HasOperatorKeyUnderAnyScheme(dmn->pdmnState->pubKeyOperator.Get(), + /*self=*/opt_proTx->proTxHash)) { + return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-key"); + } + newList.UpdateMN(opt_proTx->proTxHash, newState); if (debugLogs) { LogPrintf("%s -- MN %s updated at height %d: %s\n", __func__, opt_proTx->proTxHash.ToString(), nHeight, @@ -490,6 +521,23 @@ bool CSpecialTxProcessor::RebuildListFromBlock(const CBlock& block, gsl::not_nul const bool operator_changed{newState->pubKeyOperator != opt_proTx->pubKeyOperator}; const uint16_t target_version{is_v24_deployed ? std::max(old_version, opt_proTx->nVersion) : (operator_changed ? opt_proTx->nVersion : old_version)}; + + // Per-transaction checks ran against pindexPrev, so an earlier transaction in this same + // block is invisible to them. Re-evaluate against the list as rebuilt so far: this update + // moves the operator key to a new unique-property slot if it rotates the key or crosses + // the legacy->basic boundary (which re-encodes the key), and if that slot is held by + // another masternode the re-key in UpdateMN() would throw out of block assembly. Reject + // cleanly. Scoped to those two cases so a pre-existing cross-scheme pair's non-migrating + // routine update is not blocked. + { + const bool migrating{IsSchemeMigration(old_version, target_version)}; + if (is_v24_deployed && (operator_changed || migrating) && + newList.HasOperatorKeyUnderAnyScheme(opt_proTx->pubKeyOperator.Get(), + /*self=*/opt_proTx->proTxHash)) { + return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-key"); + } + } + if (operator_changed) { // reset all operator related fields and put MN into PoSe-banned state in case the operator key changes newState->ResetOperatorFields(); @@ -497,12 +545,12 @@ bool CSpecialTxProcessor::RebuildListFromBlock(const CBlock& block, gsl::not_nul newState->pubKeyOperator = opt_proTx->pubKeyOperator; } newState->keyIDVoting = opt_proTx->keyIDVoting; + // SetStateVersion() re-encodes the operator key to target_version's scheme, so the stored + // key stays consistent with its version whether it was carried in this payload (possibly + // under a different version's encoding) or migrated in place. if (!SetStateVersion(*newState, target_version, dmn->nType, state)) { return false; } - if (operator_changed) { - newState->pubKeyOperator.SetLegacy(target_version == ProTxVersion::LegacyBLS); - } if (target_version >= ProTxVersion::ExtAddr) { newState->payouts = opt_proTx->nVersion >= ProTxVersion::ExtAddr ? opt_proTx->payouts @@ -513,17 +561,6 @@ bool CSpecialTxProcessor::RebuildListFromBlock(const CBlock& block, gsl::not_nul newState->payouts.clear(); } - // As in the registration path: CheckProUpRegTx ran against pindexPrev, so a second - // transaction in this same block claiming the same key under the other encoding is - // invisible to it. Same key-change scoping as that check -- an update keeping its own key - // cannot create a duplicate. UpdateMN() reports duplicates by throwing, which would - // escape block assembly, so reject cleanly here instead. - if (is_v24_deployed && !(opt_proTx->pubKeyOperator == dmn->pdmnState->pubKeyOperator) && - newList.HasOperatorKeyUnderAnyScheme(opt_proTx->pubKeyOperator.Get(), - /*self=*/opt_proTx->proTxHash)) { - return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-key"); - } - newList.UpdateMN(opt_proTx->proTxHash, newState); if (debugLogs) { @@ -1221,6 +1258,16 @@ bool CheckProUpServTx(const CTransaction& tx, gsl::not_null return false; } + // A service update carries no operator key, but raising a legacy masternode to the basic scheme + // re-encodes its stored key, moving it to the basic-scheme unique-property slot. If another + // masternode already holds that key under either encoding, the re-key in UpdateMN() would throw + // out of block assembly, so reject the migration cleanly here. + if (DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_V24) && + IsSchemeMigration(dmn->pdmnState->nVersion, opt_ptx->nVersion) && + mnList.HasOperatorKeyUnderAnyScheme(dmn->pdmnState->pubKeyOperator.Get(), /*self=*/opt_ptx->proTxHash)) { + return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key"); + } + // don't allow updating to addresses already used by other MNs for (const auto& entry : opt_ptx->netInfo->GetEntries()) { if (const auto service_opt{entry.GetAddrPort()}) { @@ -1290,6 +1337,20 @@ bool CheckProUpRegTx(const CTransaction& tx, gsl::not_null p return false; } + // This update moves the masternode's operator key to a new unique-property slot when it either + // rotates the key or crosses the legacy->basic scheme boundary (which re-encodes the key). Reject + // if that target slot is already held by another masternode -- under either encoding -- so the + // re-key in UpdateMN() cannot collide and throw out of block assembly. Scoped to those two cases + // so a pre-existing cross-scheme pair's non-migrating routine update is not blocked. + if (DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_V24)) { + const bool key_changed{!(opt_ptx->pubKeyOperator == dmn->pdmnState->pubKeyOperator)}; + const bool migrating{IsSchemeMigration(dmn->pdmnState->nVersion, opt_ptx->nVersion)}; + if ((key_changed || migrating) && + mnList.HasOperatorKeyUnderAnyScheme(opt_ptx->pubKeyOperator.Get(), /*self=*/opt_ptx->proTxHash)) { + return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key"); + } + } + const auto owner_payouts = GetOwnerPayouts(*opt_ptx); if (!IsPayoutListTriviallyValid(owner_payouts, dmn->pdmnState->keyIDOwner, opt_ptx->keyIDVoting, state)) return false; @@ -1315,17 +1376,8 @@ bool CheckProUpRegTx(const CTransaction& tx, gsl::not_null p return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key"); } } - - // As above, but for the key under its other encoding, which the check above cannot see. Only - // when the key is actually changing: an update that keeps its own key cannot create a new - // duplicate, and probing it anyway would let a cross-scheme pair formed before activation - // permanently block that masternode's registrar updates unless it rotated its key -- making an - // old squat more harmful rather than less. - if (DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_V24) && - !(opt_ptx->pubKeyOperator == dmn->pdmnState->pubKeyOperator) && - mnList.HasOperatorKeyUnderAnyScheme(opt_ptx->pubKeyOperator.Get(), /*self=*/opt_ptx->proTxHash)) { - return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-key"); - } + // Cross-scheme duplicates for this update are rejected earlier (see the collision guard after the + // version-change check), which also covers the key-less migration case. if (!DeploymentDIP0003Enforced(pindexPrev->nHeight, Params().GetConsensus())) { if (dmn->pdmnState->keyIDOwner != opt_ptx->keyIDVoting) { diff --git a/src/node/miner.cpp b/src/node/miner.cpp index bd61a3504a01..5e5fc9ebe4fd 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -378,6 +378,23 @@ bool BlockAssembler::TestPackageTransactions(const CTxMemPool::setEntries& packa return false; } + // A special transaction that was valid when it entered the mempool can be invalidated by + // intervening state, most sharply by a fork activating a rule it violates, and nothing + // evicts it. Selection otherwise trusts mempool validity, so the entry would be picked into + // every template and TestBlockValidity would then reject the whole block, leaving an honest + // miner unable to produce one at all. Recheck here, at package granularity: the caller adds + // every member of a package that passes, so dropping one member while keeping its + // descendants would itself yield an invalid template. check_sigs is off because signatures + // were verified on entry and cannot have changed; note CheckMNHFTx() verifies its quorum + // signature regardless, which is cheap enough here given how rare MNHF signals are. + if (it->GetTx().IsSpecialTxVersion()) { + TxValidationState tx_state; + if (!m_chain_helper.special_tx->CheckSpecialTx(it->GetTx(), m_chainstate.m_chain.Tip(), + m_chainstate.CoinsTip(), /*check_sigs=*/false, tx_state)) { + return false; + } + } + const auto& txid = it->GetTx().GetHash(); if (!m_isman.IsInstantSendEnabled() || m_isman.IsLocked(txid)) { continue; diff --git a/src/node/miner.h b/src/node/miner.h index 09777650be17..465929be17dc 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -208,7 +208,7 @@ class BlockAssembler * Increments nPackagesSelected / nDescendantsUpdated with corresponding * statistics from the package selection (for logging statistics). */ void addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated, - const CBlockIndex* pindexPrev) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs); + const CBlockIndex* pindexPrev) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, mempool.cs); // helper functions for addPackageTxs() /** Remove confirmed (inBlock) entries from given set */ @@ -219,7 +219,7 @@ class BlockAssembler * locktime * These checks should always succeed, and they're here * only as an extra check in case of suboptimal node configuration */ - bool TestPackageTransactions(const CTxMemPool::setEntries& package) const; + bool TestPackageTransactions(const CTxMemPool::setEntries& package) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** Sort the package in an order that is valid to appear in a block */ void SortForBlock(const CTxMemPool::setEntries& package, std::vector& sortedEntries); }; diff --git a/src/rpc/evo.cpp b/src/rpc/evo.cpp index 19add2fe9e66..b1b692da42c1 100644 --- a/src/rpc/evo.cpp +++ b/src/rpc/evo.cpp @@ -1246,6 +1246,18 @@ static RPCHelpMan protx_update_registrar_wrapper(const bool specific_legacy_bls_ ptx.pubKeyOperator = dmn->pdmnState->pubKeyOperator; } + // A legacy masternode migrates to the basic scheme via a (non-legacy) registrar update, keeping + // its operator key (migration) or supplying a new one (rotation). ptx.nVersion is already set from + // the deployment state above -- LegacyBLS for the legacy-BLS RPC variant, basic otherwise -- so + // only the key encoding needs fixing up here: a reused key is stored in the legacy encoding, so + // re-encode it to the basic scheme to keep the stored key consistent with its version (and the + // assertion below holding). A freshly parsed key already matches the requested scheme, and basic + // masternodes are untouched. + if (!use_legacy && ptx.pubKeyOperator != CBLSLazyPublicKey() && ptx.pubKeyOperator.IsLegacy()) { + const CBLSPublicKey& pubkey{ptx.pubKeyOperator.Get()}; + ptx.pubKeyOperator.Set(pubkey, /*specificLegacyScheme=*/false); + } + CHECK_NONFATAL(ptx.pubKeyOperator.IsLegacy() == (ptx.nVersion == ProTxVersion::LegacyBLS)); if (!request.params[2].get_str().empty()) { From d62a0319e0eb1afe17ff78afbdac5b2974e8bd07 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 18 Jul 2026 13:19:38 +0300 Subject: [PATCH 3/5] test: cover BLS scheme migration and cross-scheme operator-key uniqueness Unit tests in evo_deterministicmns_tests.cpp and a functional test extension in feature_dip3_v19.py for the two fixes on this branch: cross-scheme operator-key uniqueness and in-place legacy->basic migration that keeps the same key. Each rejection test was watched fail first by flipping the v24 activation height. Covers the desync and evoDB-diff reconstruction paths, the mempool and same-block cross-scheme pairings, the migration collision guards, the block-template stale special-tx recheck, the HasOperatorKeyUnderAnyScheme helper, and the pre-v24 non-retroactivity guard. Co-Authored-By: Claude Opus 4.8 --- src/test/evo_deterministicmns_tests.cpp | 1313 +++++++++++++++++++++- test/functional/feature_protx_version.py | 47 +- 2 files changed, 1351 insertions(+), 9 deletions(-) diff --git a/src/test/evo_deterministicmns_tests.cpp b/src/test/evo_deterministicmns_tests.cpp index 24ace3bc7469..86785836d827 100644 --- a/src/test/evo_deterministicmns_tests.cpp +++ b/src/test/evo_deterministicmns_tests.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -18,6 +19,7 @@ #include #include #include +#include #include #include #include