feat: fire up 🏃 test chains by first block (v20, mn_rr) - 9/9 #6667
Conversation
WalkthroughThe changes adjust activation heights and logic for consensus features, particularly "v20" and "MN_RR", in both the main codebase and test framework. In consensus parameter initialization, v20 activation is now explicitly tied to DIP0003 activation height, and MN_RR is set to activate no earlier than v20, with an assertion enforcing this order. Reward reallocation to the credit pool is gated by v20 activation to prevent premature fund allocation. Several test scripts update activation heights, expected reward values, and assertion logic to align with these new parameters, replacing explicit activation calls with state assertions and adding robustness in coin selection. The test framework introduces configurable Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (18)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used🧬 Code Graph Analysis (2)test/functional/feature_llmq_is_cl_conflicts.py (1)
test/functional/feature_llmq_evo.py (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (11)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/test_framework/test_framework.py (1)
602-602: Added TODO comment for future refactoring.The comment helps identify that this method should be moved to DashTestFramework where it more logically belongs. This is good documentation but should be addressed in a follow-up PR.
Consider creating a ticket for this refactoring to ensure it's not forgotten.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/chainparams.cpp(1 hunks)src/evo/creditpool.cpp(1 hunks)src/masternode/payments.cpp(1 hunks)src/test/util/setup_common.cpp(1 hunks)test/functional/feature_asset_locks.py(5 hunks)test/functional/feature_dip4_coinbasemerkleroots.py(2 hunks)test/functional/feature_governance.py(1 hunks)test/functional/feature_llmq_chainlocks.py(2 hunks)test/functional/feature_llmq_rotation.py(1 hunks)test/functional/feature_llmq_simplepose.py(1 hunks)test/functional/feature_mnehf.py(0 hunks)test/functional/rpc_blockchain.py(2 hunks)test/functional/rpc_masternode.py(2 hunks)test/functional/test_framework/test_framework.py(5 hunks)
💤 Files with no reviewable changes (1)
- test/functional/feature_mnehf.py
🧰 Additional context used
🧬 Code Graph Analysis (7)
test/functional/feature_llmq_simplepose.py (1)
test/functional/test_framework/test_framework.py (1)
delay_v20(1213-1214)
test/functional/feature_llmq_rotation.py (1)
test/functional/test_framework/test_framework.py (1)
delay_v20(1213-1214)
test/functional/feature_governance.py (1)
test/functional/test_framework/test_framework.py (1)
delay_v20(1213-1214)
test/functional/feature_dip4_coinbasemerkleroots.py (1)
test/functional/test_framework/test_framework.py (1)
delay_v20(1213-1214)
test/functional/feature_llmq_chainlocks.py (1)
test/functional/test_framework/test_framework.py (2)
delay_v20(1213-1214)activate_v20(1257-1258)
test/functional/rpc_masternode.py (1)
test/functional/test_framework/util.py (1)
assert_equal(64-69)
test/functional/test_framework/test_framework.py (1)
test/functional/test_framework/util.py (1)
append_config(418-421)
🪛 GitHub Actions: Clang Diff Format Check
src/masternode/payments.cpp
[error] 41-43: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / 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
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
🔇 Additional comments (29)
src/evo/creditpool.cpp (1)
245-247: Clear comment improvement regarding credit pool existenceThe updated comment clarifies the dependency between v20 activation and the credit pool functionality. This change aligns with the PR's objective to make v20 a prerequisite for credit pool features, which is consistent with the broader changes where v20 activation is now explicitly tied to DIP0003 activation height.
test/functional/feature_governance.py (1)
21-21: Good refactoring to use framework method for v20 activationThe change from a hardcoded command-line argument to the
delay_v20framework method improves consistency with other test files and makes the test more maintainable. This follows good software engineering practices by centralizing the activation height configuration logic.src/test/util/setup_common.cpp (1)
402-402: Updated checkpoint hash to match new activation parametersThis checkpoint hash update is necessary because changing the activation heights of v20 and mn_rr would result in different block hash values for the test chain. The update ensures that tests relying on this checkpoint will continue to pass with the new activation parameters.
test/functional/feature_llmq_simplepose.py (1)
25-25: Appropriately configured v20 activation for LLMQ testAdding
delay_v20(height=9999)ensures this test runs without v20 features activated. This is consistent with the test's requirements and the PR's strategy for managing test activation heights. Setting a very high activation height effectively prevents v20 features from activating during the test execution.test/functional/feature_llmq_rotation.py (1)
56-56: Make test activation height consistent with deployment parametersThis change ensures that the v20 fork activates at height 900 during the test, which aligns with the projected activation height checked later in the test (line 111).
test/functional/feature_dip4_coinbasemerkleroots.py (2)
22-22: Good addition of a constant for v20 activation heightCreating a named constant improves the maintainability of the code and clearly communicates the desired height for v20 activation.
50-50: Ensure consistent v20 activation heightThis change uses the constant to delay v20 activation, ensuring consistent behavior with other tests and the consensus parameters.
test/functional/rpc_blockchain.py (4)
188-189: Update softfork activation heights for testsThese changes align the test expectations with the new consensus parameters:
- v20 activation height set to 412 (matching DIP0003)
- mn_rr activation height lowered to 16
The comment correctly explains that v20 should not activate earlier than DIP0003.
215-216: Update expected softfork status to match new activation heightsThis update correctly reflects that:
- v20 remains inactive at the current test height
- mn_rr is now active due to its earlier activation height (16)
These changes ensure the test properly validates the output of the getblockchaininfo RPC.
188-189: LGTM: Activation heights reduced to match PR objectives.The activation heights are correctly adjusted to enable earlier availability of fork features in regtest mode. The v20 fork is set to activate just after DIP0003 at height 412, which aligns with the architectural dependency between these forks. The mn_rr fork activation is substantially lowered to height 16, supporting faster startup for development environments.
215-216: LGTM: Expected softfork values updated to match new activation heights.The assertions are properly updated to reflect the new activation heights, with v20 expected to be inactive at height 412 and mn_rr expected to be active at height 16 (since the test runs at height 200). This maintains test consistency with the configured activation parameters.
src/masternode/payments.cpp (1)
44-45:⚠️ Potential issueCritical safety improvement: gate mn_rr behind v20 activation
This change prevents potential loss of funds by ensuring that reward reallocation only happens when the credit pool exists (after v20 activation).
The comment clearly explains the reasoning: since the credit pool doesn't exist before v20 activation, any funds sent to it would be permanently lost.
There's a Clang format issue reported by the pipeline. Run clang-format on these lines to fix the formatting:
- // Credit Pool doesn't exist before V20. If any part of reward will re-allocated to credit pool before v20 activation these fund will be just permanently lost. Applyable for devnets, regtest, testnet - if (fV20Active && DeploymentActiveAfter(pindexPrev, m_consensus_params, Consensus::DEPLOYMENT_MN_RR)) { + // Credit Pool doesn't exist before V20. If any part of reward will re-allocated to credit pool before v20 activation these fund will be just permanently lost. Applyable for devnets, regtest, testnet + if (fV20Active && DeploymentActiveAfter(pindexPrev, m_consensus_params, Consensus::DEPLOYMENT_MN_RR)) {Likely an incorrect or invalid review comment.
test/functional/feature_llmq_chainlocks.py (2)
23-23: Activation height configuration updated to use delay_v20The test now uses the new
delay_v20method from the test framework instead of directly setting activation heights, which is consistent with the PR's goal of activating features at the earliest possible block height on regtest chains.
34-35: Changed from mn_rr to v20 activationReplaced the activation of MN_RR at height 900 with v20 at height 200, which aligns with the changes in
src/chainparams.cppwhere v20 activation is now tied to DIP0003 height and MN_RR activates at block 1.src/chainparams.cpp (2)
798-798: V20 activation now tied to DIP0003 heightThis change ensures v20 activation happens no earlier than DIP0003, maintaining the required activation sequence while allowing for earlier activation on regtest. This is crucial for functional tests since v20 introduces features like ChainLocks and Credit Pool that depend on DIP0003 functionality.
799-799: MN_RR now activates at block 1 on regtestSetting MN_RR to activate at block 1 enables immediate activation of masternode reward reallocation on regtest, speeding up test chains as intended in the PR objectives. Since MN_RR depends on v20 for credit pool functionality, the actual reward reallocation will only take effect after v20 activates.
test/functional/rpc_masternode.py (3)
35-46: Updated masternode payment verification for v20 formatThe payment verification logic now properly handles the first entry with
6ascript (OP_RETURN) in v20 blocks, which is added to the coinbase transaction after v20 activation. This ensures proper counting and verification of actual payee addresses.
50-53: Adjusted condition to use num_payees instead of array lengthUsing the new
num_payeesvariable properly accounts for the added OP_RETURN entry in v20 blocks when determining if the transaction has an operator reward. This ensures accurate testing of both reward scenarios.
83-93: Updated protx info verification for v20 payment structureThe verification logic now correctly handles the new payment structure:
- When only 2 payees exist (including OP_RETURN), it verifies against the masternode payout address
- When 3 payees exist, it checks for two possible arrangements of payout and operator payout addresses
This change accommodates the new v20/MN_RR payment structure where Credit Pool entries change the payee structure.
test/functional/test_framework/test_framework.py (4)
1195-1195: Added v20_height parameter to facilitate unified feature activation control.This adds a default height of 100 for v20 activation in the DashTestFramework, supporting the PR's goal of lowering activation heights and standardizing control across tests.
1213-1215: Good implementation of v20 activation height control method.This method provides a clean way for tests to customize when v20 activates, improving test flexibility without needing to modify command line parameters directly.
1175-1175: Properly integrated v20 activation height parameter with node configuration.The implementation ensures all nodes share the same v20 activation height by appending it to their configuration alongside DIP3 parameters.
637-637: Added v20 activation height to dynamically created node configuration.This modification correctly propagates the v20 activation height to dynamically created nodes, ensuring consistent test behavior across all nodes regardless of when they're created.
test/functional/feature_asset_locks.py (6)
57-57: Lowered mn_rr activation height to 620.This change aligns with the PR's goal of activating soft-forks at earlier block heights to improve test efficiency.
251-251: Replaced manual v20 activation with an assertion.Good change that leverages the new v20 activation height configuration approach, ensuring v20 is active by the time this test runs rather than manually activating it.
517-519: Added fallback for refreshing the coin list.This improves test robustness by checking if we've exhausted available coins and getting a new list if needed, preventing test failures due to insufficient coins.
521-523: Refined locking condition logic.This improves the test by limiting individual lock amounts to 99 COIN once the total exceeds 10,000 COIN, preventing unnecessarily large locks while still ensuring we reach the desired total.
625-625: Updated mn_rr activation parameter to match configuration.This ensures consistency between the test configuration parameter at line 57 and the actual activation call, maintaining test coherence.
637-637: Updated expected platform reward value.The reward value has been updated to 104,549,943 to reflect the recalculated rewards with earlier activation heights. This ensures test validity with the new consensus parameters.
…it pool It is appliable for test networks which could have custom height of forks
src/chainparams.cpp
Outdated
| @@ -796,7 +796,7 @@ class CRegTestParams : public CChainParams { | |||
| consensus.DIP0024QuorumsHeight = 1; // Always have dip0024 quorums unless overridden | |||
| consensus.V19Height = 1; // Always active unless overriden | |||
| consensus.V20Height = consensus.DIP0003Height; // Active not earlier than dip0003. Functional tests (DashTestFramework) uses height 100 (same as coinbase maturity) | |||
| consensus.MN_RRHeight = 900; | |||
| consensus.MN_RRHeight = 1; | |||
There was a problem hiding this comment.
mn_rr should not activate prior to v20 so maybe = consensus.V20Height;?
There was a problem hiding this comment.
I'd prefer mn_rr be activated early as possible, even if regtest adjusted v20's height.
What do you think about = consensus.V20Height; AND:
diff --git a/src/chainparams.cpp b/src/chainparams.cpp
index 8938264e2c..980cad78a1 100644
--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -1031,8 +1031,9 @@ static void MaybeUpdateHeights(const ArgsManager& args, Consensus::Params& conse
consensus.V19Height = int{height};
} else if (name == "v20") {
consensus.V20Height = int{height};
+ consensus.MN_RRHeight = std::max(consensus.MN_RRHeight, int{height});
} else if (name == "mn_rr") {
- consensus.MN_RRHeight = int{height};
+ consensus.MN_RRHeight = std::max(consensus.V20Height, int{height});
} else {
throw std::runtime_error(strprintf("Invalid name (%s) for -testactivationheight=name@height.", arg));
}
There was a problem hiding this comment.
I tried 78bb23a and it doesn't work (rpc_masternodes.py fails because mn_rr doesn't follow height).
mn_rr should be adjusted on regtest everytime when v20 activation is adjusted. It adds unneded complexity
There was a problem hiding this comment.
Pull Request Overview
This PR finalizes the activation of the v20 and MN_RR forks on regtest by adjusting activation heights and updating test expectations. Key changes include:
- Updating activation height parameters for v20 and MN_RR across various tests and configuration files.
- Modifying reward reallocation logic and test assertions to reflect updated subsidy and credit pool calculations.
- Adjusting expected outcomes in functional tests (e.g. coinbase rewards and block pruning) to match the new fork behavior.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/functional/test_framework/test_framework.py | Add delayed v20 activation to dynamically initialized datadir config |
| test/functional/rpc_masternode.py | Update masternode payee counting and assertion logic |
| test/functional/rpc_blockchain.py | Adjust activation heights for v20 and MN_RR in blockchain info tests |
| test/functional/feature_mnehf.py | Remove explicit v20 activation to align with updated fork behavior |
| test/functional/feature_llmq_simplepose.py | Set v20 activation delay via delay_v20 |
| test/functional/feature_llmq_rotation.py | Delay v20 activation to height 900 |
| test/functional/feature_llmq_chainlocks.py | Activate v20 (instead of MN_RR) prior to chainlocks tests |
| test/functional/feature_index_prune.py | Update expected prune heights |
| test/functional/feature_governance.py | Replace static v20 activation parameter with delay_v20 mechanism |
| test/functional/feature_dip4_coinbasemerkleroots.py | Add delay_v20 call to adjust v20 activation height |
| test/functional/feature_asset_locks.py | Update expected platform rewards and mn_rr activation height adjustments |
| src/test/util/setup_common.cpp | Update test vector hash for v19 before activation setup |
| src/masternode/payments.cpp | Require v20 activation before applying credit pool reward reallocation |
| src/evo/creditpool.cpp | Clarify that no credit pool exists before v20 |
| src/chainparams.cpp | Update fork activation heights, setting v20 at DIP0003 and MN_RR to block 1 |
Comments suppressed due to low confidence (3)
test/functional/feature_asset_locks.py:637
- Verify that the updated expected platform reward value (104549943) correctly reflects the new reward reallocation logic after v20 activation.
assert_equal(platform_reward, 104549943)
src/masternode/payments.cpp:46
- Ensure that gating the platform reward reallocation with fV20Active correctly prevents fund loss before v20 is active. A brief comment to clarify this logic might help future maintainers.
if (fV20Active && DeploymentActiveAfter(pindexPrev, m_consensus_params, Consensus::DEPLOYMENT_MN_RR)) {
src/chainparams.cpp:799
- [nitpick] Double-check that setting MN_RRHeight to 1 is intentional and consistent with legacy test expectations, as it may affect the activation order and behavior of forks.
consensus.MN_RRHeight = 1;
test/functional/rpc_masternode.py
Outdated
| payments_payee += payments_block_payees[i]["address"] | ||
| if i < len(payments_block_payees) - 1: | ||
| payments_payee += ", " | ||
| num_payees += 1 |
There was a problem hiding this comment.
[nitpick] Using a separate counter (num_payees) for payee assertions may be slightly ambiguous. Consider adding a clarifying comment or refactoring the loop to improve readability.
| num_payees += 1 | |
| num_payees = len(payments_block_payees) - 1 # Exclude the first element |
Copilot uses AI. Check for mistakes.
Also group Dash-specific args at the end of the list in affected tests while at it.
Issue being fixed or feature implemented
This PR is the final PR that activates all currently buried soft-forks on earlier block as possible.
This series of PR chased 3 ultimate goals:
Prior work: #6511 and other PRs
What was done?
Changes are related to 2 forks that has been activated on high height on regtest:
Functional test
rpc_masternode.pyis compatible now with MN_RR fork (expected 3 or 4 outputs of coinbase tx: miner, credit pool, masternode reward, optional operator reward)V20
V20 must not be active before fork dip0003 so far as all features depends on DIP0003.
For BitcoinTestFramework v20's activation is delayed to block 500 same as DIP0003 because unit tests and functional tests that are backported from bitcoin expects certains behaviour: manually constructed blocks do not have CbTx transaction of version 2 (dip003) or version 3 (v20 with CL) and may not be accepted after DIP3 or v20 activation.
For DashTestFramework v20's activation is delayed to block 100 because wallet should have enough coins to create masternodes / evonodes; but with fixed subsidy equaled to 2 it takes thousands of blocks before masternodes can be created. 100 blocks is chosen to match with coinbase maturity. Higher height may cause v20 to be activated too late for some functional tests. Height can be lower than 100 blocks, maybe low as just 50 blocks, but 50 * 500 = 25000 tDash which is not enough to create more than 8 evo nodes.
MN_RR
Part of block reward is not coming to credit pool before it is actually exist (v20 activated).
How Has This Been Tested?
Run unit / functional tests that possible affected by this PR.
Time is slightly reduced for
feature_asset_locks.py,feature_mnehf.py,feature_llmq_chainlocks.pyas expected.Time is unexpectedly increased for
feature_llmq_rotation.py(should not be changed).Disk usage is significantly reduced as expected (less blocks means less storage for blocks; less logs for each node).
PR:
Disk usage:
develop:
Disk usage:
Breaking Changes
On Regtest fork
v20is activated on block 1 if not specified otherwise.Use
-testactivationheight=v20@100to activate v20 from block 100 and increase subsidy of first blocks, if you want to register any masternode.On Regtest fork
MN_RRis activated on block 1 if not speicifed otherwise.Use
-testactivationheight=mn_rr@{HEIGHT}to activate mn_rr from block{HEIGHT}to trigger platform activation.{HEIGHT}may be less than v20, but actual reward reallocation will happens only since v20 (credit pool in CbTx) is activated.No changes for devnets, testnet3, mainnet.\
Checklist: