refactor: drop dependency of evo/specialtxman on validation.h - #7668
refactor: drop dependency of evo/specialtxman on validation.h#7668knst wants to merge 15 commits into
Conversation
It doesn't change logic but simplify further refactoring and reduces conflicts with other PRs in the future
… in CSpecialTxProcessor
…ecalculation twice GetBlockSubsidy is leightful helper and appears in perf as responsible for only 0.01% of calculation, so, commit is not threated as any noticeable performance improvement
CheckSpecialTxInner still evaluates the flag from m_chainman for now and the next step is moving that to its callers
|
🕓 Ready for review — 15 ahead in queue (commit f932324) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe change removes several Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to This refactor passes explicit consensus and validation context through special-transaction processing without any identified current-head correctness or operational risk. Sequence Diagram(s)sequenceDiagram
participant MemPoolAccept
participant CSpecialTxProcessor
participant ConsensusParams
participant Chainstate
MemPoolAccept->>ConsensusParams: Determine V24 activation
MemPoolAccept->>CSpecialTxProcessor: Check special transaction
CSpecialTxProcessor->>Chainstate: Validate against chain context
CSpecialTxProcessor-->>MemPoolAccept: Return validation result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
ProcessSpecialTxsInBlock called Chainstate::RecordBackgroundMNListHash, the last reason it needed the complete Chainstate type.
Make MNListUpdates the single non-optional result: ProcessBlock always fill it and consumers checks if there's actually diff non-empty by call of diff.HasChanges() instead of the checking optional. Copying an unchanged list is leightful operation due to using immer maps data-structure.
There's several new exception has been added to the list of existing circular dependencies: all of them had been pre-existing but now they are discovered by removing the shorter loop specialtxman <-> validation
4a1b3b7 to
f932324
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: f93232473f
ℹ️ 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".
| blockundo.vtxundo.reserve(block.vtx.size() - 1); | ||
| bool fDIP0001Active_context = DeploymentActiveAt(*pindex, params.GetConsensus(), Consensus::DEPLOYMENT_DIP0001); | ||
|
|
||
| const CAmount blockSubsidy = GetBlockSubsidy(pindex, params.GetConsensus()); |
There was a problem hiding this comment.
When benchmark logging is enabled, this moved call now runs between time_2 and time_2_1, so its cost is charged to ProcessSpecialTxsInBlock; meanwhile the unchanged time_subsidy calculation still measures time_5_1 to time_5_2, where no subsidy calculation occurs. This makes the reported GetBlockSubsidy time nearly zero and contaminates the special-transaction timing, so the timing boundaries should move with the call.
AGENTS.md reference: AGENTS.md:L204-L220
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Both supplied findings are confirmed: the refactor leaves subsidy benchmark accounting at the old location, and one intermediate commit contains a syntax error repaired by its successor. Neither is a blocking correctness issue at HEAD. Verification used the exact source and commit diffs; the reviewers’ reported build and test results were not independently rerun.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — Despite being framed as dependency cleanup, this cross-cutting refactor changes how consensus parameters, deployment activation, block subsidies, and deterministic masternode state flow through special-transaction validation and block connection, where regressions could cause consensus divergence or incorrect persisted state. - Phase 1 reviewers: not run (skipped for throughput: 25 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer
🟡 1 suggestion(s) | 💬 1 nitpick(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:4862-4865: Fold the RollforwardBlock brace repair into e961d9c180
Commit e961d9c180 removes the opening `{` from this conditional but retains its closing `}`, prematurely ending Chainstate::RollforwardBlock and leaving the following statements at namespace scope. Commit e1f7c1f991 restores the brace, so HEAD is correct, but the preceding commit cannot compile and would need to be skipped during bisection. Move the brace repair into e961d9c180 while keeping the MNListUpdates interface refactor in e1f7c1f991.
- [NITPICK] src/validation.cpp:2385: Move the subsidy benchmark alongside the calculation
The moved GetBlockSubsidy call now executes between time_2 and time_2_1, so its cost is included in the ProcessSpecialTxsInBlock benchmark. However, lines 2545–2550 still accumulate and report GetBlockSubsidy timing from time_5_1 to time_5_2, an interval that no longer contains the calculation. This makes the benchmark labels misleading when comparing validation costs across the refactor. Move the subsidy timing boundaries and accumulator update alongside the calculation, and adjust the surrounding interval to avoid double-counting.
| if (!m_chain_helper->special_tx->ProcessSpecialTxsInBlock(*this, m_chain, block, pindex, is_v24_active, inputs, blockSubsidy, /*fJustCheck=*/false, /*fCheckCbTxMerkleRoots=*/false, state, mnlist_updates)) { | ||
| return error("RollforwardBlock(DASH): ProcessSpecialTxsInBlock for block %s failed with %s", | ||
| pindex->GetBlockHash().ToString(), state.ToString()); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Fold the RollforwardBlock brace repair into e961d9c
Commit e961d9c removes the opening { from this conditional but retains its closing }, prematurely ending Chainstate::RollforwardBlock and leaving the following statements at namespace scope. Commit e1f7c1f restores the brace, so HEAD is correct, but the preceding commit cannot compile and would need to be skipped during bisection. Move the brace repair into e961d9c while keeping the MNListUpdates interface refactor in e1f7c1f.
source: ['claude']
| blockundo.vtxundo.reserve(block.vtx.size() - 1); | ||
| bool fDIP0001Active_context = DeploymentActiveAt(*pindex, params.GetConsensus(), Consensus::DEPLOYMENT_DIP0001); | ||
|
|
||
| const CAmount blockSubsidy = GetBlockSubsidy(pindex, params.GetConsensus()); |
There was a problem hiding this comment.
💬 Nitpick: Move the subsidy benchmark alongside the calculation
The moved GetBlockSubsidy call now executes between time_2 and time_2_1, so its cost is included in the ProcessSpecialTxsInBlock benchmark. However, lines 2545–2550 still accumulate and report GetBlockSubsidy timing from time_5_1 to time_5_2, an interval that no longer contains the calculation. This makes the benchmark labels misleading when comparing validation costs across the refactor. Move the subsidy timing boundaries and accumulator update alongside the calculation, and adjust the surrounding interval to avoid double-counting.
source: ['claude']
thepastaclaw
added
the
pastaclaw:commented
2 participants
Issue being fixed or feature implemented
One more circular dependency is trivial to avoid.
What was done?
It helps to break circular dependency over evo/specialtxman <-> validation and reveal other circular dependencies that has been hidden by this short cycle.
NOTE: once v24 is buried deployment, some code could be re-refactored and simplified because chainman is not needed for buried deployments.
How Has This Been Tested?
Run & updated test/lint/lint-circular-dependencies.py
Breaking Changes
N/A
Checklist: