diff --git a/doc/release-notes-7473.md b/doc/release-notes-7473.md new file mode 100644 index 000000000000..a4313023e891 --- /dev/null +++ b/doc/release-notes-7473.md @@ -0,0 +1,25 @@ +Notable changes +=============== + +* Masternode operator key uniqueness is now enforced per key rather than per BLS + encoding. After the v24 hard fork activates, registering or updating to an + operator public key already used by another masternode is rejected regardless of + which BLS scheme (legacy or basic) either side is encoded under, closing a gap + where the same key could be claimed twice under different encodings. + +Updated RPCs +------------ + +* After the v24 hard fork activates, a legacy (LegacyBLS) masternode migrates to the + basic BLS scheme *in place* when its state version is raised, keeping the same + operator key: the stored operator public key is re-encoded from the legacy to the + basic scheme, with no key rotation required. `protx update_service` and + `protx update_registrar` build the corresponding basic-scheme migration + transaction for a legacy masternode, and the masternode is no longer PoSe-banned by + the change. The `..._legacy` RPC variants keep the masternode on the legacy scheme. + Previously a legacy masternode could not move to the basic scheme without + effectively rotating its operator key, and `update_registrar` clamped the + transaction version silently. + +These changes are gated on the v24 deployment and have no effect until it activates +(v24 is not yet scheduled on mainnet or testnet). diff --git a/src/evo/deterministicmns.h b/src/evo/deterministicmns.h index 3a684ffa78df..05dc5bfdf3e5 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. @@ -551,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 bb3578524d81..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; } @@ -384,6 +404,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) { @@ -461,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, @@ -481,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(); @@ -488,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 @@ -1126,6 +1183,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)) { @@ -1192,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()}) { @@ -1261,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; @@ -1286,6 +1376,8 @@ bool CheckProUpRegTx(const CTransaction& tx, gsl::not_null p 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()) { diff --git a/src/test/evo_deterministicmns_tests.cpp b/src/test/evo_deterministicmns_tests.cpp index 24ace3bc7469..a91fa85de720 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