Skip to content
1 change: 1 addition & 0 deletions src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ BITCOIN_CORE_H = \
util/moneystr.h \
util/overflow.h \
util/overloaded.h \
util/pointer.h \
util/ranges.h \
util/readwritefile.h \
util/underlying.h \
Expand Down
12 changes: 6 additions & 6 deletions src/coinjoin/client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ void CCoinJoinClientSession::ProcessMessage(CNode& peer, CChainState& active_cha
if (!m_mn_sync.IsBlockchainSynced()) return;

if (!mixingMasternode) return;
if (mixingMasternode->pdmnState->netInfo.GetPrimary() != peer.addr) return;
if (mixingMasternode->pdmnState->netInfo->GetPrimary() != peer.addr) return;

if (msg_type == NetMsgType::DSSTATUSUPDATE) {
CCoinJoinStatusUpdate psssup;
Expand Down Expand Up @@ -1113,7 +1113,7 @@ bool CCoinJoinClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymized,

m_clientman.AddUsedMasternode(dsq.masternodeOutpoint);

if (connman.IsMasternodeOrDisconnectRequested(dmn->pdmnState->netInfo.GetPrimary())) {
if (connman.IsMasternodeOrDisconnectRequested(dmn->pdmnState->netInfo->GetPrimary())) {
WalletCJLogPrint(m_wallet, /* Continued */
"CCoinJoinClientSession::JoinExistingQueue -- skipping connection, masternode=%s\n", dmn->proTxHash.ToString());
continue;
Expand Down Expand Up @@ -1185,7 +1185,7 @@ bool CCoinJoinClientSession::StartNewQueue(CAmount nBalanceNeedsAnonymized, CCon
continue;
}

if (connman.IsMasternodeOrDisconnectRequested(dmn->pdmnState->netInfo.GetPrimary())) {
if (connman.IsMasternodeOrDisconnectRequested(dmn->pdmnState->netInfo->GetPrimary())) {
WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- skipping connection, masternode=%s\n",
dmn->proTxHash.ToString());
nTries++;
Expand Down Expand Up @@ -1225,7 +1225,7 @@ bool CCoinJoinClientSession::ProcessPendingDsaRequest(CConnman& connman)

CService mn_addr;
if (auto dmn = m_dmnman.GetListAtChainTip().GetMN(pendingDsaRequest.GetProTxHash())) {
mn_addr = Assert(dmn->pdmnState)->netInfo.GetPrimary();
mn_addr = Assert(dmn->pdmnState)->netInfo->GetPrimary();
} else {
WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- cannot find address to connect, masternode=%s\n", __func__,
pendingDsaRequest.GetProTxHash().ToString());
Expand Down Expand Up @@ -1827,7 +1827,7 @@ void CCoinJoinClientSession::RelayIn(const CCoinJoinEntry& entry, CConnman& conn
{
if (!mixingMasternode) return;

connman.ForNode(mixingMasternode->pdmnState->netInfo.GetPrimary(), [&entry, &connman, this](CNode* pnode) {
connman.ForNode(mixingMasternode->pdmnState->netInfo->GetPrimary(), [&entry, &connman, this](CNode* pnode) {
WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::RelayIn -- found master, relaying message to %s\n",
pnode->addr.ToStringAddrPort());
CNetMsgMaker msgMaker(pnode->GetCommonVersion());
Expand Down Expand Up @@ -1883,7 +1883,7 @@ void CCoinJoinClientSession::GetJsonInfo(UniValue& obj) const
assert(mixingMasternode->pdmnState);
obj.pushKV("protxhash", mixingMasternode->proTxHash.ToString());
obj.pushKV("outpoint", mixingMasternode->collateralOutpoint.ToStringShort());
obj.pushKV("service", mixingMasternode->pdmnState->netInfo.GetPrimary().ToStringAddrPort());
obj.pushKV("service", mixingMasternode->pdmnState->netInfo->GetPrimary().ToStringAddrPort());
}
obj.pushKV("denomination", ValueFromAmount(CoinJoin::DenominationToAmount(nSessionDenom)));
obj.pushKV("state", GetStateString());
Expand Down
4 changes: 2 additions & 2 deletions src/evo/core_write.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
ret.pushKV("type", ToUnderlying(nType));
ret.pushKV("collateralHash", collateralOutpoint.hash.ToString());
ret.pushKV("collateralIndex", (int)collateralOutpoint.n);
ret.pushKV("service", netInfo.GetPrimary().ToStringAddrPort());
ret.pushKV("service", netInfo->GetPrimary().ToStringAddrPort());
ret.pushKV("ownerAddress", EncodeDestination(PKHash(keyIDOwner)));
ret.pushKV("votingAddress", EncodeDestination(PKHash(keyIDVoting)));
if (CTxDestination dest; ExtractDestination(scriptPayout, dest)) {
Expand Down Expand Up @@ -114,7 +114,7 @@
ret.pushKV("version", nVersion);
ret.pushKV("type", ToUnderlying(nType));
ret.pushKV("proTxHash", proTxHash.ToString());
ret.pushKV("service", netInfo.GetPrimary().ToStringAddrPort());
ret.pushKV("service", netInfo->GetPrimary().ToStringAddrPort());
if (CTxDestination dest; ExtractDestination(scriptOperatorPayout, dest)) {
ret.pushKV("operatorPayoutAddress", EncodeDestination(dest));
}
Expand Down
46 changes: 27 additions & 19 deletions src/evo/deterministicmns.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <stats/client.h>
#include <uint256.h>
#include <univalue.h>
#include <util/pointer.h>
#include <validationinterface.h>

#include <optional>
Expand Down Expand Up @@ -409,7 +410,7 @@ void CDeterministicMNList::AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTota
throw(std::runtime_error(strprintf("%s: Can't add a masternode %s with a duplicate collateralOutpoint=%s", __func__,
dmn->proTxHash.ToString(), dmn->collateralOutpoint.ToStringShort())));
}
for (const NetInfoEntry& entry : dmn->pdmnState->netInfo.GetEntries()) {
for (const NetInfoEntry& entry : dmn->pdmnState->netInfo->GetEntries()) {
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
const CService& service{service_opt.value()};
if (!AddUniqueProperty(*dmn, service)) {
Expand Down Expand Up @@ -459,13 +460,13 @@ void CDeterministicMNList::UpdateMN(const CDeterministicMN& oldDmn, const std::s
// Using this temporary map as a checkpoint to roll back to in case of any issues.
decltype(mnUniquePropertyMap) mnUniquePropertyMapSaved = mnUniquePropertyMap;

auto updateNetInfo = [this](const CDeterministicMN& dmn, const MnNetInfo& oldInfo,
const MnNetInfo& newInfo) -> std::string {
if (oldInfo != newInfo) {
auto updateNetInfo = [this](const CDeterministicMN& dmn, const std::shared_ptr<NetInfoInterface>& oldInfo,
const std::shared_ptr<NetInfoInterface>& newInfo) -> std::string {
if (util::shared_ptr_not_equal(oldInfo, newInfo)) {
// We track each individual entry in netInfo as opposed to netInfo itself (preventing us from
// using UpdateUniqueProperty()), so we need to successfully purge all old entries and insert
// new entries to successfully update.
for (const NetInfoEntry& old_entry : oldInfo.GetEntries()) {
for (const NetInfoEntry& old_entry : oldInfo->GetEntries()) {
if (const auto& service_opt{old_entry.GetAddrPort()}) {
const CService& service{service_opt.value()};
if (!DeleteUniqueProperty(dmn, service)) {
Expand All @@ -475,7 +476,7 @@ void CDeterministicMNList::UpdateMN(const CDeterministicMN& oldDmn, const std::s
return "invalid address";
}
}
for (const NetInfoEntry& new_entry : newInfo.GetEntries()) {
for (const NetInfoEntry& new_entry : newInfo->GetEntries()) {
if (const auto& service_opt{new_entry.GetAddrPort()}) {
const CService& service{service_opt.value()};
if (!AddUniqueProperty(dmn, service)) {
Expand All @@ -489,6 +490,7 @@ void CDeterministicMNList::UpdateMN(const CDeterministicMN& oldDmn, const std::s
return "";
};

assert(oldState->netInfo && pdmnState->netInfo);
if (auto err = updateNetInfo(*dmn, oldState->netInfo, pdmnState->netInfo); !err.empty()) {
mnUniquePropertyMap = mnUniquePropertyMapSaved;
throw(std::runtime_error(strprintf("%s: Can't update masternode %s with addresses, reason=%s", __func__,
Expand Down Expand Up @@ -549,7 +551,7 @@ void CDeterministicMNList::RemoveMN(const uint256& proTxHash)
throw(std::runtime_error(strprintf("%s: Can't delete a masternode %s with a collateralOutpoint=%s", __func__,
proTxHash.ToString(), dmn->collateralOutpoint.ToStringShort())));
}
for (const NetInfoEntry& entry : dmn->pdmnState->netInfo.GetEntries()) {
for (const NetInfoEntry& entry : dmn->pdmnState->netInfo->GetEntries()) {
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
const CService& service{service_opt.value()};
if (!DeleteUniqueProperty(*dmn, service)) {
Expand Down Expand Up @@ -730,6 +732,7 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
newList.DecreaseScores();

const bool isMNRewardReallocation{DeploymentActiveAfter(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_MN_RR)};
const bool is_v23_deployed{DeploymentActiveAfter(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_V23)};

// we skip the coinbase
for (int i = 1; i < (int)block.vtx.size(); i++) {
Expand Down Expand Up @@ -777,7 +780,7 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
}
}

for (const NetInfoEntry& entry : proTx.netInfo.GetEntries()) {
for (const NetInfoEntry& entry : proTx.netInfo->GetEntries()) {
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
const CService& service{service_opt.value()};
if (newList.HasUniqueProperty(service)) {
Expand All @@ -795,7 +798,7 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no

auto dmnState = std::make_shared<CDeterministicMNState>(proTx);
dmnState->nRegisteredHeight = nHeight;
if (proTx.netInfo.IsEmpty()) {
if (proTx.netInfo->IsEmpty()) {
// start in banned pdmnState as we need to wait for a ProUpServTx
dmnState->BanIfNotBanned(nHeight);
}
Expand All @@ -813,7 +816,7 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-payload");
}

for (const NetInfoEntry& entry : opt_proTx->netInfo.GetEntries()) {
for (const NetInfoEntry& entry : opt_proTx->netInfo->GetEntries()) {
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
const CService& service{service_opt.value()};
if (newList.HasUniqueProperty(service) &&
Expand All @@ -837,6 +840,10 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
}

auto newState = std::make_shared<CDeterministicMNState>(*dmn->pdmnState);
if (is_v23_deployed) {
// Extended addresses support in v23 means that the version can be updated
newState->nVersion = opt_proTx->nVersion;
}
newState->netInfo = opt_proTx->netInfo;
newState->scriptOperatorPayout = opt_proTx->scriptOperatorPayout;
if (opt_proTx->nType == MnType::Evo) {
Expand Down Expand Up @@ -877,7 +884,9 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, gsl::no
newState->ResetOperatorFields();
newState->BanIfNotBanned(nHeight);
// we update pubKeyOperator here, make sure state version matches
newState->nVersion = opt_proTx->nVersion;
// Make sure we don't accidentally downgrade the state version if using version after basic BLS
newState->nVersion = newState->nVersion > ProTxVersion::BasicBLS ? newState->nVersion : opt_proTx->nVersion;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. What happens when there are more new versions?.. should we allow them here? i.e.

Suggested change
newState->nVersion = newState->nVersion > ProTxVersion::BasicBLS ? newState->nVersion : opt_proTx->nVersion;
newState->nVersion = (newState->nVersion > ProTxVersion::BasicBLS && opt_proTx->nVersion <= ProTxVersion::BasicBLS) ? newState->nVersion : opt_proTx->nVersion;

Also, don't we need a similar thing for the new newState->nVersion = opt_proTx->nVersion; in TRANSACTION_PROVIDER_UPDATE_SERVICE part above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a version after ExtAddr, this code will need to be revisited, the change here is meant to ensure two things:

  • That the LegacyBLS to BasicBLS transition is honored (using opt_proTx->nVersion from the ProUpRegTx)
  • That if the node is already at ExtAddr (either from a ProRegTx or a ProUpServTx), the ProUpRegTx (which can only have a version up to BasicBLS), doesn't downgrade newState->nVersion (set as ExtAddr) to BasicBLS (from opt_proTx->nVersion) by setting newState->nVersion to itself.

This handling isn't needed for TRANSACTION_PROVIDER_UPDATE_SERVICE since we want to update nVersion to reflect ExtAddr.

This is alongside a two-pronged check

  • The first prong would reject a ProUpRegTx with nVersion = ExtAddr (by refusing to check the deployment status of extended addresses if not a ProRegTx or a ProUpServTx, forcing the highest valid version to be BasicBLS) to prevent a potential LegacyBLS to ExtAddr leap (source)
  • The second prong would reject unnatural version changes so that a ProUpServTx doesn't cause a potential LegacyBLS to ExtAddr leap (source)

Copy link

@UdjinM6 UdjinM6 Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking more about this issue - should we even care? These txes can be created by MN owners/operators only and only for the MN they own/operate, so no one else is affected. Maybe we should simply let them mess it up for now if they decide to use some custom code (instead of our RPCs) to create these txes? New protx upgrade/downgrade checks (which are activated with v23) will handle that later in a proper place.

Copy link
Collaborator Author

@kwvg kwvg Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not comfortable with leaving the masternode state in an inconsistent state, since that affects all clients that could process the aberrant transaction. I would be more amicable to strip down some of the checks if the worst that could happen is corrupting the local data directory but the wider implications mean that it's better to impose greater restrictions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this condition will only work for post-v23 txes (newState->nVersion > ProTxVersion::BasicBLS) and you have checks for them in CheckProUpRegTx() which are activated with v23 so this extra logic here is simply redundant probably.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@UdjinM6 is this resolvable?

Copy link

@UdjinM6 UdjinM6 Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my suggestion is to keep it newState->nVersion = opt_proTx->nVersion; here + 5c9a116

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProUpRegTx cannot be version ExtAddr anyways, it is capped at BasicBLS, the change is to ensure that there is a registrar update (which naturally can only be up to BasicBLS), it doesn't apply its transaction version to the state if it's a lower version than ExtAddr, which is set by ProUpServTx.

To prevent a future relaxation of maximum version checks from setting ProUpRegTx to version ExtAddr, a more explicit check has been added in dash#6729 (92984c9)

Copy link

@UdjinM6 UdjinM6 Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProUpRegTx cannot be version ExtAddr anyways, it is capped at BasicBLS

That's the part I don't like

the change is to ensure that there is a registrar update (which naturally can only be up to BasicBLS), it doesn't apply its transaction version to the state if it's a lower version than ExtAddr, which is set by ProUpServTx.

But what if there is another version upgrade in the future which should be done via ProUpRegTx too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the part I don't like

It's not great but I briefly tried uniformly setting the nVersion to ExtAddr but it seemed troubling, even if we can constrain application of versioning on the internal state, the version bump indicates no actual change in the transaction itself, requiring selective ignorance of the version field.

Creates this weird situation where the version change (which indicates a change in serialization) may either hint at a change or none at all. This wasn't really a problem in the past since the BLS version update affected all transactions but the way the dmnState handles things, it never envisioned changes to only some transaction types but not others.

So at that point, dmnState needs to track what the effective serialization would be based on the highest version set among all transactions, which means monitoring for unintended version backsliding.

But what if there is another version upgrade in the future which should be done via ProUpRegTx too?

Then the code can be modified again to be

  // Assuming the version after ExtAddr can be set by ProUpRegTx, first condition will apply the version update
  // if version *not* set to ExtAddr (can be done since we bar ProUpRegTx from bearing nVersion=ExtAddr).
  // Second condition apply the version update if transaction is *above* ExtAddr.
  if (newState->nVersion < ProTxVersion::ExtAddr || opt_proTx->nVersion > ProTxVersion::ExtAddr) {
      newState->nVersion = opt_proTx->nVersion;
  }

newState->netInfo = NetInfoInterface::MakeNetInfo(newState->nVersion);
newState->pubKeyOperator = opt_proTx->pubKeyOperator;
}
newState->keyIDVoting = opt_proTx->keyIDVoting;
Expand Down Expand Up @@ -1175,7 +1184,7 @@ void CDeterministicMNManager::CleanupCache(int nHeight)
template <typename ProTx>
static bool CheckService(const ProTx& proTx, TxValidationState& state)
{
switch (proTx.netInfo.Validate()) {
switch (proTx.netInfo->Validate()) {
case NetInfoStatus::BadAddress:
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-addr");
case NetInfoStatus::BadPort:
Expand Down Expand Up @@ -1228,8 +1237,8 @@ static bool CheckPlatformFields(const ProTx& proTx, TxValidationState& state)
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-platform-http-port");
}

if (proTx.platformP2PPort == proTx.platformHTTPPort || proTx.platformP2PPort == proTx.netInfo.GetPrimary().GetPort() ||
proTx.platformHTTPPort == proTx.netInfo.GetPrimary().GetPort()) {
if (proTx.platformP2PPort == proTx.platformHTTPPort || proTx.platformP2PPort == proTx.netInfo->GetPrimary().GetPort() ||
proTx.platformHTTPPort == proTx.netInfo->GetPrimary().GetPort()) {
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-platform-dup-ports");
}

Expand Down Expand Up @@ -1276,8 +1285,7 @@ static std::optional<ProTx> GetValidatedPayload(const CTransaction& tx, gsl::not
state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-payload");
return std::nullopt;
}
const bool is_basic_scheme_active{DeploymentActiveAfter(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_V19)};
if (!opt_ptx->IsTriviallyValid(is_basic_scheme_active, state)) {
if (!opt_ptx->IsTriviallyValid(pindexPrev, state)) {
// pass the state returned by the function above
return std::nullopt;
}
Expand All @@ -1294,7 +1302,7 @@ bool CheckProRegTx(CDeterministicMNManager& dmnman, const CTransaction& tx, gsl:

// It's allowed to set addr to 0, which will put the MN into PoSe-banned state and require a ProUpServTx to be issues later
// If any of both is set, it must be valid however
if (!opt_ptx->netInfo.IsEmpty() && !CheckService(*opt_ptx, state)) {
if (!opt_ptx->netInfo->IsEmpty() && !CheckService(*opt_ptx, state)) {
// pass the state returned by the function above
return false;
}
Expand Down Expand Up @@ -1354,7 +1362,7 @@ bool CheckProRegTx(CDeterministicMNManager& dmnman, const CTransaction& tx, gsl:
auto mnList = dmnman.GetListForBlock(pindexPrev);

// only allow reusing of addresses when it's for the same collateral (which replaces the old MN)
for (const NetInfoEntry& entry : opt_ptx->netInfo.GetEntries()) {
for (const NetInfoEntry& entry : opt_ptx->netInfo->GetEntries()) {
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
const CService& service{service_opt.value()};
if (mnList.HasUniqueProperty(service) &&
Expand Down Expand Up @@ -1432,7 +1440,7 @@ bool CheckProUpServTx(CDeterministicMNManager& dmnman, const CTransaction& tx, g
}

// don't allow updating to addresses already used by other MNs
for (const NetInfoEntry& entry : opt_ptx->netInfo.GetEntries()) {
for (const NetInfoEntry& entry : opt_ptx->netInfo->GetEntries()) {
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
const CService& service{service_opt.value()};
if (mnList.HasUniqueProperty(service) && mnList.GetUniquePropertyMN(service)->proTxHash != opt_ptx->proTxHash) {
Expand Down
Loading
Loading