refactor: drop circular dependency over validation <-> masternode/payment#7339
refactor: drop circular dependency over validation <-> masternode/payment#7339knst wants to merge 6 commits into
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit c75e64a) |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThis PR introduces a Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/validation.cpp (1)
2571-2581:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass this chainstate’s chain into payment validation.
ConnectBlock()runs on whicheverCChainStateis being advanced, but these calls still threadm_chainman.ActiveChain().IsBlockValueValid()uses that chain forCSuperblock::GetPaymentsLimit(...), andIsBlockPayeeValid()uses it for superblock/governance checks, so background/snapshot validation can evaluate a block against the wrong chain context. Passm_chain(or a local reference to this chainstate’s chain) instead.Suggested fix
+ const CChain& chain = m_chain; - if (!m_chain_helper->mn_payments->IsBlockValueValid(m_chainman.ActiveChain(), block, pindex->nHeight, blockSubsidy + feeReward, strError, check_superblock)) { + if (!m_chain_helper->mn_payments->IsBlockValueValid(chain, block, pindex->nHeight, blockSubsidy + feeReward, strError, check_superblock)) { // NOTE: Do not punish, the node might be missing governance data LogPrintf("ERROR: ConnectBlock(DASH): %s\n", strError); return state.Invalid(BlockValidationResult::BLOCK_RESULT_UNSET, "bad-cb-amount"); } @@ - if (!m_chain_helper->mn_payments->IsBlockPayeeValid(m_chainman.ActiveChain(), *block.vtx[0], pindex->pprev, blockSubsidy, feeReward, mn_reward_era, check_superblock)) { + if (!m_chain_helper->mn_payments->IsBlockPayeeValid(chain, *block.vtx[0], pindex->pprev, blockSubsidy, feeReward, mn_reward_era, check_superblock)) { // NOTE: Do not punish, the node might be missing governance data LogPrintf("ERROR: ConnectBlock(DASH): couldn't find masternode or superblock payments\n"); return state.Invalid(BlockValidationResult::BLOCK_RESULT_UNSET, "bad-cb-payee"); }🤖 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/validation.cpp` around lines 2571 - 2581, ConnectBlock is calling m_chain_helper->mn_payments->IsBlockValueValid(...) and IsBlockPayeeValid(...) with m_chainman.ActiveChain(), which can be the wrong chain during background/snapshot validation; change those calls to pass this CChainState's chain (use m_chain or a local reference to m_chain) instead of m_chainman.ActiveChain() so CSuperblock::GetPaymentsLimit and superblock/governance checks use the correct chain context (update the two call sites that reference IsBlockValueValid and IsBlockPayeeValid to pass m_chain).
🤖 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/validation.cpp`:
- Around line 2571-2581: ConnectBlock is calling
m_chain_helper->mn_payments->IsBlockValueValid(...) and IsBlockPayeeValid(...)
with m_chainman.ActiveChain(), which can be the wrong chain during
background/snapshot validation; change those calls to pass this CChainState's
chain (use m_chain or a local reference to m_chain) instead of
m_chainman.ActiveChain() so CSuperblock::GetPaymentsLimit and
superblock/governance checks use the correct chain context (update the two call
sites that reference IsBlockValueValid and IsBlockPayeeValid to pass m_chain).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ec84e219-dc6e-4c80-954b-025e317994e1
📒 Files selected for processing (10)
src/evo/chainhelper.cppsrc/evo/creditpool.cppsrc/masternode/payments.cppsrc/masternode/payments.hsrc/node/miner.cppsrc/rpc/masternode.cppsrc/test/block_reward_reallocation_tests.cppsrc/validation.cppsrc/validation.htest/lint/lint-circular-dependencies.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean refactor PR that decouples CMNPaymentsProcessor from ChainstateManager and replaces a boolean fV20Active flag with an MnRewardEra enum computed by callers via a new GetMnRewardEraAfter helper. Both verifier agents converged on no blocking findings, and the two nitpicks raised were low-signal observations dropped during verification.
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
refactor-validation-payments
branch
from
dc2e5ea to
37ad935
Compare
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
refactor-validation-payments
branch
from
37ad935 to
fc05d3e
Compare
|
This pull request has conflicts, please rebase. |
… helpers It helps to minimize dependencies of masternode/payments on validation.h and move calls such as is-deployment-active outside to callers
knst
force-pushed
the
refactor-validation-payments
branch
from
fc05d3e to
c75e64a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/masternode/payments.cpp (1)
73-93: 💤 Low valueConsider
constexpr std::arrayfor the immutable period table.Since
vecPeriodsis never modified after initialization, usingstatic constexpr std::arrayprovides compile-time initialization and clearer const semantics.♻️ Suggested change
- // Periods used to reallocate the masternode reward from 50% to 60% - static std::vector<int> vecPeriods{ + // Periods used to reallocate the masternode reward from 50% to 60% + static constexpr std::array<int, 19> vecPeriods{ 513, // Period 1: 51.3% 526, // Period 2: 52.6% ... 600 // Period 19: 60% };🤖 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/masternode/payments.cpp` around lines 73 - 93, Replace the `static std::vector<int> vecPeriods` declaration with `static constexpr std::array<int, 19> vecPeriods` to provide compile-time initialization and clearer const semantics for the immutable period table. Change the vector initialization syntax to array initialization syntax while keeping all 19 period values unchanged. This change makes the fixed-size, never-modified collection more efficient and semantically explicit.
🤖 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 dereference of node.chain_helper in the
GetRequiredPaymentsString call lacks a null pointer guard, creating a potential
crash path. Add a CHECK_NONFATAL guard for node.chain_helper before this
dereference, mirroring the pattern used in the earlier loop at line 310 to
ensure consistent safety checks throughout the function.
---
Nitpick comments:
In `@src/masternode/payments.cpp`:
- Around line 73-93: Replace the `static std::vector<int> vecPeriods`
declaration with `static constexpr std::array<int, 19> vecPeriods` to provide
compile-time initialization and clearer const semantics for the immutable period
table. Change the vector initialization syntax to array initialization syntax
while keeping all 19 period values unchanged. This change makes the fixed-size,
never-modified collection more efficient and semantically explicit.
🪄 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: e0bc48bf-12a0-48b5-a6e5-810772bd16f1
📒 Files selected for processing (10)
src/evo/chainhelper.cppsrc/evo/creditpool.cppsrc/masternode/payments.cppsrc/masternode/payments.hsrc/node/miner.cppsrc/rpc/masternode.cppsrc/test/block_reward_reallocation_tests.cppsrc/validation.cppsrc/validation.htest/lint/lint-circular-dependencies.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/evo/creditpool.cpp
- src/validation.h
- src/test/block_reward_reallocation_tests.cpp
- src/node/miner.cpp
- src/masternode/payments.h
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/masternode/payments.cpp (1)
73-93: 💤 Low valueConsider
constexpr std::arrayfor the immutable period table.Since
vecPeriodsis never modified after initialization, usingstatic constexpr std::arrayprovides compile-time initialization and clearer const semantics.♻️ Suggested change
- // Periods used to reallocate the masternode reward from 50% to 60% - static std::vector<int> vecPeriods{ + // Periods used to reallocate the masternode reward from 50% to 60% + static constexpr std::array<int, 19> vecPeriods{ 513, // Period 1: 51.3% 526, // Period 2: 52.6% ... 600 // Period 19: 60% };🤖 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/masternode/payments.cpp` around lines 73 - 93, Replace the `static std::vector<int> vecPeriods` declaration with `static constexpr std::array<int, 19> vecPeriods` to provide compile-time initialization and clearer const semantics for the immutable period table. Change the vector initialization syntax to array initialization syntax while keeping all 19 period values unchanged. This change makes the fixed-size, never-modified collection more efficient and semantically explicit.
🤖 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 dereference of node.chain_helper in the
GetRequiredPaymentsString call lacks a null pointer guard, creating a potential
crash path. Add a CHECK_NONFATAL guard for node.chain_helper before this
dereference, mirroring the pattern used in the earlier loop at line 310 to
ensure consistent safety checks throughout the function.
---
Nitpick comments:
In `@src/masternode/payments.cpp`:
- Around line 73-93: Replace the `static std::vector<int> vecPeriods`
declaration with `static constexpr std::array<int, 19> vecPeriods` to provide
compile-time initialization and clearer const semantics for the immutable period
table. Change the vector initialization syntax to array initialization syntax
while keeping all 19 period values unchanged. This change makes the fixed-size,
never-modified collection more efficient and semantically explicit.
🪄 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: e0bc48bf-12a0-48b5-a6e5-810772bd16f1
📒 Files selected for processing (10)
src/evo/chainhelper.cppsrc/evo/creditpool.cppsrc/masternode/payments.cppsrc/masternode/payments.hsrc/node/miner.cppsrc/rpc/masternode.cppsrc/test/block_reward_reallocation_tests.cppsrc/validation.cppsrc/validation.htest/lint/lint-circular-dependencies.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/evo/creditpool.cpp
- src/validation.h
- src/test/block_reward_reallocation_tests.cpp
- src/node/miner.cpp
- src/masternode/payments.h
🛑 Comments failed to post (1)
src/rpc/masternode.cpp (1)
320-320:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
node.chain_helperbefore dereference in projected payees path.Line 320 dereferences
node.chain_helperwithout a check, unlike the earlier loop at Line 310. ReuseCHECK_NONFATALhere to avoid a null-pointer crash path.Suggested fix
- std::string strPayments = GetRequiredPaymentsString(*node.chain_helper->superblocks, tip_mn_list, h, projection[i]); + std::string strPayments = GetRequiredPaymentsString(*CHECK_NONFATAL(node.chain_helper)->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` at line 320, The dereference of node.chain_helper in the GetRequiredPaymentsString call lacks a null pointer guard, creating a potential crash path. Add a CHECK_NONFATAL guard for node.chain_helper before this dereference, mirroring the pattern used in the earlier loop at line 310 to ensure consistent safety checks throughout the function.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean behavior-preserving refactor that removes the cycle between masternode/payments and validation by threading explicit chain/era/superblock-check context. Two in-scope suggestions: the new explicit chain parameter still uses the global active chain inside a per-chainstate method, and the new is_v24 flag is reconstructed from a superblock-check enum that conflates two orthogonal concepts.
🟡 2 suggestion(s)
🤖 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/validation.cpp`:
- [SUGGESTION] src/validation.cpp:2428-2438: Pass this chainstate's m_chain into payment validation instead of m_chainman.ActiveChain()
`CChainState::ConnectBlock` is a per-chainstate method (validation.h:563 defines the instance's own `m_chain`), but both `IsBlockValueValid` and `IsBlockPayeeValid` are called with `m_chainman.ActiveChain()`. With background/snapshot validation, `ConnectBlock` can run on a chainstate that is not the active one, in which case the chain passed to payment validation describes a different tip than the block being connected. The previous implementation hid this same coupling by calling `m_chainman.ActiveChain()` internally, so this PR preserves rather than introduces the bug — but now that the chain is an explicit parameter, the natural fix is to thread `m_chain` here so the new abstraction is correct by construction.
In `src/masternode/payments.cpp`:
- [SUGGESTION] src/masternode/payments.cpp:320-365: is_v24 reconstructed from SuperBlockCheckType conflates two orthogonal concepts
At payments.cpp:320 and :365, `is_v24` is derived as `check_superblock == SuperBlockCheckType::DisallowDuplicates`. This works only because the sole production caller in `ConnectBlock` (validation.cpp:2422-2425) folds two independent signals — `IsSuperblockValidationRequired` and `DEPLOYMENT_V24` active — into the same enum value. Pre-refactor, `is_v24` was computed locally from `DeploymentActiveAfter(..., DEPLOYMENT_V24)` independent of whether the superblock check ran. A future caller (RPC, fuzz harness, or test) that wants V24-active duplicate detection but with a different superblock-check policy has no way to express that, and silently downgrading to `AllowDuplicates` won't trip any assertion. Consider either passing V24 as a separate parameter (keeping `SuperBlockCheckType` as a 2-state), deriving the era/V24 flag from `pindexPrev` inside payments via a small helper next to `GetMnRewardEraAfter`, or at minimum asserting at the caller that `DisallowDuplicates` is only used when V24 is active.
2 participants
Issue being fixed or feature implemented
There's an exception for existing circular dependency in test/lint/lint-circular-dependencies.py:
"masternode/payments -> validation -> masternode/payments",What was done?
MnRewardErato use in masternode/payment helpersHow Has This Been Tested?
Run unit & functional tests; run linter
test/lint/lint-circular-dependencies.pyPlease note, that 2 other circular dependencies always had existed but had been hidden behind
masternode/payment <-> validation:Breaking Changes
N/A
Checklist: