Skip to content
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

Jump to bottom

refactor!: Governance collateral/fee is actually a commitment #6182

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
+125 −121
Draft

refactor!: Governance collateral/fee is actually a commitment #6182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fdf36d3
refactor!: Governance collateral/fee is actually a commitment
UdjinM6 Sep 22, 2024
6287c14
refactor: make clang format a bit happier
UdjinM6 Sep 22, 2024
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
7 changes: 4 additions & 3 deletions src/governance/common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@
namespace Governance
{

Object::Object(const uint256& nHashParent, int nRevision, int64_t nTime, const uint256& nCollateralHash, const std::string& strDataHex) :
Object::Object(const uint256& nHashParent, int nRevision, int64_t nTime, const uint256& commitment_hash,
const std::string& strDataHex) :
hashParent{nHashParent},
revision{nRevision},
time{nTime},
collateralHash{nCollateralHash},
m_commitment_hash{commitment_hash},
masternodeOutpoint{},
vchSig{},
vchData{ParseHex(strDataHex)}
Expand Down Expand Up @@ -46,7 +47,7 @@ UniValue Object::ToJson() const
UniValue obj(UniValue::VOBJ);
obj.pushKV("objectHash", GetHash().ToString());
obj.pushKV("parentHash", hashParent.ToString());
obj.pushKV("collateralHash", collateralHash.ToString());
obj.pushKV("commitmentHash", m_commitment_hash.ToString());
obj.pushKV("createdAt", time);
obj.pushKV("revision", revision);
UniValue data;
Expand Down
18 changes: 6 additions & 12 deletions src/governance/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ class Object
public:
Object() = default;

Object(const uint256& nHashParent, int nRevision, int64_t nTime, const uint256& nCollateralHash, const std::string& strDataHex);
Object(const uint256& nHashParent, int nRevision, int64_t nTime, const uint256& commitment_hash,
const std::string& strDataHex);

UniValue ToJson() const;

Expand All @@ -55,8 +56,8 @@ class Object
/// time this object was created
int64_t time{0};

/// fee-tx
uint256 collateralHash{};
/// commitment tx id
uint256 m_commitment_hash{};

/// Masternode info for signed objects
COutPoint masternodeOutpoint;
Expand All @@ -67,15 +68,8 @@ class Object

SERIALIZE_METHODS(Object, obj)
{
READWRITE(
obj.hashParent,
obj.revision,
obj.time,
obj.collateralHash,
obj.vchData,
obj.type,
obj.masternodeOutpoint
);
READWRITE(obj.hashParent, obj.revision, obj.time, obj.m_commitment_hash, obj.vchData, obj.type,
obj.masternodeOutpoint);
if (!(s.GetType() & SER_GETHASH)) {
READWRITE(obj.vchSig);
}
Expand Down
6 changes: 3 additions & 3 deletions src/governance/governance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,7 @@ struct sortProposalsByVotes {
bool operator()(const std::pair<CGovernanceObject*, int>& left, const std::pair<CGovernanceObject*, int>& right) const
{
if (left.second != right.second) return (left.second > right.second);
return (UintToArith256(left.first->GetCollateralHash()) > UintToArith256(right.first->GetCollateralHash()));
return (UintToArith256(left.first->GetCommitmentHash()) > UintToArith256(right.first->GetCommitmentHash()));
}
};

Expand Down Expand Up @@ -684,7 +684,7 @@ std::optional<const CGovernanceObject> CGovernanceManager::CreateGovernanceTrigg
// no sb_opt, no trigger
if (!sb_opt.has_value()) return std::nullopt;

//TODO: Check if nHashParentIn, nRevision and nCollateralHashIn are correct
// TODO: Check if nHashParentIn, nRevision and m_commitment_hash are correct
LOCK2(cs_main, cs);

// Check if identical trigger (equal DataHash()) is already created (signed by other masternode)
Expand Down Expand Up @@ -1177,7 +1177,7 @@ void CGovernanceManager::CheckPostponedObjects(PeerManager& peerman)

std::string strError;
bool fMissingConfirmations;
if (govobj.IsCollateralValid(m_chainman, strError, fMissingConfirmations)) {
if (govobj.IsCommitmentValid(m_chainman, strError, fMissingConfirmations)) {
if (govobj.IsValidLocally(Assert(m_dmnman)->GetListAtChainTip(), m_chainman, strError, false)) {
AddGovernanceObject(govobj, peerman);
} else {
Expand Down
79 changes: 43 additions & 36 deletions src/governance/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ CGovernanceObject::CGovernanceObject()
}

CGovernanceObject::CGovernanceObject(const uint256& nHashParentIn, int nRevisionIn, int64_t nTimeIn,
const uint256& nCollateralHashIn, const std::string& strDataHexIn) :
const uint256& commitment_hash, const std::string& strDataHexIn) :
cs(),
m_obj{nHashParentIn, nRevisionIn, nTimeIn, nCollateralHashIn, strDataHexIn}
m_obj{nHashParentIn, nRevisionIn, nTimeIn, commitment_hash, strDataHexIn}
{
// PARSE JSON DATA STORAGE (VCHDATA)
LoadData();
Expand Down Expand Up @@ -375,19 +375,21 @@ UniValue CGovernanceObject::ToJson() const
void CGovernanceObject::UpdateLocalValidity(const CDeterministicMNList& tip_mn_list, const ChainstateManager& chainman)
{
AssertLockHeld(cs_main);
// THIS DOES NOT CHECK COLLATERAL, THIS IS CHECKED UPON ORIGINAL ARRIVAL
// THIS DOES NOT CHECK COMMITMENT TX, THIS IS CHECKED UPON ORIGINAL ARRIVAL
fCachedLocalValidity = IsValidLocally(tip_mn_list, chainman, strLocalValidityError, false);
}


bool CGovernanceObject::IsValidLocally(const CDeterministicMNList& tip_mn_list, const ChainstateManager& chainman, std::string& strError, bool fCheckCollateral) const
bool CGovernanceObject::IsValidLocally(const CDeterministicMNList& tip_mn_list, const ChainstateManager& chainman,
std::string& strError, bool check_commitment) const
{
bool fMissingConfirmations = false;

return IsValidLocally(tip_mn_list, chainman, strError, fMissingConfirmations, fCheckCollateral);
return IsValidLocally(tip_mn_list, chainman, strError, fMissingConfirmations, check_commitment);
}

bool CGovernanceObject::IsValidLocally(const CDeterministicMNList& tip_mn_list, const ChainstateManager& chainman, std::string& strError, bool& fMissingConfirmations, bool fCheckCollateral) const
bool CGovernanceObject::IsValidLocally(const CDeterministicMNList& tip_mn_list, const ChainstateManager& chainman,
std::string& strError, bool& fMissingConfirmations, bool check_commitment) const
{
AssertLockHeld(cs_main);

Expand All @@ -408,14 +410,14 @@ bool CGovernanceObject::IsValidLocally(const CDeterministicMNList& tip_mn_list,
strError = strprintf("Invalid proposal data, error messages: %s", validator.GetErrorMessages());
return false;
}
if (fCheckCollateral && !IsCollateralValid(chainman, strError, fMissingConfirmations)) {
strError = "Invalid proposal collateral";
if (check_commitment && !IsCommitmentValid(chainman, strError, fMissingConfirmations)) {
strError = "Invalid proposal commitment";
return false;
}
return true;
}
case GovernanceObject::TRIGGER: {
if (!fCheckCollateral) {
if (!check_commitment) {
// nothing else we can check here (yet?)
return true;
}
Expand All @@ -442,12 +444,12 @@ bool CGovernanceObject::IsValidLocally(const CDeterministicMNList& tip_mn_list,
}
}

CAmount CGovernanceObject::GetMinCollateralFee() const
CAmount CGovernanceObject::GetMinCommitmentAmount() const
{
// Only 1 type has a fee for the moment but switch statement allows for future object types
// Only 1 type has a commitment requirement for the moment but switch statement allows for future object types
switch (m_obj.type) {
case GovernanceObject::PROPOSAL: {
return GOVERNANCE_PROPOSAL_FEE_TX;
return GOVERNANCE_COMMITMENT_AMOUNT;
}
case GovernanceObject::TRIGGER: {
return 0;
Expand All @@ -458,7 +460,8 @@ CAmount CGovernanceObject::GetMinCollateralFee() const
}
}

bool CGovernanceObject::IsCollateralValid(const ChainstateManager& chainman, std::string& strError, bool& fMissingConfirmations) const
bool CGovernanceObject::IsCommitmentValid(const ChainstateManager& chainman, std::string& strError,
bool& fMissingConfirmations) const
{
AssertLockHeld(cs_main);

Expand All @@ -468,22 +471,23 @@ bool CGovernanceObject::IsCollateralValid(const ChainstateManager& chainman, std

// RETRIEVE TRANSACTION IN QUESTION
uint256 nBlockHash;
CTransactionRef txCollateral = GetTransaction(/* block_index */ nullptr, /* mempool */ nullptr, m_obj.collateralHash, Params().GetConsensus(), nBlockHash);
if (!txCollateral) {
strError = strprintf("Can't find collateral tx %s", m_obj.collateralHash.ToString());
LogPrintf("CGovernanceObject::IsCollateralValid -- %s\n", strError);
CTransactionRef commitment_tx = GetTransaction(/* block_index */ nullptr, /* mempool */ nullptr,
m_obj.m_commitment_hash, Params().GetConsensus(), nBlockHash);
if (!commitment_tx) {
strError = strprintf("Can't find commitment tx %s", m_obj.m_commitment_hash.ToString());
LogPrintf("CGovernanceObject::%s -- %s\n", __func__, strError);
return false;
}

if (nBlockHash == uint256()) {
strError = strprintf("Collateral tx %s is not mined yet", txCollateral->ToString());
LogPrintf("CGovernanceObject::IsCollateralValid -- %s\n", strError);
strError = strprintf("Commitment tx %s is not mined yet", commitment_tx->ToString());
LogPrintf("CGovernanceObject::%s -- %s\n", __func__, strError);
return false;
}

if (txCollateral->vout.empty()) {
if (commitment_tx->vout.empty()) {
strError = "tx vout is empty";
LogPrintf("CGovernanceObject::IsCollateralValid -- %s\n", strError);
LogPrintf("CGovernanceObject::%s -- %s\n", __func__, strError);
return false;
}

Expand All @@ -492,28 +496,29 @@ bool CGovernanceObject::IsCollateralValid(const ChainstateManager& chainman, std
CScript findScript;
findScript << OP_RETURN << ToByteVector(nExpectedHash);

CAmount nMinFee = GetMinCollateralFee();
CAmount commitment_amount = GetMinCommitmentAmount();

LogPrint(BCLog::GOBJECT, "CGovernanceObject::IsCollateralValid -- txCollateral->vout.size() = %s, findScript = %s, nMinFee = %lld\n",
txCollateral->vout.size(), ScriptToAsmStr(findScript, false), nMinFee);
LogPrint(BCLog::GOBJECT, /* Continued */
"CGovernanceObject::%s -- commitment_tx->vout.size() = %s, findScript = %s, commitment_amount = %lld\n",
__func__, commitment_tx->vout.size(), ScriptToAsmStr(findScript, false), commitment_amount);

bool foundOpReturn = false;
for (const auto& output : txCollateral->vout) {
LogPrint(BCLog::GOBJECT, "CGovernanceObject::IsCollateralValid -- txout = %s, output.nValue = %lld, output.scriptPubKey = %s\n",
output.ToString(), output.nValue, ScriptToAsmStr(output.scriptPubKey, false));
for (const auto& output : commitment_tx->vout) {
LogPrint(BCLog::GOBJECT, "CGovernanceObject::%s -- txout = %s, output.nValue = %lld, output.scriptPubKey = %s\n",
__func__, output.ToString(), output.nValue, ScriptToAsmStr(output.scriptPubKey, false));
if (!output.scriptPubKey.IsPayToPublicKeyHash() && !output.scriptPubKey.IsUnspendable()) {
strError = strprintf("Invalid Script %s", txCollateral->ToString());
LogPrintf("CGovernanceObject::IsCollateralValid -- %s\n", strError);
strError = strprintf("Invalid Script %s", commitment_tx->ToString());
LogPrintf("CGovernanceObject::%s -- %s\n", __func__, strError);
return false;
}
if (output.scriptPubKey == findScript && output.nValue >= nMinFee) {
if (output.scriptPubKey == findScript && output.nValue >= commitment_amount) {
foundOpReturn = true;
}
}

if (!foundOpReturn) {
strError = strprintf("Couldn't find opReturn %s in %s", nExpectedHash.ToString(), txCollateral->ToString());
LogPrintf("CGovernanceObject::IsCollateralValid -- %s\n", strError);
strError = strprintf("Couldn't find opReturn %s in %s", nExpectedHash.ToString(), commitment_tx->ToString());
LogPrintf("CGovernanceObject::%s -- %s\n", __func__, strError);
return false;
}

Expand All @@ -528,15 +533,17 @@ bool CGovernanceObject::IsCollateralValid(const ChainstateManager& chainman, std
}
}

if (nConfirmationsIn < GOVERNANCE_FEE_CONFIRMATIONS) {
strError = strprintf("Collateral requires at least %d confirmations to be relayed throughout the network (it has only %d)", GOVERNANCE_FEE_CONFIRMATIONS, nConfirmationsIn);
if (nConfirmationsIn >= GOVERNANCE_MIN_RELAY_FEE_CONFIRMATIONS) {
if (nConfirmationsIn < GOVERNANCE_COMMITMENT_CONFIRMATIONS) {
strError = strprintf("Commitment tx requires at least %d confirmations to be relayed throughout the network "
"(it has only %d)",
GOVERNANCE_COMMITMENT_CONFIRMATIONS, nConfirmationsIn);
if (nConfirmationsIn >= GOVERNANCE_COMMITMENT_MIN_RELAY_CONFIRMATIONS) {
fMissingConfirmations = true;
strError += ", pre-accepted -- waiting for required confirmations";
} else {
strError += ", rejected -- try again later";
}
LogPrintf("CGovernanceObject::IsCollateralValid -- %s\n", strError);
LogPrintf("CGovernanceObject::%s -- %s\n", __func__, strError);

return false;
}
Expand Down
27 changes: 14 additions & 13 deletions src/governance/object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ extern RecursiveMutex cs_main;
static constexpr double GOVERNANCE_FILTER_FP_RATE = 0.001;


static constexpr CAmount GOVERNANCE_PROPOSAL_FEE_TX = (1 * COIN);
static constexpr CAmount GOVERNANCE_COMMITMENT_AMOUNT = (1 * COIN);
static constexpr int64_t GOVERNANCE_COMMITMENT_CONFIRMATIONS = 6;
Copy link

Choose a reason for hiding this comment

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

@UdjinM6, can you explain the rationale/purpose for the 1 and 6 block confirmations here? Does it make more sense now to require InstantSend and/or ChainLocks for proposal validity instead of block confirmations (in a separate issue/PR)?

Copy link
Author

Choose a reason for hiding this comment

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

We had no DML (InstantX wasn't as reliable as deterministic InstantSend) and no ChainLocks (reorgs up to 6 blocks could happen easily) back then. We could indeed consider proposals that have their commitments IS-/ChainLocked safe to accept without waiting for 6 confs now. I'd probably still require at least 1 conf to keep the logic simple because handling "orphan" proposals can be complicated (error/bug-prone) imo.

static constexpr int64_t GOVERNANCE_COMMITMENT_MIN_RELAY_CONFIRMATIONS = 1;

static constexpr int64_t GOVERNANCE_FEE_CONFIRMATIONS = 6;
static constexpr int64_t GOVERNANCE_MIN_RELAY_FEE_CONFIRMATIONS = 1;
static constexpr int64_t GOVERNANCE_UPDATE_MIN = 60 * 60;
static constexpr int64_t GOVERNANCE_DELETION_DELAY = 10 * 60;
static constexpr int64_t GOVERNANCE_ORPHAN_EXPIRATION_TIME = 10 * 60;
Expand Down Expand Up @@ -142,7 +142,8 @@ class CGovernanceObject
public:
CGovernanceObject();

CGovernanceObject(const uint256& nHashParentIn, int nRevisionIn, int64_t nTime, const uint256& nCollateralHashIn, const std::string& strDataHexIn);
CGovernanceObject(const uint256& nHashParentIn, int nRevisionIn, int64_t nTime, const uint256& commitment_hash,
const std::string& strDataHexIn);

CGovernanceObject(const CGovernanceObject& other);

Expand All @@ -168,10 +169,7 @@ class CGovernanceObject
return m_obj.type;
}

const uint256& GetCollateralHash() const
{
return m_obj.collateralHash;
}
const uint256& GetCommitmentHash() const { return m_obj.m_commitment_hash; }

const COutPoint& GetMasternodeOutpoint() const
{
Expand Down Expand Up @@ -228,12 +226,15 @@ class CGovernanceObject

// CORE OBJECT FUNCTIONS

bool IsValidLocally(const CDeterministicMNList& tip_mn_list, const ChainstateManager& chainman, std::string& strError, bool fCheckCollateral) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
bool IsValidLocally(const CDeterministicMNList& tip_mn_list, const ChainstateManager& chainman,
std::string& strError, bool check_commitment) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);

bool IsValidLocally(const CDeterministicMNList& tip_mn_list, const ChainstateManager& chainman, std::string& strError, bool& fMissingConfirmations, bool fCheckCollateral) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
bool IsValidLocally(const CDeterministicMNList& tip_mn_list, const ChainstateManager& chainman, std::string& strError,
bool& fMissingConfirmations, bool check_commitment) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);

/// Check the collateral transaction for the budget proposal/finalized budget
bool IsCollateralValid(const ChainstateManager& chainman, std::string& strError, bool& fMissingConfirmations) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
/// Check the commitment transaction for the budget proposal/finalized budget
bool IsCommitmentValid(const ChainstateManager& chainman, std::string& strError, bool& fMissingConfirmations) const
EXCLUSIVE_LOCKS_REQUIRED(cs_main);

void UpdateLocalValidity(const CDeterministicMNList& tip_mn_list, const ChainstateManager& chainman);

Expand All @@ -247,7 +248,7 @@ class CGovernanceObject
}
}

CAmount GetMinCollateralFee() const;
CAmount GetMinCommitmentAmount() const;

UniValue GetJSONObject() const;

Expand Down
2 changes: 1 addition & 1 deletion src/interfaces/node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class GOV
virtual ~GOV() {}
virtual void getAllNewerThan(std::vector<CGovernanceObject> &objs, int64_t nMoreThanTime) = 0;
virtual int32_t getObjAbsYesCount(const CGovernanceObject& obj, vote_signal_enum_t vote_signal) = 0;
virtual bool getObjLocalValidity(const CGovernanceObject& obj, std::string& error, bool check_collateral) = 0;
virtual bool getObjLocalValidity(const CGovernanceObject& obj, std::string& error, bool check_commitment) = 0;
virtual bool isEnabled() = 0;
virtual void setContext(NodeContext* context) {}
};
Expand Down
Loading
Loading