refactor: rename hard-fork v23 to hard-fork v24 #6847
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughReplaces deployment identifier DEPLOYMENT_V23 with DEPLOYMENT_V24 across consensus, chainparams, deploymentinfo, rpc, Evo (creditpool, deterministicmns, providertx, specialtxman), and functional tests. Migrates the V23 deployment configuration (bit, start/timeout, window/thresholds, falloff, useEHF) to the V24 slot for MAIN, TESTNET, DEVNET, and REGTEST. Renames the consensus enum member to DEPLOYMENT_V24 and renames LimitAmountV23 → LimitAmountV24. Test expectations and getblockchaininfo reporting updated to use "v24". Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 3
🧹 Nitpick comments (4)
src/evo/creditpool.cpp (1)
192-196: Use passed consensusParams instead of global Params() in activation checks.Within this function you already receive consensusParams; mixing in Params().GetConsensus() can lead to subtle inconsistencies on devnet/regtest overrides. Prefer the passed instance for all checks here.
Apply locally within the changed hunk:
- if (DeploymentActiveAt(*block_index, Params().GetConsensus(), Consensus::DEPLOYMENT_V24)) { + if (DeploymentActiveAt(*block_index, consensusParams, Consensus::DEPLOYMENT_V24)) { - } else if (DeploymentActiveAt(*block_index, Params().GetConsensus(), Consensus::DEPLOYMENT_WITHDRAWALS)) { + } else if (DeploymentActiveAt(*block_index, consensusParams, Consensus::DEPLOYMENT_WITHDRAWALS)) {Additionally (outside this hunk), consider adjusting the earlier DIP0003 gate similarly:
// earlier in this function/file if (!DeploymentActiveAt(*block_index, consensusParams, Consensus::DEPLOYMENT_DIP0003)) { ... }test/functional/feature_asset_locks.py (2)
705-709: Assert activation after enabling v24.Mirror the withdrawals test by asserting the softfork is active to tighten the check.
def test_v24_fork(self, node_wallet, node, pubkey): self.log.info("Testing asset unlock after 'v24' activation...") self.activate_by_name('v24', 750) + assert softfork_active(node_wallet, 'v24') self.log.info(f'post-v24 height: {node.getblockcount()} credit: {self.get_credit_pool_balance()}')
742-759: Reuse helper for “not mined” assertions.For consistency with earlier tests, consider using ensure_tx_is_not_mined(txid_in_block) instead of manual membership checks.
src/evo/deterministicmns.cpp (1)
1012-1018: Doc nit: clarify scope of enforcement.Note that IsVersionChangeValid() is only called by ProUp* txs, not ProRegTx. Consider adding that explicitly to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
-
src/chainparams.cpp(4 hunks) -
src/consensus/params.h(1 hunks) -
src/deploymentinfo.cpp(1 hunks) -
src/evo/creditpool.cpp(1 hunks) -
src/evo/creditpool.h(1 hunks) -
src/evo/deterministicmns.cpp(2 hunks) -
src/evo/providertx.cpp(1 hunks) -
src/evo/specialtxman.cpp(2 hunks) -
src/rpc/blockchain.cpp(1 hunks) -
test/functional/feature_asset_locks.py(2 hunks) -
test/functional/rpc_blockchain.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/rpc_blockchain.pytest/functional/feature_asset_locks.py
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/rpc/blockchain.cppsrc/evo/creditpool.cppsrc/evo/creditpool.hsrc/evo/specialtxman.cppsrc/consensus/params.hsrc/evo/providertx.cppsrc/evo/deterministicmns.cppsrc/deploymentinfo.cppsrc/chainparams.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/creditpool.cppsrc/evo/creditpool.hsrc/evo/specialtxman.cppsrc/evo/providertx.cppsrc/evo/deterministicmns.cpp
🧠 Learnings (4)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/evo/specialtxman.cppsrc/evo/providertx.cppsrc/evo/deterministicmns.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Applied to files:
src/evo/specialtxman.cpp
📚 Learning: 2025-07-09T15:02:26.899Z
Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Applied to files:
src/evo/deterministicmns.cpp
🧬 Code graph analysis (3)
src/evo/creditpool.cpp (2)
src/chainparams.cpp (2)
Params(1354-1357)Params(1354-1354)src/evo/creditpool.h (2)
currentLimit(42-42)latelyUnlocked(43-43)
src/evo/deterministicmns.cpp (1)
src/chainparams.cpp (2)
Params(1354-1357)Params(1354-1354)
test/functional/feature_asset_locks.py (1)
test/functional/test_framework/test_framework.py (1)
activate_by_name(1539-1578)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/providertx.cpp
[error] 22-30: Clang format differences detected in src/evo/providertx.cpp. The check command 'git diff -U0 origin/develop --
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (12)
src/evo/creditpool.h (1)
124-125: LimitAmountV24 rename LGTM.Value and type unchanged; aligns with the doubled withdrawal cap.
src/rpc/blockchain.cpp (1)
1564-1567: getblockchaininfo now reports EHF “v24” — OK; ensure deploymentinfo name matches.The new loop entry is correct. Confirm that deploymentinfo.cpp exposes "v24" so tests/rpc see a matching key.
test/functional/rpc_blockchain.py (1)
219-229: Expectation updated to “v24” — OK; verify EHF flag is true for V24.The structure and values look consistent. Ensure chainparams marks V24 deployment with useEHF=true so “ehf”: True assertion holds.
src/deploymentinfo.cpp (1)
15-17: VersionBits name switched to "v24" — OK.Index order (“testdummy”, then “v24”) still matches DeploymentPos.
test/functional/feature_asset_locks.py (1)
269-270: Renamed call to test_v24_fork — OK.src/evo/providertx.cpp (1)
26-27: Extaddr gating moved to V24 — OK; clang-format failing (action required)Deployment switch matches the PR intent.
CI reports clang-format diffs for src/evo/providertx.cpp; repro here failed because 'clang-format' is not installed — run the project's formatter and amend the commit.CI repro command:
git diff -U0 origin/develop -- src/evo/providertx.cpp | ./contrib/devtools/clang-format-diff.py -p1src/consensus/params.h (1)
43-46: DEPLOYMENT_V24 rename verified — no code-level V23 remnants
- src/consensus/params.h: DEPLOYMENT_V24 is the second DeploymentPos.
- src/deploymentinfo.cpp: VersionBitsDeploymentInfo[1].name == "v24".
- src/chainparams.cpp: vDeployments[Consensus::DEPLOYMENT_V24] is initialized for main/test/devnet/regtest (bit=12, useEHF=true) — values vary per network but are consistent.
- No DEPLOYMENT_V23 symbol found in tracked source; remaining "v23" occurrences are limited to docs/comments/tests (e.g. doc/release-notes-.md, src/evo/providertx.h, src/llmq/quorums.cpp, test/).
src/evo/deterministicmns.cpp (1)
1022-1025: LGTM: v24 gate for version-change rules is correct.Matches the PR’s rename intent.
src/chainparams.cpp (4)
410-417: V24 deployment parameters added (testnet).Same note: confirm start/timeout before enabling signaling.
583-590: Devnet V24 start time fixed to 2025-07-01.Confirm this date is intentional and matches your current devnet plans; otherwise update.
820-827: Regtest V24 params.LGTM for tests; EHF enabled with full-window thresholds.
212-219: V24 deployment parameters added — bit 12 use verified; mainnet start/timeout still TODO
- Bit 12 is only assigned to DEPLOYMENT_V24 in src/chainparams.cpp (lines 212, 410, 583, 820).
- Bit/window/thresholds match prior v23; mainnet nStartTime = NEVER_ACTIVE and nTimeout = NO_TIMEOUT — set concrete nStartTime/nTimeout before tagging a release.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/evo/specialtxman.cpp (1)
209-209: V24 gate constant is correct (fix matches prior feedback).Now uses DEPLOYMENT_V24 (not V25). This aligns with deterministicmns.cpp and resolves the earlier mismatch.
Run to confirm there are no remaining V24/V25 mismatches in Evo code:
#!/bin/bash rg -n -C2 --type=cpp 'is_v2[45]|DEPLOYMENT_V2[45]' src/evo
🧹 Nitpick comments (1)
test/functional/rpc_blockchain.py (1)
224-224: Prefer symbolic INT64_MAX over a magic number.Use an expression for clarity and to avoid retyping the long literal.
Apply:
- 'timeout': 9223372036854775807, # "v24" does not have a timeout so is set to the max int64 value + 'timeout': (1 << 63) - 1, # max int64 for "no timeout"Optional (outside this hunk): make the same change for testdummy’s timeout and/or define at module top:
INT64_MAX = (1 << 63) - 1Then use INT64_MAX in both places.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/chainparams.cpp(4 hunks)src/consensus/params.h(1 hunks)src/deploymentinfo.cpp(1 hunks)src/evo/creditpool.cpp(1 hunks)src/evo/creditpool.h(1 hunks)src/evo/deterministicmns.cpp(2 hunks)src/evo/providertx.cpp(1 hunks)src/evo/specialtxman.cpp(2 hunks)src/rpc/blockchain.cpp(1 hunks)test/functional/feature_asset_locks.py(2 hunks)test/functional/rpc_blockchain.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/deploymentinfo.cpp
- src/evo/providertx.cpp
- src/evo/creditpool.h
- src/evo/creditpool.cpp
- src/consensus/params.h
- src/rpc/blockchain.cpp
- test/functional/feature_asset_locks.py
- src/chainparams.cpp
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/evo/deterministicmns.cppsrc/evo/specialtxman.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/deterministicmns.cppsrc/evo/specialtxman.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/rpc_blockchain.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/evo/deterministicmns.cppsrc/evo/specialtxman.cpp
📚 Learning: 2025-07-09T15:02:26.899Z
Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Applied to files:
src/evo/deterministicmns.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (4)
test/functional/rpc_blockchain.py (1)
219-229: LGTM — v24 rename and deployment wiring verified.Expectations match PR intent (EHF=true, status=defined, active=false). No DEPLOYMENT_V23 usages found; only "v23" left is a test comment at test/functional/wallet_upgradetohd.py:202 (no change needed). DEPLOYMENT_V24 is wired in src/chainparams.cpp and referenced from src/rpc/blockchain.cpp and evo/*.cpp.
src/evo/specialtxman.cpp (1)
320-323: ProUpServTx state-version update gated at V24 — LGTM.Version bump is applied only after V24; validation is already enforced in CheckProUpServTx via IsVersionChangeValid().
src/evo/deterministicmns.cpp (2)
1012-1026: IsVersionChangeValid(): V24 activation gate — LGTM.Documentation and the DeploymentActiveAfter() check both reference DEPLOYMENT_V24 consistently.
1053-1057: ExtAddr gating already enforced in CProRegTx::IsTriviallyValid — no change required.CProRegTx::IsTriviallyValid rejects nVersion > ProTxVersion::GetMaxFromDeployment(pindexPrev) (bad-protx-version) and also validates netInfo capability vs (nVersion == ProTxVersion::ExtAddr) (bad-protx-netinfo-version), so ExtAddr ProRegTx are rejected until V24 activation. See src/evo/providertx.cpp (IsTriviallyValid, ~lines 35–61) and the V24 check at src/evo/deterministicmns.cpp:1053–1057.
Issue being fixed or feature implemented
It is one of alternative options for next release; release v23 with breaking changes but without hard-fork.
Other option to backport some changes and release v22.2, such as #6848
What was done?
Hard-fork
v23is renamed tov24; updated text commentaries and functional tests.How Has This Been Tested?
Run unit & functional tests
Breaking Changes
Hard-fork "v23" is renamed to hard-fork "v24".
Checklist: