Skip to content

fix!: use the same version along all protx special transactions #7302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
knst wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

fix!: use the same version along all protx special transactions #7302

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e0b36e
refactor: simplify implementation of regression tests evo_trivialvali…
knst May 1, 2026
d86d571
fix!: use the same version along all protx special transactions
knst Apr 30, 2026
5393e1a
refactor: simplify implementation of providertx versioning
knst Apr 30, 2026
f94f7cc
refactor: drop various unused includes, such as evo/providertx
knst Apr 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/evo/providertx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,10 @@ template <typename T>
[[nodiscard]] uint16_t GetMaxFromDeployment(gsl::not_null<const CBlockIndex*> pindexPrev,
const ChainstateManager& chainman, std::optional<bool> is_basic_override)
{
constexpr bool is_extaddr_eligible{std::is_same_v<std::decay_t<T>, CProRegTx> || std::is_same_v<std::decay_t<T>, CProUpServTx>};
return ProTxVersion::GetMax(
is_basic_override ? *is_basic_override
: DeploymentActiveAfter(pindexPrev, chainman.GetConsensus(), Consensus::DEPLOYMENT_V19),
is_extaddr_eligible ? DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_V24) : false);
return ProTxVersion::GetMax(is_basic_override ? *is_basic_override
: DeploymentActiveAfter(pindexPrev, chainman.GetConsensus(),
Consensus::DEPLOYMENT_V19),
DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_V24));
}
template uint16_t GetMaxFromDeployment<CProRegTx>(gsl::not_null<const CBlockIndex*> pindexPrev,
const ChainstateManager& chainman,
Expand Down
21 changes: 7 additions & 14 deletions src/evo/specialtxman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,8 +859,8 @@ static bool CheckHashSig(const ProTx& proTx, const CBLSPublicKey& pubKey, TxVali
}

template <typename ProTx>
static std::optional<ProTx> GetValidatedPayload(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev,
const ChainstateManager& chainman, TxValidationState& state)
std::optional<ProTx> GetValidatedPayload(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev,
const ChainstateManager& chainman, TxValidationState& state)
{
if (tx.nType != ProTx::SPECIALTX_TYPE) {
state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-type");
Expand All @@ -882,15 +882,13 @@ static std::optional<ProTx> GetValidatedPayload(const CTransaction& tx, gsl::not
/**
* Validates potential changes to masternode state version by ProTx transaction version
* @param[in] pindexPrev Previous block index to validate DEPLOYMENT_V24 activation
* @param[in] tx_type Special transaction type
* @param[in] state_version Current masternode state version
* @param[in] tx_version Proposed transaction version
* @param[out] state This may be set to an Error state if any error occurred processing them
* @returns true if version change is valid or DEPLOYMENT_V24 is not active
*/
static bool IsVersionChangeValid(gsl::not_null<const CBlockIndex*> pindexPrev, const uint16_t tx_type,
const uint16_t state_version, const uint16_t tx_version,
const ChainstateManager& chainman, TxValidationState& state)
static bool IsVersionChangeValid(gsl::not_null<const CBlockIndex*> pindexPrev, const uint16_t state_version,
const uint16_t tx_version, const ChainstateManager& chainman, TxValidationState& state)
{
if (!DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_V24)) {
// New restrictions only apply after v24 deployment
Expand All @@ -907,11 +905,6 @@ static bool IsVersionChangeValid(gsl::not_null<const CBlockIndex*> pindexPrev, c
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-version-upgrade");
}

if (tx_type != TRANSACTION_PROVIDER_UPDATE_SERVICE && tx_version == ProTxVersion::ExtAddr) {
Copy link
Copy Markdown
Collaborator

@kwvg kwvg May 3, 2026

Choose a reason for hiding this comment

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

The reason we have version transition rules is to prevent the node from being classed as ExtAddr without actually opting in to it at creation or by explicitly updating the service.

Especially since once the version is upgraded the serialisation format changes. By classifying all transactions ExtAddr, a revoke transaction could indicate that the node intends to follow ExtAddr serialization when that was a) not the user's intention and b) will cause serialization mismatches which are very dependent on the version (see below)

dash/src/evo/simplifiedmns.h

Lines 78 to 79 in 4a0fd85

NetInfoSerWrapper(const_cast<std::shared_ptr<NetInfoInterface>&>(obj.netInfo),
obj.nVersion >= ProTxVersion::ExtAddr),

// Only new entries (ProRegTx) and service updates (ProUpServTx) can use ExtAddr versioning
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-version-tx-type");
}

return true;
}

Expand Down Expand Up @@ -1077,7 +1070,7 @@ bool CheckProUpServTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*>
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-hash");
}

if (!IsVersionChangeValid(pindexPrev, tx.nType, dmn->pdmnState->nVersion, opt_ptx->nVersion, chainman, state)) {
if (!IsVersionChangeValid(pindexPrev, dmn->pdmnState->nVersion, opt_ptx->nVersion, chainman, state)) {
// pass the state returned by the function above
return false;
}
Expand Down Expand Up @@ -1152,7 +1145,7 @@ bool CheckProUpRegTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> p
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-hash");
}

if (!IsVersionChangeValid(pindexPrev, tx.nType, dmn->pdmnState->nVersion, opt_ptx->nVersion, chainman, state)) {
if (!IsVersionChangeValid(pindexPrev, dmn->pdmnState->nVersion, opt_ptx->nVersion, chainman, state)) {
// pass the state returned by the function above
return false;
}
Expand Down Expand Up @@ -1219,7 +1212,7 @@ bool CheckProUpRevTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> p
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-hash");
}

if (!IsVersionChangeValid(pindexPrev, tx.nType, dmn->pdmnState->nVersion, opt_ptx->nVersion, chainman, state)) {
if (!IsVersionChangeValid(pindexPrev, dmn->pdmnState->nVersion, opt_ptx->nVersion, chainman, state)) {
// pass the state returned by the function above
return false;
}
Expand Down
9 changes: 9 additions & 0 deletions src/evo/specialtxman.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,15 @@ class CSpecialTxProcessor
};


/**
* This helper does some trivial validations that doesn't depends on collateral and
Copy link
Copy Markdown
Collaborator

@kwvg kwvg May 3, 2026

Choose a reason for hiding this comment

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

These changes are valuable but must be decoupled from changes in consensus behavior

* masternode list. Use CheckPro*Tx for the full validation of transaction,
* including bls signatures, list of masternodes and collateral
*/
template <typename ProTx>
std::optional<ProTx> GetValidatedPayload(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev,
const ChainstateManager& chainman, TxValidationState& state);
Comment thread
knst marked this conversation as resolved.

bool CheckProRegTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev,
CDeterministicMNManager& dmnman, const CCoinsViewCache& view, const ChainstateManager& chainman,
TxValidationState& state, bool check_sigs);
Expand Down
2 changes: 1 addition & 1 deletion src/test/data/trivially_invalid.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@
"proupregtx",
"legacy",
"03000300010000000000000000000000000000000000000000000000000000000000000000ffffffff016affffffff0100f2052a01000000016a00000000e40000bfd280d22a21574ea9f568f7a729b370f4051b89eab6df3d0d74b65fde1bdb7900000a6a7fb814db71eb6ef12341749d81b5fd326eb4b9445a3be65668da9856910b378ec6427fe18fc81c39499e7c7060647618f8ec94404051e4a7ab68f1b017fd9e7f94761976a914c18e0b18e0d992e7471d8a4d4da428e8e191328788ac959ca83b61eeb3f5543973253da6da8e9ac23f9e7844453ac5543b51d0177799416666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666",
"gettxpayload-fail",
"bad-protx-payload",
"Transaction with version set to null"
],
[
Expand Down
17 changes: 8 additions & 9 deletions src/test/evo_trivialvalidation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <chainparams.h>
#include <deploymentstatus.h>
#include <evo/providertx.h>
#include <evo/specialtxman.h>
#include <primitives/transaction.h>
#include <validation.h>

Expand All @@ -29,22 +30,20 @@ template <class T>
void TestTxHelper(const CMutableTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev,
const std::optional<std::string>& expected_error, const ChainstateManager& chainman)
{
const bool payload_to_fail = expected_error.has_value() && expected_error.value() == "gettxpayload-fail";
const auto opt_payload = GetTxPayload<T>(tx, false);
BOOST_CHECK_EQUAL(opt_payload.has_value(), !payload_to_fail);
TxValidationState dummy_state;
auto opt_payload = GetValidatedPayload<T>(CTransaction{tx}, pindexPrev, chainman, dummy_state);

// No need to check anything else if GetTxPayload() expected to fail
if (payload_to_fail) return;
BOOST_CHECK_EQUAL(opt_payload.has_value(), !expected_error.has_value());

TxValidationState dummy_state;
BOOST_CHECK_EQUAL(opt_payload->IsTriviallyValid(pindexPrev, chainman, dummy_state), !expected_error.has_value());
if (expected_error.has_value()) {
BOOST_CHECK_EQUAL(dummy_state.GetRejectReason(), expected_error.value());
}
}

void trivialvalidation_runner(const ChainstateManager& chainman, const std::string& json)
void trivialvalidation_runner(ChainstateManager& chainman, const std::string& json)
{
LOCK(::cs_main);

const UniValue vectors = read_json(json);

for (size_t idx = 1; idx < vectors.size(); idx++) {
Expand Down Expand Up @@ -72,7 +71,7 @@ void trivialvalidation_runner(const ChainstateManager& chainman, const std::stri
BOOST_CHECK_EQUAL(tx.nVersion, 3);
BOOST_CHECK_EQUAL(tx.GetHash(), txHash);
// Deserialization based on transaction nType
TxValidationState dummy_state;

switch (tx.nType) {
case TRANSACTION_PROVIDER_REGISTER: {
BOOST_CHECK_EQUAL(txType, "proregtx");
Expand Down