refactor: separate governance from consensus code#7330
Conversation
|
knst
force-pushed
the
refactor-dummy-governance
branch
from
e6b705b to
e92e4ed
Compare
WalkthroughThis PR extracts superblock trigger/state management into a new governance::SuperblockManager and introduces CGovernance superblock types in governance/superblock.h/cpp; it moves proposal validation into governance::ValidateProposal (object.cpp/h) and embeds governance exception types in object.h, removes legacy validators/exceptions/classes units, and rewires consumers (chain helper, init, payments, net/PeerManager, RPC, GUI, tests) and build files to use the new interfaces. Sequence Diagram(s)sequenceDiagram
participant AppInitMain
participant CChainstateHelper
participant SuperblockManager
participant CGovernanceManager
participant CMNPaymentsProcessor
participant PeerManager
AppInitMain->>CChainstateHelper: construct (creates SuperblockManager)
CChainstateHelper->>SuperblockManager: initialize triggers
AppInitMain->>CGovernanceManager: construct (pass SuperblockManager&)
CMNPaymentsProcessor->>SuperblockManager: IsSuperblockTriggered / GetSuperblockPayments
PeerManager->>NetGovernance: AlreadyHave / ProcessGetData
NetGovernance->>CGovernanceManager: query availability
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/masternode.cpp (1)
308-317:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
chain_helper/superblocksbefore projected-winner access.Line 317 dereferences
node.chain_helper->superblockswithout the non-fatal guard used at Line 308. This can crash RPC execution when the helper path is unavailable.Suggested fix
- const auto tip_mn_list = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip(); + const auto tip_mn_list = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip(); + const auto& chain_helper = *CHECK_NONFATAL(node.chain_helper); + auto& superblocks = *CHECK_NONFATAL(chain_helper.superblocks); for (int h = nStartHeight; h <= nChainTipHeight; h++) { const CBlockIndex* pIndex = pindexTip->GetAncestor(h - 1); auto payee = node.dmnman->GetListForBlock(pIndex).GetMNPayee(pIndex); if (payee) { - std::string strPayments = GetRequiredPaymentsString(*CHECK_NONFATAL(node.chain_helper)->superblocks, tip_mn_list, h, payee); + std::string strPayments = GetRequiredPaymentsString(superblocks, tip_mn_list, h, payee); if (!strFilter.empty() && strPayments.find(strFilter) == std::string::npos) continue; obj.pushKV(strprintf("%d", h), strPayments); } } @@ - std::string strPayments = GetRequiredPaymentsString(*node.chain_helper->superblocks, tip_mn_list, h, projection[i]); + std::string strPayments = GetRequiredPaymentsString(superblocks, tip_mn_list, h, projection[i]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/masternode.cpp` around lines 308 - 317, The projection loop dereferences node.chain_helper->superblocks without the safety check used earlier; update the code that constructs strPayments in the projection loop to guard chain_helper/superblocks (e.g. use CHECK_NONFATAL(node.chain_helper)->superblocks as done at the top or explicitly check node.chain_helper and node.chain_helper->superblocks and skip/continue if null) before calling GetRequiredPaymentsString for projection[i]; ensure you reference the same safety mechanism around GetRequiredPaymentsString, GetProjectedMNPayees, and projection to prevent a crash when the helper path is unavailable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/governance/governance.cpp`:
- Around line 347-350: The snapshot of the deterministic masternode list is
taken too early; move the call to m_dmnman.GetListAtChainTip() so it happens
after entering the critical section (after LOCK2(::cs_main, cs_store)) and use
that fresh tip_mn_list for ClearMasternodeVotes(), UpdateLocalValidity(), and
UpdateSentinelVariables(); i.e., remove the earlier capture, reassign
tip_mn_list inside the LOCK2 block to ensure the DML snapshot matches the
protected chainstate when those functions are invoked.
In `@src/governance/object.cpp`:
- Around line 65-78: The validation currently accepts floating values via
get_real() but the execution path uses ParsePaymentAmount ->
ParseFixedPoint(strAmount, 8, &nAmount) which rejects >8 decimal places and
scientific notation; update ValidatePaymentAmount (and any places using
GetDataValue for payment_amount) to validate by either calling ParseFixedPoint
on the original string (e.g., ParseFixedPoint(strAmount, 8, &nAmount)) and
returning the parse error, or enforce identical constraints (no scientific
notation, max 8 decimal places, integer-like formatting) before converting to
double so that ValidatePaymentAmount, ParsePaymentAmount, and code paths at the
other referenced locations behave identically to ParseFixedPoint.
In `@src/governance/superblock.cpp`:
- Line 382: The comment line containing the JSON snippet with an embedded NUL
character should be cleaned: locate the comment that starts with
{"event_block_height": 879720 ... "type": 2} inside superblock.cpp (the
commented JSON sample) and remove the literal NUL byte (0x00) from the comment
so it becomes a normal ASCII/UTF-8 comment; ensure no other non-printable
characters are left in that comment line and save the file with a clean
encoding.
- Around line 624-635: The function SuperblockManager::GetSuperblockPayments can
return false without clearing the out-parameter voutSuperblockRet, allowing
callers to see stale data; ensure voutSuperblockRet.clear() is called before the
early return path (i.e., clear it before or immediately after calling
GetBestSuperblockInternal and before returning false when
GetBestSuperblockInternal fails) so callers always receive an empty vector on
failure.
In `@src/node/interfaces.cpp`:
- Around line 311-313: The code dereferences context().chain_helper->superblocks
without checking it; update the conditional that currently checks
context().chain_helper and context().chainman to also verify
context().chain_helper->superblocks is non-null before using it, e.g. only call
context().chain_helper->superblocks->GetActiveTriggers() when superblocks is
present; adjust the if condition surrounding ActiveHeight() and the for-loop
over GetActiveTriggers() accordingly so GetActiveTriggers() is never called on a
null pointer.
---
Outside diff comments:
In `@src/rpc/masternode.cpp`:
- Around line 308-317: The projection loop dereferences
node.chain_helper->superblocks without the safety check used earlier; update the
code that constructs strPayments in the projection loop to guard
chain_helper/superblocks (e.g. use
CHECK_NONFATAL(node.chain_helper)->superblocks as done at the top or explicitly
check node.chain_helper and node.chain_helper->superblocks and skip/continue if
null) before calling GetRequiredPaymentsString for projection[i]; ensure you
reference the same safety mechanism around GetRequiredPaymentsString,
GetProjectedMNPayees, and projection to prevent a crash when the helper path is
unavailable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 77e21d83-ecad-44a9-bff4-c1a25aa9b4b5
📒 Files selected for processing (43)
src/Makefile.amsrc/active/context.cppsrc/active/context.hsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/governance/classes.hsrc/governance/core_write.cppsrc/governance/exceptions.cppsrc/governance/exceptions.hsrc/governance/governance.cppsrc/governance/governance.hsrc/governance/net_governance.cppsrc/governance/net_governance.hsrc/governance/object.cppsrc/governance/object.hsrc/governance/signing.cppsrc/governance/signing.hsrc/governance/superblock.cppsrc/governance/superblock.hsrc/governance/validators.cppsrc/governance/validators.hsrc/governance/vote.hsrc/governance/votedb.cppsrc/governance/votedb.hsrc/init.cppsrc/masternode/payments.cppsrc/masternode/payments.hsrc/net_processing.cppsrc/net_processing.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/node/interfaces.cppsrc/qt/proposalcreate.cppsrc/qt/proposallist.cppsrc/qt/proposalmodel.cppsrc/qt/proposalresume.cppsrc/rpc/governance.cppsrc/rpc/masternode.cppsrc/rpc/mining.cppsrc/test/governance_validators_tests.cppsrc/test/util/setup_common.cppsrc/wallet/interfaces.cpptest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (9)
- src/governance/validators.h
- src/governance/classes.h
- src/governance/vote.h
- src/governance/exceptions.cpp
- src/wallet/interfaces.cpp
- src/governance/exceptions.h
- test/lint/lint-circular-dependencies.py
- src/governance/validators.cpp
- src/node/chainstate.h
| bool GetDataValue(const UniValue& objJSON, const std::string& strKey, double& dValueRet, std::string& strErrorMessages) | ||
| { | ||
| try { | ||
| const UniValue uValue = objJSON[strKey]; | ||
| if (uValue.getType() == UniValue::VNUM) { | ||
| dValueRet = uValue.get_real(); | ||
| return true; | ||
| } | ||
| } catch (std::exception& e) { | ||
| strErrorMessages += std::string(e.what()) + std::string(";"); | ||
| } catch (...) { | ||
| strErrorMessages += "Unknown exception;"; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and get line counts
wc -l src/governance/object.cppRepository: dashpay/dash
Length of output: 87
🏁 Script executed:
# Read the specific lines mentioned in the review (65-78, 155-173, 974-1006)
echo "=== Lines 65-78 ===" && sed -n '65,78p' src/governance/object.cpp
echo -e "\n=== Lines 155-173 ===" && sed -n '155,173p' src/governance/object.cpp
echo -e "\n=== Lines 974-1006 ===" && sed -n '974,1006p' src/governance/object.cppRepository: dashpay/dash
Length of output: 2360
🏁 Script executed:
# Search for payment_amount usage and get context
rg "payment_amount" src/governance/object.cpp -A 3 -B 3Repository: dashpay/dash
Length of output: 367
🏁 Script executed:
# Check how CAmount is defined and used in the codebase
fd -t f -e h -e hpp | xargs rg "typedef.*CAmount|class CAmount" | head -20Repository: dashpay/dash
Length of output: 104
🏁 Script executed:
# Look for fixed-point parsing patterns in the codebase
rg "ParseMoney|CAmount.*parse" src/ -l | head -10Repository: dashpay/dash
Length of output: 291
🏁 Script executed:
# Check how other monetary amounts are parsed in governance and similar code
rg "UniValue.*get_real\(\)" src/ -B 5 -A 5 | head -80Repository: dashpay/dash
Length of output: 662
🏁 Script executed:
# Search for how payment_amount is used after validation
rg "payment_amount" src/governance/ -A 5 -B 5 | head -120Repository: dashpay/dash
Length of output: 3709
🏁 Script executed:
# Check if payment_amount is converted to CAmount anywhere
rg "payment_amount" src/governance/ -l | xargs grep -l "CAmount"Repository: dashpay/dash
Length of output: 137
🏁 Script executed:
# Look for the proposal structure and how amounts are stored/used
rg "class.*Proposal|struct.*Proposal" src/governance/ -A 10 -B 2 | head -60Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Check how other monetary amounts are handled in governance
rg "CAmount\|int64_t.*amount" src/governance/*.h | head -30Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Look for how UniValue numbers should be parsed (check existing patterns)
rg "get_int64\|getInt|ParseAmount" src/governance/ -B 3 -A 3 | head -80Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Check util/moneystr.h for proper parsing functions
head -100 src/util/moneystr.hRepository: dashpay/dash
Length of output: 850
🏁 Script executed:
# Search for ParsePaymentAmount function definition
rg "ParsePaymentAmount" src/ -B 5 -A 10 | head -100Repository: dashpay/dash
Length of output: 3093
🏁 Script executed:
# Check what AmountFromValue is (mentioned in moneystr.h comment)
rg "AmountFromValue\|ValueFromAmount" src/ -B 3 -A 3 | head -100Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Check if there are any tests for ValidatePaymentAmount
rg "ValidatePaymentAmount" src/ -B 2 -A 2Repository: dashpay/dash
Length of output: 588
🏁 Script executed:
# Look at the actual JSON format of proposals to understand the schema
rg "payment_amount.*example\|payment_amount.*\"" src/governance/ -B 3 -A 3 | head -80Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Check if double parsing could cause actual issues (scientific notation, etc.)
python3 << 'EOF'
import json
# Test what UniValue get_real() might accept vs what CAmount expects
test_values = [
"1.5", # decimal - valid coin amount
"1e6", # scientific - should this be valid?
"-1", # negative
"999999999999999999", # very large
"0.00000001", # small (1 satoshi)
]
for val in test_values:
try:
d = float(val)
print(f"{val:20s} -> double: {d}")
except:
print(f"{val:20s} -> ERROR")
EOFRepository: dashpay/dash
Length of output: 247
🏁 Script executed:
# Find the ParsePaymentAmount implementation
rg "CAmount ParsePaymentAmount" src/ -A 15Repository: dashpay/dash
Length of output: 757
🏁 Script executed:
# Check superblock.cpp for the implementation
fd superblock.cpp | xargs cat | grep -A 20 "CAmount ParsePaymentAmount"Repository: dashpay/dash
Length of output: 100
🏁 Script executed:
# Look for the actual implementation of ParsePaymentAmount
fd -e cpp -e h | xargs grep -l "ParsePaymentAmount" | xargs grep -A 15 "^CAmount ParsePaymentAmount"Repository: dashpay/dash
Length of output: 835
🏁 Script executed:
# Try to find ParsePaymentAmount implementation in superblock.cpp
file src/governance/superblock.cppRepository: dashpay/dash
Length of output: 101
🏁 Script executed:
# Read superblock.cpp to find ParsePaymentAmount
sed -n '1,50p' src/governance/superblock.cpp | cat -ARepository: dashpay/dash
Length of output: 1946
🏁 Script executed:
# Use strings to extract the implementation
strings src/governance/superblock.cpp | grep -A 10 -B 2 "ParsePaymentAmount"Repository: dashpay/dash
Length of output: 102
🏁 Script executed:
# Check if there's a clear text version or search differently
rg "ParsePaymentAmount" src/governance/superblock.cpp -A 15Repository: dashpay/dash
Length of output: 114
🏁 Script executed:
# Try with explicit encoding
iconv -f utf-16 -t utf-8 src/governance/superblock.cpp 2>/dev/null | grep -A 15 "ParsePaymentAmount" | head -30Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Look at the file size and check what's going on
stat src/governance/superblock.cpp
hexdump -C src/governance/superblock.cpp | head -20Repository: dashpay/dash
Length of output: 145
🏁 Script executed:
# Check git history for the recent content
git show HEAD:src/governance/superblock.cpp | grep -A 20 "CAmount ParsePaymentAmount"Repository: dashpay/dash
Length of output: 100
🏁 Script executed:
# Continue reading the ParsePaymentAmount implementation
sed -n '20,100p' src/governance/superblock.cpp | cat -ARepository: dashpay/dash
Length of output: 3251
🏁 Script executed:
# Try to get the full function implementation with a wider range
sed -n '18,120p' src/governance/superblock.cppRepository: dashpay/dash
Length of output: 3877
🏁 Script executed:
# Use head/tail to extract the section
head -120 src/governance/superblock.cpp | tail -100Repository: dashpay/dash
Length of output: 3830
🏁 Script executed:
# Now verify the actual problem: does double parsing diverge from ParseFixedPoint?
python3 << 'EOF'
# Test if double validation could accept values that ParseFixedPoint rejects
test_cases = [
"1.5",
"1.99999999", # More than 8 decimal places
"0.123456789", # 9 decimal places - would be truncated in fixed-point
"999999999.999999", # Large with many decimals
"1.111111111111111", # Precision loss in double to fixed-point
]
for val in test_cases:
try:
d = float(val)
# Simulate ParseFixedPoint with 8 decimal places (satoshis)
# It should convert "1.5" -> 150000000 satoshis
# And reject values with more than 8 decimal places
parts = val.split('.')
if len(parts) == 2:
decimals = len(parts[1])
if decimals > 8:
print(f"{val:25s} -> double OK but would FAIL ParseFixedPoint (too many decimals: {decimals})")
else:
print(f"{val:25s} -> both should accept")
else:
print(f"{val:25s} -> both should accept")
except:
print(f"{val:25s} -> ERROR")
EOFRepository: dashpay/dash
Length of output: 387
🏁 Script executed:
# Check what GetDataValue actually returns - does it support getting the raw string?
rg "GetDataValue\|get_str\|getValStr" src/governance/object.cpp -B 2 -A 2 | head -80Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Verify the actual validation gap: ValidatePaymentAmount should be validating what will actually be parsed
git log --oneline -20 src/governance/object.cppRepository: dashpay/dash
Length of output: 151
🏁 Script executed:
# Check if there's an issue with the validation approach itself
rg "ValidatePaymentAmount\|ValidatePayment" src/governance/ --type cppRepository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Verify the actual issue: Does validation use double while actual parsing uses fixed-point?
# Let's check if ValidatePaymentAmount is being used as the validation gate before ParsePaymentAmount is called
rg "ValidatePaymentAmount|ValidateProposal" src/ -B 3 -A 3 | head -150Repository: dashpay/dash
Length of output: 8939
🏁 Script executed:
# Check if there's any reconciliation between the two parsing methods
rg "payment_amount" src/governance/object.cpp -B 5 -A 5Repository: dashpay/dash
Length of output: 608
🏁 Script executed:
# Verify: does the validation happen before or after the actual payload is used?
git show HEAD:src/governance/object.cpp | grep -A 50 "ValidateProposal"Repository: dashpay/dash
Length of output: 3006
🏁 Script executed:
# Confirm the actual issue: can we find a case where validation passes but parsing would fail?
python3 << 'EOF'
import subprocess
import json
# The issue is: ValidatePaymentAmount uses double, but actual parsing uses ParseFixedPoint(8 decimals)
# So values with > 8 decimal places would pass double validation but fail ParseFixedPoint
test_value = "1.123456789" # 9 decimals - should pass double check but fail ParseFixedPoint
print(f"Test value: {test_value}")
print(f"- Double parsing: {'PASS' if float(test_value) > 0 else 'FAIL'}")
print(f"- ParseFixedPoint(8): FAIL (too many decimal places)")
print()
print("This demonstrates the validation gap:")
print("1. ValidatePaymentAmount would accept this via get_real()")
print("2. But ParsePaymentAmount with ParseFixedPoint(8) would reject it")
EOFRepository: dashpay/dash
Length of output: 319
🏁 Script executed:
# Check if there are any tests that verify this alignment
rg "payment_amount.*decimal|payment_amount.*precision" src/test/Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Check the actual comment in object.cpp to see what the original author knew
sed -n '155,175p' src/governance/object.cppRepository: dashpay/dash
Length of output: 671
ValidatePaymentAmount must validate the exact format that ParsePaymentAmount will accept.
Currently, ValidatePaymentAmount parses into double via get_real(), which accepts values with any number of decimal places. However, the actual processing path uses ParsePaymentAmount with ParseFixedPoint(strAmount, 8, &nAmount), which rejects values with more than 8 decimal places. This allows proposals to pass validation but fail during execution—e.g., "1.123456789" (9 decimals) would be accepted here but rejected later. Validate payment_amount by directly attempting the ParseFixedPoint conversion or by enforcing the same constraints (max 8 decimal places, no scientific notation) that the actual parser requires.
Also applies to lines 155-173 and 974-1006.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/governance/object.cpp` around lines 65 - 78, The validation currently
accepts floating values via get_real() but the execution path uses
ParsePaymentAmount -> ParseFixedPoint(strAmount, 8, &nAmount) which rejects >8
decimal places and scientific notation; update ValidatePaymentAmount (and any
places using GetDataValue for payment_amount) to validate by either calling
ParseFixedPoint on the original string (e.g., ParseFixedPoint(strAmount, 8,
&nAmount)) and returning the parse error, or enforce identical constraints (no
scientific notation, max 8 decimal places, integer-like formatting) before
converting to double so that ValidatePaymentAmount, ParsePaymentAmount, and code
paths at the other referenced locations behave identically to ParseFixedPoint.
| bool SuperblockManager::GetSuperblockPayments(const CDeterministicMNList& tip_mn_list, int nBlockHeight, | ||
| std::vector<CTxOut>& voutSuperblockRet) const | ||
| { | ||
| LOCK(cs_sb); | ||
|
|
||
| CSuperblock_sptr pSuperblock; | ||
| if (!GetBestSuperblockInternal(tip_mn_list, pSuperblock, nBlockHeight)) { | ||
| LogPrint(BCLog::GOBJECT, "GetSuperblockPayments -- Can't find superblock for height %d\n", nBlockHeight); | ||
| return false; | ||
| } | ||
|
|
||
| voutSuperblockRet.clear(); |
There was a problem hiding this comment.
Clear the out-parameter before the early return path.
A failed lookup currently returns false with whatever contents the caller already had in voutSuperblockRet. Reused vectors can then leak stale superblock outputs into downstream logic.
Suggested fix
bool SuperblockManager::GetSuperblockPayments(const CDeterministicMNList& tip_mn_list, int nBlockHeight,
std::vector<CTxOut>& voutSuperblockRet) const
{
LOCK(cs_sb);
+ voutSuperblockRet.clear();
CSuperblock_sptr pSuperblock;
if (!GetBestSuperblockInternal(tip_mn_list, pSuperblock, nBlockHeight)) {
LogPrint(BCLog::GOBJECT, "GetSuperblockPayments -- Can't find superblock for height %d\n", nBlockHeight);
return false;
}
-
- voutSuperblockRet.clear();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/governance/superblock.cpp` around lines 624 - 635, The function
SuperblockManager::GetSuperblockPayments can return false without clearing the
out-parameter voutSuperblockRet, allowing callers to see stale data; ensure
voutSuperblockRet.clear() is called before the early return path (i.e., clear it
before or immediately after calling GetBestSuperblockInternal and before
returning false when GetBestSuperblockInternal fails) so callers always receive
an empty vector on failure.
| if (context().chain_helper != nullptr && context().chainman != nullptr) { | ||
| const int32_t nTipHeight = context().chainman->ActiveHeight(); | ||
| for (const auto& trigger : context().govman->GetActiveTriggers()) { | ||
| for (const auto& trigger : context().chain_helper->superblocks->GetActiveTriggers()) { |
There was a problem hiding this comment.
Check chain_helper->superblocks before dereferencing.
Line 313 assumes context().chain_helper->superblocks is always present, but Line 311 only guards chain_helper and chainman. Add a superblocks null check before iterating triggers.
Suggested fix
- if (context().chain_helper != nullptr && context().chainman != nullptr) {
+ if (context().chain_helper != nullptr &&
+ context().chain_helper->superblocks != nullptr &&
+ context().chainman != nullptr) {
const int32_t nTipHeight = context().chainman->ActiveHeight();
for (const auto& trigger : context().chain_helper->superblocks->GetActiveTriggers()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/node/interfaces.cpp` around lines 311 - 313, The code dereferences
context().chain_helper->superblocks without checking it; update the conditional
that currently checks context().chain_helper and context().chainman to also
verify context().chain_helper->superblocks is non-null before using it, e.g.
only call context().chain_helper->superblocks->GetActiveTriggers() when
superblocks is present; adjust the if condition surrounding ActiveHeight() and
the for-loop over GetActiveTriggers() accordingly so GetActiveTriggers() is
never called on a null pointer.
e92e4ed to
1382bfa
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/masternode.cpp (1)
303-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent and missing null-checks for
chain_helperandsuperblocks.Line 308 checks
node.chain_helperwithCHECK_NONFATALinside the loop, but line 317 doesn't check at all. Neither line verifies thatnode.chain_helper->superblocksis non-null before dereferencing. The pattern established on line 303 (checkingnode.dmnmanonce before the loops) should be followed forchain_helperas well. Additionally, sincemasternode_paymentsexplicitly checkschain_helperat line 386, these checks are necessary to prevent potential null pointer dereferences.🛡️ Proposed fix: add defensive checks before the loops
const auto tip_mn_list = CHECK_NONFATAL(node.dmnman)->GetListAtChainTip(); + CHECK_NONFATAL(node.chain_helper); + CHECK_NONFATAL(node.chain_helper->superblocks); + auto& superblocks = *node.chain_helper->superblocks; for (int h = nStartHeight; h <= nChainTipHeight; h++) { const CBlockIndex* pIndex = pindexTip->GetAncestor(h - 1); auto payee = node.dmnman->GetListForBlock(pIndex).GetMNPayee(pIndex); if (payee) { - std::string strPayments = GetRequiredPaymentsString(*CHECK_NONFATAL(node.chain_helper)->superblocks, tip_mn_list, h, payee); + std::string strPayments = GetRequiredPaymentsString(superblocks, tip_mn_list, h, payee); if (!strFilter.empty() && strPayments.find(strFilter) == std::string::npos) continue; obj.pushKV(strprintf("%d", h), strPayments); } } auto projection = node.dmnman->GetListForBlock(pindexTip).GetProjectedMNPayees(pindexTip, /*nCount=*/20); for (size_t i = 0; i < projection.size(); i++) { int h = nChainTipHeight + 1 + i; - std::string strPayments = GetRequiredPaymentsString(*node.chain_helper->superblocks, tip_mn_list, h, projection[i]); + std::string strPayments = GetRequiredPaymentsString(superblocks, tip_mn_list, h, projection[i]); if (!strFilter.empty() && strPayments.find(strFilter) == std::string::npos) continue; obj.pushKV(strprintf("%d", h), strPayments); }Also applies to: 308-308, 317-317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/masternode.cpp` at line 303, The code inconsistently validates node.chain_helper and never checks chain_helper->superblocks before dereferencing, risking null-pointer access; mirror the pattern used for node.dmnman by adding a single upfront CHECK_NONFATAL(node.chain_helper) (or equivalent defensive null-check) before the loops that use chain_helper, and additionally verify chain_helper->superblocks is non-null (e.g., check pointer before iterating or dereferencing) in the functions/methods referencing chain_helper and superblocks (locations around the lines using node.chain_helper, chain_helper->superblocks, and masternode_payments), so all uses of node.chain_helper and chain_helper->superblocks are guarded consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/rpc/masternode.cpp`:
- Line 303: The code inconsistently validates node.chain_helper and never checks
chain_helper->superblocks before dereferencing, risking null-pointer access;
mirror the pattern used for node.dmnman by adding a single upfront
CHECK_NONFATAL(node.chain_helper) (or equivalent defensive null-check) before
the loops that use chain_helper, and additionally verify
chain_helper->superblocks is non-null (e.g., check pointer before iterating or
dereferencing) in the functions/methods referencing chain_helper and superblocks
(locations around the lines using node.chain_helper, chain_helper->superblocks,
and masternode_payments), so all uses of node.chain_helper and
chain_helper->superblocks are guarded consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6a3ac0a2-e02a-4e75-bd60-29e09bb860f5
📒 Files selected for processing (42)
src/Makefile.amsrc/active/context.cppsrc/active/context.hsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/governance/classes.hsrc/governance/core_write.cppsrc/governance/exceptions.cppsrc/governance/exceptions.hsrc/governance/governance.cppsrc/governance/governance.hsrc/governance/net_governance.cppsrc/governance/net_governance.hsrc/governance/object.cppsrc/governance/object.hsrc/governance/signing.cppsrc/governance/signing.hsrc/governance/superblock.cppsrc/governance/superblock.hsrc/governance/validators.cppsrc/governance/validators.hsrc/governance/vote.hsrc/governance/votedb.cppsrc/governance/votedb.hsrc/init.cppsrc/masternode/payments.cppsrc/masternode/payments.hsrc/net_processing.cppsrc/net_processing.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/node/interfaces.cppsrc/qt/proposalcreate.cppsrc/qt/proposalmodel.cppsrc/rpc/governance.cppsrc/rpc/masternode.cppsrc/rpc/mining.cppsrc/test/governance_validators_tests.cppsrc/test/util/setup_common.cppsrc/wallet/interfaces.cpptest/functional/feature_governance_cl.pytest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (9)
- src/governance/classes.h
- src/governance/exceptions.h
- test/lint/lint-circular-dependencies.py
- src/governance/validators.cpp
- src/wallet/interfaces.cpp
- src/node/chainstate.h
- src/governance/validators.h
- src/governance/vote.h
- src/governance/exceptions.cpp
✅ Files skipped from review due to trivial changes (4)
- src/Makefile.am
- src/governance/votedb.cpp
- src/governance/votedb.h
- src/active/context.h
🚧 Files skipped from review as they are similar to previous changes (20)
- src/qt/proposalcreate.cpp
- src/qt/proposalmodel.cpp
- src/evo/chainhelper.cpp
- src/governance/net_governance.h
- src/active/context.cpp
- src/governance/net_governance.cpp
- src/rpc/mining.cpp
- src/governance/object.h
- src/test/governance_validators_tests.cpp
- src/net_processing.h
- src/governance/superblock.h
- src/governance/signing.h
- src/test/util/setup_common.cpp
- src/node/chainstate.cpp
- src/masternode/payments.h
- src/node/interfaces.cpp
- src/evo/chainhelper.h
- src/init.cpp
- src/net_processing.cpp
- src/governance/governance.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Refactor extracts SuperblockManager from CGovernanceManager, removes CProposalValidator, and moves governance net wiring to NetGovernance handler. Mechanical and behavior-preserving in the governance-enabled path. Two notable items: an embedded NUL byte committed into the new superblock.cpp (causes the file to be classified as binary by tooling), and a subtle behavioral change in PeerManagerImpl::AlreadyHave for governance INVs when -disablegovernance is set.
Reviewed commit: 1382bfa
🟡 1 suggestion(s) | 💬 3 nitpick(s)
4 additional finding(s)
🟡 suggestion: AlreadyHave for governance INVs changes semantics under -disablegovernance
src/net_processing.cpp (lines 2368-2373)
Pre-PR, the MSG_GOVERNANCE_OBJECT / MSG_GOVERNANCE_OBJECT_VOTE branch unconditionally returned !m_govman.ConfirmInventoryRequest(inv). Post-PR, it iterates m_handlers and returns false if no handler claims the inv. The only handler that answers for governance INVs is NetGovernance, and it is registered in init.cpp:2273 only inside the if (is_governance_enabled) block. Result: when started with -disablegovernance, the node now returns false ("we want it") for every incoming governance inv rather than calling ConfirmInventoryRequest, which previously returned true while the chain wasn't synced or when the object/vote was already known. The net effect is a node configured to disable governance will request governance objects/votes it has no handler to process. Either preserve the prior fallback (return !m_govman.ConfirmInventoryRequest(inv)) when no handler claims the inv, or register a no-op NetGovernance whose AlreadyHave returns true for these inv types when governance is disabled.
💬 nitpick: Embedded NUL byte in superblock.cpp
src/governance/superblock.cpp (line 382)
Line 382 contains a literal NUL (0x00) byte immediately before the newline at the end of the comment (verified: file has exactly one NUL at byte offset 14884; file(1) reports the source as data rather than text). This causes ripgrep and other text-oriented tooling to classify the entire newly-added source file as binary and skip it for searches/lint. No runtime impact, but it should be cleaned up before merge to avoid breaking review and maintenance tooling on this file.
💡 Suggested change
// {\"event_block_height\": 879720, \"payment_addresses\": \"yd5KMREs3GLMe6mTJYr3YrH1juwNwrFCfB\", \"payment_amounts\": \"5.00000000\", \"proposal_hashes\": \"485817fddbcab6c55c9a6856dabc8b19ed79548bda8c01712daebc9f74f287f4\", \"type\": 2}
💬 nitpick: SuperblockManager::Clear() does not reset m_loaded
src/governance/superblock.cpp (lines 521-525)
Clear() only wipes m_triggers and leaves m_loaded untouched. CGovernanceManager::Clear() at governance.cpp:1001 also doesn't reset its mirror flag is_loaded, so the two stay in sync today and Clear() isn't reached from production code. But the asymmetry is latent: a future caller wanting to put the manager back into an unloaded state will find that SuperblockManager::IsValid() still returns true and CMNPaymentsProcessor::IsBlockValueValid / IsBlockPayeeValid continue to skip their m_loaded-keyed early-out. Either also reset m_loaded in Clear(), or document that Clear() is scoped to in-memory data only.
💬 nitpick: Expired trigger left in m_triggers when AddTrigger returns false
src/governance/governance.cpp (lines 305-310)
SuperblockManager::AddTrigger (superblock.cpp:427-456) emplaces the new trigger into m_triggers before evaluating !pSuperblock->IsExpired(cachedHeight) as its return value. When the trigger is already height-expired, AddTrigger returns false but the entry remains in m_triggers. The caller marks the govobj for deletion via PrepareDeletion but does not call RemoveTrigger(nHash), so the SuperblockManager temporarily holds an entry whose backing object is already flagged for deletion until the next CheckAndRemove sweep (governance.cpp:417). This matches pre-PR behavior, but the new docstring ("Returns true if the resulting trigger is live") obscures it. Either roll back the emplace when IsExpired is true, or update the docstring to clarify that a false return still leaves the entry present until the next sweep.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/net_processing.cpp`:
- [SUGGESTION] lines 2368-2373: AlreadyHave for governance INVs changes semantics under -disablegovernance
Pre-PR, the MSG_GOVERNANCE_OBJECT / MSG_GOVERNANCE_OBJECT_VOTE branch unconditionally returned `!m_govman.ConfirmInventoryRequest(inv)`. Post-PR, it iterates `m_handlers` and returns false if no handler claims the inv. The only handler that answers for governance INVs is `NetGovernance`, and it is registered in `init.cpp:2273` only inside the `if (is_governance_enabled)` block. Result: when started with `-disablegovernance`, the node now returns false ("we want it") for every incoming governance inv rather than calling `ConfirmInventoryRequest`, which previously returned true while the chain wasn't synced or when the object/vote was already known. The net effect is a node configured to disable governance will request governance objects/votes it has no handler to process. Either preserve the prior fallback (`return !m_govman.ConfirmInventoryRequest(inv)`) when no handler claims the inv, or register a no-op NetGovernance whose AlreadyHave returns true for these inv types when governance is disabled.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
…ernance/governance
…declarations & includes, move more network code to NetGovernance
…nce to unique_ptr Initialization of order is fixed, this workaround is no more relevant for governance
knst
force-pushed
the
refactor-dummy-governance
branch
from
1382bfa to
51a12ae
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51a12aeb3e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| continue; | ||
| } | ||
|
|
||
| entry.obj->UpdateSentinelVariables(tip_mn_list); |
There was a problem hiding this comment.
When block validation/mining (or masternode winners) calls IsSuperblockTriggered while the governance scheduler is running CGovernanceManager::CheckAndRemove, this call updates the same CGovernanceObject cached flags (fCachedFunding, fCachedValid, fDirtyCache, etc.) that CheckAndRemove still updates under cs_store. The new cs_sb lock does not synchronize with that existing cs_store path, so the refactor introduces unsynchronized concurrent writes/reads of the trigger's cached state, which can produce transient wrong superblock-trigger decisions or trip TSAN; keep these cache mutations behind a common lock or move the cached flags under the object mutex.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rpc/masternode.cpp`:
- Line 320: The second winners loop dereferences node.chain_helper when calling
GetRequiredPaymentsString (using tip_mn_list and projection[i]) without the
CHECK_NONFATAL guard used earlier; add the same non-null check for
node.chain_helper before any use in that loop (i.e., ensure
CHECK_NONFATAL(node.chain_helper) or equivalent guard surrounds the call to
GetRequiredPaymentsString and any other accesses to node.chain_helper in the
second winner loop) so both loops consistently validate node.chain_helper before
dereferencing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3743a100-f522-4fa9-a06b-bc4e9b4c7fb5
📒 Files selected for processing (44)
src/Makefile.amsrc/active/context.cppsrc/active/context.hsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/governance/classes.hsrc/governance/core_write.cppsrc/governance/exceptions.cppsrc/governance/exceptions.hsrc/governance/governance.cppsrc/governance/governance.hsrc/governance/net_governance.cppsrc/governance/net_governance.hsrc/governance/object.cppsrc/governance/object.hsrc/governance/signing.cppsrc/governance/signing.hsrc/governance/superblock.cppsrc/governance/superblock.hsrc/governance/validators.cppsrc/governance/validators.hsrc/governance/vote.hsrc/governance/votedb.cppsrc/governance/votedb.hsrc/init.cppsrc/masternode/payments.cppsrc/masternode/payments.hsrc/net_processing.cppsrc/net_processing.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/node/interfaces.cppsrc/qt/proposalcreate.cppsrc/qt/proposallist.cppsrc/qt/proposalmodel.cppsrc/qt/proposalresume.cppsrc/rpc/governance.cppsrc/rpc/masternode.cppsrc/rpc/mining.cppsrc/test/governance_validators_tests.cppsrc/test/util/setup_common.cppsrc/wallet/interfaces.cpptest/functional/feature_governance_cl.pytest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (9)
- src/wallet/interfaces.cpp
- src/governance/exceptions.h
- test/lint/lint-circular-dependencies.py
- src/governance/vote.h
- src/governance/validators.cpp
- src/governance/classes.h
- src/node/chainstate.h
- src/governance/exceptions.cpp
- src/governance/validators.h
✅ Files skipped from review due to trivial changes (2)
- src/governance/votedb.cpp
- src/qt/proposallist.cpp
🚧 Files skipped from review as they are similar to previous changes (18)
- src/qt/proposalmodel.cpp
- test/functional/feature_governance_cl.py
- src/governance/votedb.h
- src/node/chainstate.cpp
- src/governance/signing.h
- src/Makefile.am
- src/test/governance_validators_tests.cpp
- src/node/interfaces.cpp
- src/masternode/payments.h
- src/governance/net_governance.h
- src/test/util/setup_common.cpp
- src/governance/object.h
- src/governance/superblock.h
- src/net_processing.cpp
- src/masternode/payments.cpp
- src/init.cpp
- src/governance/net_governance.cpp
- src/governance/governance.h
| for (size_t i = 0; i < projection.size(); i++) { | ||
| int h = nChainTipHeight + 1 + i; | ||
| std::string strPayments = GetRequiredPaymentsString(*node.govman, tip_mn_list, h, projection[i]); | ||
| std::string strPayments = GetRequiredPaymentsString(*node.chain_helper->superblocks, tip_mn_list, h, projection[i]); |
There was a problem hiding this comment.
Use the same non-null guard for node.chain_helper in both winner loops.
Line 320 dereferences node.chain_helper without CHECK_NONFATAL, unlike Line 310. This can crash on edge paths where the first loop doesn’t hit the guarded call.
Suggested fix
+ const auto& superblock_manager = *CHECK_NONFATAL(CHECK_NONFATAL(node.chain_helper)->superblocks);
+
for (int h = nStartHeight; h <= nChainTipHeight; h++) {
const CBlockIndex* pIndex = pindexTip->GetAncestor(h - 1);
auto payee = node.dmnman->GetListForBlock(pIndex).GetMNPayee(pIndex);
if (payee) {
- std::string strPayments = GetRequiredPaymentsString(*CHECK_NONFATAL(node.chain_helper)->superblocks,
- tip_mn_list, h, payee);
+ std::string strPayments = GetRequiredPaymentsString(superblock_manager, tip_mn_list, h, payee);
if (!strFilter.empty() && strPayments.find(strFilter) == std::string::npos) continue;
obj.pushKV(strprintf("%d", h), strPayments);
}
}
@@
- std::string strPayments = GetRequiredPaymentsString(*node.chain_helper->superblocks, tip_mn_list, h, projection[i]);
+ std::string strPayments = GetRequiredPaymentsString(superblock_manager, tip_mn_list, h, projection[i]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rpc/masternode.cpp` at line 320, The second winners loop dereferences
node.chain_helper when calling GetRequiredPaymentsString (using tip_mn_list and
projection[i]) without the CHECK_NONFATAL guard used earlier; add the same
non-null check for node.chain_helper before any use in that loop (i.e., ensure
CHECK_NONFATAL(node.chain_helper) or equivalent guard surrounds the call to
GetRequiredPaymentsString and any other accesses to node.chain_helper in the
second winner loop) so both loops consistently validate node.chain_helper before
dereferencing.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward findings from prior runs remain valid at HEAD 51a12ae: the governance INV handling regression for -disablegovernance nodes, the AddTrigger emplace-before-expiry-check leak, and the Clear() / m_loaded contract asymmetry. The latest delta from 1382bfa introduced no new defects beyond formatting/line-break churn; the prior embedded NUL byte issue in superblock.cpp is fixed.
Reviewed commit: 51a12ae
🟡 1 suggestion(s) | 💬 2 nitpick(s)
3 finding(s) posted in the review body
suggestion: Governance INV handling regresses for `-disablegovernance` nodes
src/net_processing.cpp (lines 2368-2373)
PeerManagerImpl::AlreadyHave() now answers MSG_GOVERNANCE_OBJECT / MSG_GOVERNANCE_OBJECT_VOTE by iterating m_handlers and falling through to return false when none claims the INV. NetGovernance is the only handler covering those types (src/governance/net_governance.cpp:203-208), and init.cpp:2273 only registers it when is_governance_enabled is true. With -disablegovernance, the handler is absent, so every governance INV is reported as "not have" and the peer goes on to issue GETDATA for data it cannot process. The prior call !m_govman.ConfirmInventoryRequest(inv) short-circuited via IsBlockchainSynced() and also tracked request state in m_requested_hash_time; both behaviors are now bypassed for disabled-governance nodes. Either gate this case explicitly (return true when no handler claims it) or keep calling into CGovernanceManager for the disabled path.
nitpick: `AddTrigger` leaves expired trigger registered when returning false
src/governance/superblock.cpp (lines 452-455)
SuperblockManager::AddTrigger() emplaces the new trigger into m_triggers at line 453 and only afterwards returns !pSuperblock->IsExpired(cachedHeight). When the trigger is already height-expired the function returns false; the caller in CGovernanceManager::AddGovernanceObjectInternal() (governance.cpp:304-307) interprets that as a failed add and marks govobj for deletion without calling RemoveTrigger(). The expired entry then remains in m_triggers until the next Clean() sweep, contradicting the contract in superblock.h:133-135 and leaking a stale entry to readers such as GetActiveTriggers() (which does not filter by expiry). Either erase before returning false, or check IsExpired before the emplace.
nitpick: `SuperblockManager::Clear` does not reset `m_loaded`
src/governance/superblock.cpp (lines 520-523)
Clear() empties m_triggers but leaves m_loaded untouched, while IsValid() (superblock.h:130) is defined entirely in terms of m_loaded. After a Clear() the manager still reports IsValid()==true against an empty trigger set, which is the opposite of what the API name suggests. The reachable impact is low today — CGovernanceManager::Clear() is not currently invoked from any production path and the flat-database load path calls GovernanceStore::Clear() (the base class), which does not touch the superblock manager — but the extracted class now exposes the asymmetry as its own contract issue. Calling SetLoaded(false) from Clear() would make the API self-consistent.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] In `src/net_processing.cpp`:2368-2373: Governance INV handling regresses for `-disablegovernance` nodes
`PeerManagerImpl::AlreadyHave()` now answers `MSG_GOVERNANCE_OBJECT` / `MSG_GOVERNANCE_OBJECT_VOTE` by iterating `m_handlers` and falling through to `return false` when none claims the INV. `NetGovernance` is the only handler covering those types (src/governance/net_governance.cpp:203-208), and `init.cpp:2273` only registers it when `is_governance_enabled` is true. With `-disablegovernance`, the handler is absent, so every governance INV is reported as "not have" and the peer goes on to issue GETDATA for data it cannot process. The prior call `!m_govman.ConfirmInventoryRequest(inv)` short-circuited via `IsBlockchainSynced()` and also tracked request state in `m_requested_hash_time`; both behaviors are now bypassed for disabled-governance nodes. Either gate this case explicitly (return true when no handler claims it) or keep calling into `CGovernanceManager` for the disabled path.
- [NITPICK] In `src/governance/superblock.cpp`:452-455: `AddTrigger` leaves expired trigger registered when returning false
`SuperblockManager::AddTrigger()` emplaces the new trigger into `m_triggers` at line 453 and only afterwards returns `!pSuperblock->IsExpired(cachedHeight)`. When the trigger is already height-expired the function returns `false`; the caller in `CGovernanceManager::AddGovernanceObjectInternal()` (governance.cpp:304-307) interprets that as a failed add and marks `govobj` for deletion without calling `RemoveTrigger()`. The expired entry then remains in `m_triggers` until the next `Clean()` sweep, contradicting the contract in `superblock.h:133-135` and leaking a stale entry to readers such as `GetActiveTriggers()` (which does not filter by expiry). Either erase before returning false, or check `IsExpired` before the emplace.
- [NITPICK] In `src/governance/superblock.cpp`:520-523: `SuperblockManager::Clear` does not reset `m_loaded`
`Clear()` empties `m_triggers` but leaves `m_loaded` untouched, while `IsValid()` (superblock.h:130) is defined entirely in terms of `m_loaded`. After a `Clear()` the manager still reports `IsValid()==true` against an empty trigger set, which is the opposite of what the API name suggests. The reachable impact is low today — `CGovernanceManager::Clear()` is not currently invoked from any production path and the flat-database load path calls `GovernanceStore::Clear()` (the base class), which does not touch the superblock manager — but the extracted class now exposes the asymmetry as its own contract issue. Calling `SetLoaded(false)` from `Clear()` would make the API self-consistent.
Inline posting failed; posted findings in the top-level review body.
|
Some issues found by claw look real, pls consider 1316271 |
3 participants
Issue being fixed or feature implemented
The
dash-chainstate(see #7234) requires as dependency full governance implementation, even if it is not functional or relevant to dash-chainstate (the governance uses off-chain objects and they can't be retrieved without p2p communication).It brings extra files to compilation unit of kernel / dash-chainstate:
Also, Governance blocks backporting https://github.com/bitcoin/bitcoin/pull/25064/files because it uses timedata.cpp
Also, Governance requires extra dependency of node/txindex (and recursively index/base which is out-of-scope for kernel)
What was done?
The only usage of governance in coinsensus code is a validation of superblocks (CMNPaymentsProcessor); superblock validator has been extracted from CGovernanceManager to own file and class
governance::SuperblockManager.It can be simply replaced to stub NullSuperblockmanager, same as
class NullNodeSyncNotifier final : public NodeSyncNotifierfor kernel project.There are also several relevant refactorings:
CGovernanceExceptionmoved togovernance/object.hwhere it's usedCProposalValidatoris replaced by a simple helpergovernance::ValidateProposaland moved to `governance/object.h'CGovermentManagershould not know anything about signer if it's not a masternode. Abstract interfaceGovernanceSignerParentis removed.How Has This Been Tested?
2 circular dependencies disappeared:
Unit & functional tests succeed.
Mock proposal is created on testnet with Dash-Qt.
Breaking Changes
N/A
Checklist: