feat: start v19 from block 1 - fire up test chains by first block - 8/n #6511
Conversation
WalkthroughThe pull request introduces comprehensive modifications across multiple files in the Dash cryptocurrency project, focusing primarily on version 19 (V19) activation and related protocol changes. The changes span core consensus parameters, masternode management, RPC interfaces, and extensive test framework updates. Key modifications include lowering the V19 activation height to 1 in regtest environments, removing version-specific checks for EvoNode registrations, and updating test frameworks to accommodate these changes. The modifications simplify activation logic, enhance flexibility in masternode handling, and adjust test scenarios to reflect the new protocol state. The changes appear to be part of a broader protocol upgrade strategy, streamlining version deployment and reducing conditional logic around feature activations. The updates touch multiple components of the Dash ecosystem, indicating a coordinated effort to modernize and optimize the protocol's implementation. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 6
🧹 Nitpick comments (24)
src/bls/bls_ies.cpp (1)
Line range hint
12-17: Switch from templateOutto fixedstd::vector<unsigned char>&output parameter
This design simplifies the interface but removes flexibility for other container types.src/bls/bls.h (1)
277-277: Spellcheck in comment
"intollerant" should be spelled "intolerant" or rephrased for clarity.- //! GetPublicKey() is intollerant to BLS scheme + //! GetPublicKey() is intolerant to BLS schemetest/functional/feature_dip3_deterministicmns.py (5)
214-214: Conditionally choosing registrar command
The inline conditional is concise. Consider encapsulating this repeated logic in a helper function to avoid duplication.
229-229: Conditionally picking BLS generation scheme
Same pattern: multiple inline checks forsoftfork_active. A single wrapper function for BLS generation might improve readability.
257-257: Conditional usage ofregister_fundvs.register_fund_legacy
Consolidating these into a dedicated function could reduce code repetition.
273-273: Conditional usage ofregistervs.register_legacy
Same improvement idea: a helper function to abstract this conditional.
291-291: Conditional usage ofupdate_registrarvs.update_registrar_legacy
Following the same pattern, a shared utility function for these changes may simplify maintenance.src/llmq/dkgsession.cpp (1)
531-531: Duplicate pattern inqc.sig = m_mn_activeman->Sign(...)
This is identical to line 219. If the logic is repeated in multiple places, consider extracting a helper function for signing.src/evo/deterministicmns.cpp (2)
183-183: Inline comment referencing v19 activation
Ensure these comments remain updated if future versions or flags replace DEPLOYMENT_V19.
1244-1248: Migration blocked if v19 is active
This early return is fine for preventing logic conflicts, but confirm that failing migrations won't introduce unexpected states for partially migrated data.src/rpc/evo.cpp (4)
402-402: Register-fund wrapper dispatch
Conditionally usingself.m_name == "protx register_fund_legacy"is a neat trick. Just ensure new commands or changes in naming conventions keep it correct.
497-497: Register-prepare wrapper
Reiterating the pattern suggests a possible single function for all variants.
654-654:use_legacyassignment
Check for consistent usage throughout the code. Potentially rename tom_use_legacy_signingfor clarity.
975-975:ParseBLSSecretKeyusage
Be mindful that any invalid or empty keys produce immediate errors. Possibly wrap in a safer parse with better error messages if needed.test/functional/test_framework/test_framework.py (3)
1303-1304: Check for v19
This logic is re-used in multiple places. Consider centralizing a functionis_softfork_active(name)to reduce duplication.
1422-1422:generate(1, sync_fun=self.no_op)
This might be a leftover debug approach. If no longer needed, remove to keep code succinct.
1923-1928: Block threshold logic
"if cur_block < 200" is a hard-coded threshold. Provide context or define as a constant for maintainability.src/test/bls_tests.cpp (4)
66-66: Clarify the comment
The note about the second bool argument is helpful, but consider expanding or rephrasing for clarity.
70-71: Test coverage for hex parsing
Excellent handling of invalid hex. Ensure boundary cases (very short or extremely long) are consistently tested.
75-76: Repeated logic in hex string tests
The lines are repeating similar checks. Consider factoring them out into a helper function for DRY principle.
147-147: Large-scale aggregation
Signing with large aggregated keys can be performance-sensitive. Confirm performance remains acceptable.src/test/rpc_tests.cpp (1)
566-566: Ensure all relevant edge cases are tested.
Here, the test checks the behavior when explicitly specifying" 0". Consider including additional checks for boundary inputs (e.g., erroneous scheme values) to ensure robust coverage.src/rpc/governance.cpp (1)
316-319: Multi-line logging for clarity.
Splitting the log print across multiple lines enhances readability. Ensure that log formatting tokens (such as%s,%d, etc.) match argument types to prevent runtime logging errors.src/chainparams.cpp (1)
1034-1035: Confirm argument name consistency when setting V19 height.The newly introduced
"v19"argument setsconsensus.V19Height. Verify that all references to the command-line parameter match this naming so that it won't be mistakenly referred to as"v18"or any previous naming.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
src/bench/bls.cpp(3 hunks)src/bls/bls.cpp(0 hunks)src/bls/bls.h(3 hunks)src/bls/bls_ies.cpp(4 hunks)src/bls/bls_ies.h(1 hunks)src/bls/bls_worker.cpp(1 hunks)src/chainparams.cpp(2 hunks)src/chainparamsbase.cpp(1 hunks)src/coinjoin/coinjoin.cpp(2 hunks)src/evo/assetlocktx.h(1 hunks)src/evo/deterministicmns.cpp(3 hunks)src/evo/mnauth.cpp(3 hunks)src/evo/providertx.h(4 hunks)src/evo/specialtxman.cpp(2 hunks)src/llmq/dkgsession.cpp(5 hunks)src/llmq/dkgsession.h(1 hunks)src/llmq/dkgsessionmgr.cpp(1 hunks)src/llmq/dkgsessionmgr.h(1 hunks)src/llmq/signing_shares.cpp(1 hunks)src/masternode/node.cpp(0 hunks)src/masternode/node.h(0 hunks)src/rpc/evo.cpp(13 hunks)src/rpc/governance.cpp(1 hunks)src/test/bls_tests.cpp(10 hunks)src/test/evo_deterministicmns_tests.cpp(3 hunks)src/test/evo_mnhf_tests.cpp(1 hunks)src/test/rpc_tests.cpp(3 hunks)src/test/util/setup_common.cpp(1 hunks)src/validation.cpp(1 hunks)test/functional/feature_dip3_deterministicmns.py(6 hunks)test/functional/feature_dip3_v19.py(2 hunks)test/functional/feature_dip4_coinbasemerkleroots.py(2 hunks)test/functional/feature_governance_cl.py(0 hunks)test/functional/feature_llmq_connections.py(0 hunks)test/functional/feature_llmq_evo.py(2 hunks)test/functional/feature_llmq_is_cl_conflicts.py(0 hunks)test/functional/feature_llmq_is_retroactive.py(0 hunks)test/functional/feature_llmq_rotation.py(1 hunks)test/functional/feature_notifications.py(0 hunks)test/functional/interface_zmq_dash.py(0 hunks)test/functional/p2p_instantsend.py(0 hunks)test/functional/p2p_quorum_data.py(0 hunks)test/functional/rpc_blockchain.py(1 hunks)test/functional/rpc_mnauth.py(1 hunks)test/functional/rpc_verifyislock.py(0 hunks)test/functional/test_framework/test_framework.py(5 hunks)
💤 Files with no reviewable changes (12)
- test/functional/rpc_verifyislock.py
- test/functional/feature_notifications.py
- test/functional/interface_zmq_dash.py
- test/functional/p2p_instantsend.py
- test/functional/feature_llmq_connections.py
- test/functional/p2p_quorum_data.py
- test/functional/feature_llmq_is_retroactive.py
- src/bls/bls.cpp
- test/functional/feature_governance_cl.py
- test/functional/feature_llmq_is_cl_conflicts.py
- src/masternode/node.h
- src/masternode/node.cpp
✅ Files skipped from review due to trivial changes (1)
- src/evo/assetlocktx.h
🔇 Additional comments (73)
src/bls/bls_ies.cpp (3)
47-48: Verify minimum length of pk.ToByteVector(false) before resizing
Ensure that the returned vector always has at least 32 bytes before calling resize(32), to avoid any potential out-of-range issues.
83-84: Consistent check for vector length
Same concern as above: validate the vector size from ToByteVector(false) before resizing to 32 bytes.
100-101: Confirm safe resizing to 32 bytes
Double-check that pk.ToByteVector(false) returns data long enough to be truncated or padded to 32 bytes.
src/bls/bls_ies.h (1)
11-14: Documentation clarity
The doc comment clarifies basic scheme usage. Consider adding examples or references on encryption steps for better developer guidance.
src/evo/specialtxman.cpp (2)
Line range hint 209-213: Toggling bls_legacy_scheme
Storing to a global atomic is straightforward here, but verify no concurrency issues arise if multiple threads manipulate bls_legacy_scheme in parallel.
Line range hint 230-234: Reverting bls_legacy_scheme
When reorgs or block invalidations occur, switching back to old rules is correct. Just ensure the system can fully re-initialize any dependent states after toggling.
src/bls/bls.h (2)
261-265: Constructor sets modOrder to false
Ensure that using false for SetByteVector represents the intended scheme and no confusion arises about which parameter controls legacy vs. modOrder. Consider adding inline comments clarifying this distinction.
332-335: Signature constructor with explicit legacy parameter
This is a clean interface. Just ensure that the caller validates bytes.size() matches the expected signature length before calling.
test/functional/feature_dip3_deterministicmns.py (1)
15-15: Importing softfork_active
Good addition. Ensure that the helper is always available in the test framework environment.
src/llmq/dkgsession.cpp (4)
87-88: Use of m_use_legacy_bls: clarity check
The member variable initialization logic is correct, but please ensure that any additional code paths that depend on m_use_legacy_bls are well-documented to avoid confusion.
219-219: Signature call with legacy parameter
Using the Sign method with m_use_legacy_bls is consistent. Confirm all call sites handle potential differences in signature format.
725-725: Signature call in SendJustification
Same approach: verifying whether a justification might need explicit error handling if Sign fails would be beneficial.
1015-1017: Legacy vs. basic BLS signatures
Line 1016 calls Sign(commitmentHash, use_legacy_bls); and line 1017 calls skShare.Sign(commitmentHash, bls::bls_legacy_scheme.load()); which might diverge. Ensure it's intentional to use use_legacy_bls in one line and bls_legacy_scheme.load() in the next.
src/rpc/evo.cpp (4)
200-205: ParseBLSSecretKey with hardcoded false
Line 205 passes false to SetHexStr, ignoring the second parameter’s meaning. Confirm if no variant uses the modOrder parameter.
310-312: Generic function for signing with legacy
This addition clarifies a single SignSpecialTxPayloadByHash. Good approach. Validate that all specialized payload types are tested.
Also applies to: 317-317
449-449: Register wrapper dispatch
Same pattern. Keep the string matching updated if the CLI syntax changes.
1050-1050: Inverting !isV19active
Double-check that the sign call is correct if activation status changes mid-test.
test/functional/test_framework/test_framework.py (3)
1337-1337: Line missing
No direct code is visible here. Possibly a placeholder or leftover. Please confirm it’s needed or remove.
1388-1389: Double-check the v19_active usage
Same approach. Make sure softfork_active call is cheap or that caching is used if called frequently.
1419-1419: register_fund fallback
Check that the correct function is always chosen for non-legacy.
test/functional/rpc_mnauth.py (1)
20-20: Modified test parameters
Switched from (1, 0) to (2, 1). Good for expanded coverage. Check that the extra node is effectively used in test steps.
src/test/evo_mnhf_tests.cpp (1)
63-66: Consider extending tests for legacy signing.
The newly introduced use_legacy parameter is always set to false. If legacy signing is still supported, it may be prudent to include tests explicitly covering true to ensure full coverage.
src/llmq/dkgsessionmgr.h (1)
16-20: Forward declarations look fine.
These newly added template classes introduce a flexible and reusable design for handling encrypted contributions. Ensure they are thoroughly tested wherever they are instantiated.
src/chainparamsbase.cpp (1)
23-23: Addition of 'v19' activation height is consistent.
The updated help text now includes “v19”, matching the new logic across the PR without apparent conflicts.
test/functional/feature_dip3_v19.py (4)
47-47: Enabling v19 at height 200 aligns with the new activation scheme.
This updated parameter ensures that tests trigger v19-specific logic at block 200, simplifying early activation for Regtest.
51-52: Method for explicit activation of v19 is clear and maintainable.
Encapsulating v19 activation in a dedicated method tidies up test logic and supports straightforward adjustments in the future.
72-72: Mining an additional test quorum before v19 activation.
Creating a quorum ensures relevant masternode data is in place, protecting against edge cases.
74-74: Explicit activation at block 200 aligns with the test parameter.
This call to activate_v19 after mining ensures the test environment transitions seamlessly into v19-specific scenarios.
src/evo/mnauth.cpp (3)
54-55: Switching to basic BLS for all clients
This change ensures consistency with the updated BLS signing scheme by explicitly passing false. It clarifies the signing method and aligns with the broader shift toward explicit scheme usage.
90-91: Extended debugging output
Appending ToString(false) is a sensible choice for matching the newly enforced basic BLS scheme, helping to differentiate legacy from basic scheme signatures in logs.
117-117: Explicitly verifying with basic BLS
Ensuring that .VerifyInsecure(..., false) is called with false aligns the verification flow with the basic BLS scheme introduced above, preserving consistency and preventing potential mix-ups between legacy and basic modes.
src/evo/providertx.h (4)
115-115: Renaming parameter to is_basic_scheme_active
This rename makes the parameter’s meaning clearer and aligns with the updated usage of the basic BLS scheme, improving readability.
195-195: Consistent naming for is_basic_scheme_active
Carrying over the parameter rename ensures uniform function signatures across different transaction types.
260-260: Parameter rename in CProUpRegTx
The new naming removes ambiguity. Validate that all callers correctly pass the new parameter, especially in older or transitional code.
324-324: CProUpRevTx alignment with updated naming
Ensuring each transaction class uses is_basic_scheme_active keeps the codebase coherent, especially with BLS scheme expansions.
src/bench/bls.cpp (3)
32-37: Explicitly specifying false in Sign(...)
Introducing the second parameter clarifies the intention to use the basic scheme. No functional issues noted.
74-75: Aggregation consistency
Passing false in both signatures (sig1 & sig2) indicates uniform usage of the basic scheme, reducing confusion in aggregated signatures.
92-92: Parameterization in benchmarking
Including false in secKey.Sign(...) ensures the benchmarks accurately capture the basic BLS signing performance.
test/functional/feature_llmq_evo.py (2)
19-19: No functional concerns
Addition or shift in imports is minor, and the usage remains consistent. No further action required.
48-48: Lowered activation height to 400
Reducing the test activation height speeds up scenario testing and aligns with the PR’s goal of faster test cycles. Verify that no unintended side effects arise from earlier activation.
src/test/bls_tests.cpp (11)
27-28: Confirm signing usage and test coverage
Signing calls now explicitly pass legacy_scheme. Ensure test coverage includes both true and false.
47-47: Check consistency in signing approach
Verify that this call to Sign(msgHash, legacy_scheme) is consistent with other test vectors.
78-80: Validate error messages
Ensure the user gets informative errors when hex string fails to parse due to length constraints.
105-105: Aggregate key usage
Double-check if the aggregated key remains valid under both legacy and new BLS schemes in edge cases.
173-173: Consistent signing in loops
Loop-based signing is correct. Verify that all partial signatures aggregate consistently with legacy_scheme.
224-224: Secure aggregation checks
Ensure that “secure” path is tested with both true/false for legacy_scheme for thorough coverage.
267-267: Runtime retrieval of legacy scheme
The approach is good. Verify that bls::bls_legacy_scheme.load() is always up-to-date and thread-safe.
274-274: Check message integrity
Ensure message validity is properly flagged. Good usage of legacy_scheme in test coverage.
280-280: Test failure path
Overriding the signature with a separate key is an effective negative test. Nicely handled.
385-385: Threshold signature test
Explicitly passing legacy_scheme is consistent with the rest of the patch. Looks good.
402-402: Recovery verification
Signing with subset keys for recovery is a nice test. Ensure thorough coverage for legacy_scheme.
test/functional/feature_dip4_coinbasemerkleroots.py (5)
118-118: Quorum list updates
Reduced the expected new quorums from 3 to 2. Confirm if this aligns with the new logic for quorum activation.
126-126: Expected new quorum
Similar reduction to two quorums. Make sure no breakages occur in other test cases expecting 3 quorums.
133-134: Quorum transitions
Deleted + added set suggests an old quorum is replaced. Confirm correct lifecycle of ephemeral quorums.
139-139: Expanded new quorums
Including second and third quorums in expected new set. Ensure that the final set matches chain state.
156-157: Validation of block transitions
Again, old quorum replaced by third. Double-check block range logic to ensure consistent state transitions.
src/llmq/dkgsessionmgr.cpp (1)
11-11: Confirm usage of new include
#include <bls/bls_ies.h> is introduced. Verify that all usage remains necessary to avoid unused includes.
src/coinjoin/coinjoin.cpp (2)
65-67: Ensure correct usage of the legacy parameter in BLS signature verification.
Looks consistent with the new scheme parameter (false), but please confirm that VerifyInsecure behavior meets all security requirements in this context.
104-106: Check consistency of false parameter across BLS usage.
This matches the other signature checks in this file. Just be sure all sign-and-verify calls consistently use the correct scheme parameter.
test/functional/feature_llmq_rotation.py (2)
102-102: Double-check the expected number of nonzero DKGs.
The assertion now checks for “4” instead of a larger value, reflecting the removal of additional quorums. This looks correct as long as only two new quorums are formed each cycle.
105-105: Validate the updated expected quorums list.
expectedNew includes only [h_100_0, h_100_1] instead of old references to 106. This change is consistent with the removal of h_106_0 and h_106_1.
test/functional/rpc_blockchain.py (1)
175-175: Confirm v19 softfork logic.
v19 is now active at height 1, which alters the testing assumptions about when it triggers. Ensure that the rest of the test suite is aligned with this earlier activation.
src/test/util/setup_common.cpp (1)
395-395: Updated checkpoint hash verification.
The checkpoint at block 497 has a new hash. Verify that no references to the old hash remain and ensure test dependencies are still valid.
src/test/rpc_tests.cpp (4)
546-546: Straightforward test assertion.
This test line simply asserts that the scheme is "basic". It appears consistent with the surrounding logic and the earlier call to bls generate.
558-560: Good coverage for basic scheme checks.
These consecutive lines verify that the generated secret key yields the expected "basic" scheme and correct public key. This maintains consistency across BLS usage.
579-579: Consistent usage of legacy scheme parameter.
This test verifies that secret keys work correctly with legacy scheme " 1". The explicit usage is consistent with broader changes to the BLS code.
582-582: Validates basic scheme fallback.
Line 582 ensures that omitting the legacy parameter results in the "basic" scheme. This distinction is good for verifying default behavior.
src/bls/bls_worker.cpp (1)
765-765: Explicitly loading legacy scheme.
Using secKey.Sign(msgHash, bls::bls_legacy_scheme.load()) clarifies which scheme is in use. Double-check for consistent usage in all relevant signing contexts.
src/test/evo_deterministicmns_tests.cpp (3)
135-135: Explicit legacy signature invocation.
The code now explicitly calls Sign(..., bls::bls_legacy_scheme) on line 135. This change is consistent with the deprecation of the single-argument Sign() function.
174-174: Ensuring consistent scheme for revocations.
Similar usage of operatorKey.Sign(..., bls::bls_legacy_scheme) in line 174 ensures revocations also utilize the correct scheme.
861-861: Custom activation height logic.
Initializing the test chain at height 894 with v19 activation at 900 helps ensure that the transition logic is exercised. This approach is fine for test coverage.
src/llmq/signing_shares.cpp (1)
1544-1544: Ensure consistency with the rest of the codebase.
This line now explicitly loads the legacy scheme parameter in both Sign(...) and Set(...). Please confirm that all other calls to Sign within the codebase also consistently pass the same scheme parameter to avoid any mismatch.
Would you like a script to search for all other calls to Sign that might still rely on a default or different scheme parameter?
src/chainparams.cpp (1)
802-802: Double-check early activation for EvoDb.
Setting consensus.V19Height = 1 means V19 logic applies almost immediately. The comment mentions a potential conflict with EvoDb activation from block 2. Ensure that no code paths rely on V19 logic before EvoDb is fully ready.
Would you like a script to find EvoDb references that might implicitly depend on V19 being active strictly from block 2 or later?
knst
force-pushed
the
forks-speedup-v19-p2
branch
from
d9fb7ac to
6ccf151
Compare
f76f943 docs: comments about invariant for CBLSSecretKey for bls scheme (Konstantin Akimov) 5c36bb2 docs: add TODO about bls global variable (Konstantin Akimov) 6fa033e feat: p2p message mnauth to always use Basic BLS scheme (Konstantin Akimov) ce18dcd refactor: enforce passing bls scheme for each call of sign by BLS (Konstantin Akimov) 954e5ef fix: undefined behaviour in evo RPC calls of RPCHelpMan (Konstantin Akimov) 5ea9ff9 feat: enforce using _legacy suffix for protx commands; no more guessing based if v19 is activated (Konstantin Akimov) 99e9c65 feat: use Basic scheme only for public key serialization in bls_ies (Konstantin Akimov) 91f10b1 refactor: removed unused functions from bls_ies (Konstantin Akimov) 26c058a refactor: optimize usages bls_ies.h and bls.h (Konstantin Akimov) 7cbb26b style: clang suggestion for bls rpcs (Konstantin Akimov) 394d649 feat: use basic scheme for RPC `bls generate` by default (Konstantin Akimov) caedaba refactor: remove CheckMalleable without bls scheme argument (Konstantin Akimov) f728fc9 refactor: drop legacy flag from CActiveMasternodeInfo (Konstantin Akimov) 4acdd79 refactor: remove general constructor of BLS objects from bytes, add only to objects that uses it (Konstantin Akimov) f1a7e95 fix: condition of bls activation to make work even from block 1 in specialtxman (Konstantin Akimov) b5cd09e fix: rename argument name for IsTriviallyValid for protx (Konstantin Akimov) 544031d fix: do not pass param 'modOrder' for bls::PrivateKey (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Many usages of CBLS{Signature,PrivateKey,PublicKey} assume using global variable, even if it can be specified explicitly in some situations. These over-usages of `bls::bls_legacy_scheme` are blocker for changing buried height of activation of fork `V19` on Regtest. This PR helps unblock changing v19 height on RegTest, see: #6511 Prior improvements for BLS: #5443 ## What was done? This PR does: - fixes Undefined Behavior in rpc `protx register_legacy`, `protx register_fund_legacy`, `protx resiter_prepare_legacy`, `protx update_registrar_legacy` - drop flag "is legacy" from secret key initialization and serialization (as it has no difference) - enforce specifying legacy flag to bls functions `Sign`, `ToByteVector` and in a helper `SignSpecialTxPayloadByHash` - removed unused methods from bls_ies.h and optimized headers usages - enforce p2p message `mnauth` to use Basic BLS scheme - enforce using flag `legacy` in RPC `bls generate` and `bls fromsecret` instead guessing which one user want based on v19 activation status. ## How Has This Been Tested? Run unit and functional tests; update testing framework `test_framework.py` to use `_legacy` RPCs which has been ignored before. Reindex testnet `src/qt/dash-qt -reindex -testnet -assumevalid=0` - SUCCEED Reindex mainnet `src/qt/dash-qt -reindex -assumevalid=0` - SUCCEED ## Breaking Changes On RegTest RPC: `bls generate`, `bls fromsecret` should pass flag `is_legacy` for pre-v19 blocks On RegTest RPC: `protx register_legacy`, `protx register_fund_legacy`, `protx register_prepare_legacy`, `protx update_registrar_legacy` should be used in pre-v19 blocks. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: reindexed with no issues, light ACK f76f943 PastaPastaPasta: utACK f76f943 Tree-SHA512: 69878d2e3037c27d504103a4f5f38ee8cc5ca56676cea0ff92a309762c90ef6399ddb1359193669353af13fee98f3902aa5f3926c9f9b3c7ad7aa3b66ca9f23b
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
forks-speedup-v19-p2
branch
from
7f7c974 to
e521202
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/rpc/evo.cpp (1)
1048-1048: Split long line according to clang-format rulesThe line exceeds the maximum length limit. Consider splitting it for better readability.
- const bool isV19active{DeploymentActiveAfter(WITH_LOCK(cs_main, return chainman.ActiveChain().Tip();), Params().GetConsensus(), Consensus::DEPLOYMENT_V19)}; + const bool isV19active{DeploymentActiveAfter( + WITH_LOCK(cs_main, return chainman.ActiveChain().Tip();), + Params().GetConsensus(), + Consensus::DEPLOYMENT_V19 + )};🧰 Tools
🪛 cppcheck (2.10-2)
[error] 1048-1048: There is an unknown macro here somewhere. Configuration is required. If ; is a macro then please configure it.
(unknownMacro)
test/functional/test_framework/test_framework.py (1)
1524-1526: LGTM! Consider enhancing the comment.The code correctly ensures that at least 8 blocks are generated after v20 activation, which is necessary for masternodes to be usable in quorums. The implementation is clean and uses appropriate test framework methods.
Consider expanding the comment to explain why exactly 8 blocks are needed:
- # it should be at least 8 blocks since v20 when MN can be used in quorums + # Generate at least 8 blocks (SIGN_HEIGHT_OFFSET) after v20 activation to make masternodes eligible for quorum signing sessions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/chainparams.cpp(2 hunks)src/chainparamsbase.cpp(1 hunks)src/evo/deterministicmns.cpp(3 hunks)src/rpc/evo.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(1 hunks)src/test/util/setup_common.cpp(1 hunks)test/functional/feature_dip3_v19.py(2 hunks)test/functional/feature_dip4_coinbasemerkleroots.py(2 hunks)test/functional/feature_governance.py(1 hunks)test/functional/feature_governance_cl.py(0 hunks)test/functional/feature_llmq_connections.py(0 hunks)test/functional/feature_llmq_evo.py(2 hunks)test/functional/feature_llmq_is_cl_conflicts.py(0 hunks)test/functional/feature_llmq_is_retroactive.py(0 hunks)test/functional/feature_llmq_rotation.py(1 hunks)test/functional/feature_notifications.py(0 hunks)test/functional/interface_zmq_dash.py(0 hunks)test/functional/p2p_instantsend.py(0 hunks)test/functional/p2p_quorum_data.py(0 hunks)test/functional/rpc_blockchain.py(1 hunks)test/functional/rpc_mnauth.py(1 hunks)test/functional/rpc_verifyislock.py(0 hunks)test/functional/test_framework/test_framework.py(1 hunks)
💤 Files with no reviewable changes (9)
- test/functional/feature_governance_cl.py
- test/functional/feature_llmq_connections.py
- test/functional/p2p_instantsend.py
- test/functional/rpc_verifyislock.py
- test/functional/feature_notifications.py
- test/functional/feature_llmq_is_cl_conflicts.py
- test/functional/feature_llmq_is_retroactive.py
- test/functional/p2p_quorum_data.py
- test/functional/interface_zmq_dash.py
🚧 Files skipped from review as they are similar to previous changes (9)
- src/test/evo_deterministicmns_tests.cpp
- test/functional/feature_llmq_rotation.py
- src/chainparamsbase.cpp
- test/functional/feature_dip3_v19.py
- test/functional/feature_llmq_evo.py
- test/functional/rpc_blockchain.py
- test/functional/rpc_mnauth.py
- src/test/util/setup_common.cpp
- test/functional/feature_dip4_coinbasemerkleroots.py
🧰 Additional context used
🪛 cppcheck (2.10-2)
src/rpc/evo.cpp
[error] 1048-1048: There is an unknown macro here somewhere. Configuration is required. If ; is a macro then please configure it.
(unknownMacro)
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/evo.cpp
[warning] 1045-1045: Code formatting issue: Long line needs to be split into multiple lines according to clang-format rules
🔇 Additional comments (3)
test/functional/feature_governance.py (1)
94-94: LGTM! Test optimizationThe reduction in block generation from 10 to 3 blocks helps optimize the test execution time while maintaining test coverage.
Also applies to: 99-100
src/chainparams.cpp (1)
802-802: LGTM! V19 activation configurationSetting V19Height to 1 in regtest mode and making it configurable via command line arguments aligns with the PR objective of starting v19 from block 1. This provides flexibility for testing different activation heights.
Also applies to: 1034-1035
src/evo/deterministicmns.cpp (1)
1253-1257: Refactor duplicate V19 activation checkThe V19 activation check is duplicated in both migration functions. Consider extracting this into a helper function to reduce code duplication.
+ bool IsMigrationPossible(const CBlockIndex* tip, const Consensus::Params& consensusParams) { + return !DeploymentActiveAfter(tip, consensusParams, Consensus::DEPLOYMENT_V19); + } bool CDeterministicMNManager::MigrateDBIfNeeded() { - if (DeploymentActiveAfter(m_chainstate.m_chain.Tip(), consensusParams, Consensus::DEPLOYMENT_V19)) { + if (!IsMigrationPossible(m_chainstate.m_chain.Tip(), Params().GetConsensus())) { LogPrintf("CDeterministicMNManager::%s -- migration is not possible\n", __func__); return false; } bool CDeterministicMNManager::MigrateDBIfNeeded2() { - if (DeploymentActiveAfter(m_chainstate.m_chain.Tip(), consensusParams, Consensus::DEPLOYMENT_V19)) { + if (!IsMigrationPossible(m_chainstate.m_chain.Tip(), Params().GetConsensus())) { LogPrintf("CDeterministicMNManager::%s -- migration is not possible\n", __func__); return false; }Also applies to: 1369-1373
It's activated long time ago and this check has no meaning anymore
knst
force-pushed
the
forks-speedup-v19-p2
branch
from
e521202 to
f91f1f5
Compare
|
CI succeed here: https://gitlab.com/dashpay/dash/-/pipelines/1614748442 Last force-push fixes clang-format |
knst
force-pushed
the
forks-speedup-v19-p2
branch
from
f91f1f5 to
88888e2
Compare
…- 9/9 ef5cde5 feat: enforce mn_rr activation after v20 on regtest (UdjinM6) 1ac1628 test: simplify calculation of num payees (Konstantin Akimov) 5c3b284 doc: edit commentary about Credit Pool existance (Konstantin Akimov) 96e9cef fix: adjust rpc_blockchin.py functional test (Konstantin Akimov) fd32e57 fix: update number of blocks before pruning for feature_index_prune.py (Konstantin Akimov) 2127f99 test: adjust activation heights on rpc_blockchain.py for v20 and mn_rr (Konstantin Akimov) 046d5ee feat: activate mn_rr fork from block 1 on RegTest (Konstantin Akimov) 080207e test: speed-up feature_asset_locks.py functional test by generating less blocks (Konstantin Akimov) 3d67e7d fix: require v20 fork activated for reallocation block reward to credit pool (Konstantin Akimov) 641f889 feat: start v20 from the same block as DIP0003 on regtest (Konstantin Akimov) Pull request description: ## 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: - improve performance of functional tests: no need to generate many blocks just waiting until some feature is activated - it helps for unit & functional tests to test actual code that works on mainnet currently with all features enabled. Not some pre-feature status - it helps platform's dev environment to start faster (some final integration is expected after this PR). Prior work: #6511 and other PRs ## What was done? Changes are related to 2 forks that has been activated on high height on regtest: - v20 which set subsidy of each block to fixed value 5, increases governance share of reward (10% to 20%), introduces EHF forks and add to coinbase transaction ChainLocks and Credit Pool, also changes behavior in build SML lists. - mn_rr: part of reward of each block is actually goes to credit pool, triggers platform chain genesis blocks Functional test `rpc_masternode.py` is 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.py` as 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: ``` feature_asset_locks.py | ✓ Passed | 110 s feature_dip4_coinbasemerkleroots.py | ✓ Passed | 97 s feature_governance.py --descriptors | ✓ Passed | 186 s feature_governance.py --legacy-wallet | ✓ Passed | 184 s feature_llmq_chainlocks.py | ✓ Passed | 105 s feature_llmq_rotation.py | ✓ Passed | 133 s feature_mnehf.py | ✓ Passed | 71 s rpc_blockchain.py --v1transport | ✓ Passed | 13 s rpc_blockchain.py --v2transport | ✓ Passed | 12 s rpc_masternode.py | ✓ Passed | 8 s ALL | ✓ Passed | 919 s (accumulated) Runtime: 186 s ``` Disk usage: ``` 1396020 /tmp/test_runner_∋_🏃_20250512_115202 ``` develop: ``` feature_asset_locks.py | ✓ Passed | 131 s feature_dip4_coinbasemerkleroots.py | ✓ Passed | 90 s feature_governance.py --descriptors | ✓ Passed | 181 s feature_governance.py --legacy-wallet | ✓ Passed | 186 s feature_llmq_chainlocks.py | ✓ Passed | 120 s feature_llmq_rotation.py | ✓ Passed | 92 s feature_mnehf.py | ✓ Passed | 92 s rpc_blockchain.py --v1transport | ✓ Passed | 12 s rpc_blockchain.py --v2transport | ✓ Passed | 12 s rpc_masternode.py | ✓ Passed | 8 s ALL | ✓ Passed | 924 s (accumulated) ``` Disk usage: ``` 1633584 /tmp/test_runner_∋_🏃_20250512_121826/ ``` ## Breaking Changes On Regtest fork `v20` is activated on block 1 if not specified otherwise. Use `-testactivationheight=v20@100` to activate v20 from block 100 and increase subsidy of first blocks, if you want to register any masternode. On Regtest fork `MN_RR` is 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: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK ef5cde5 PastaPastaPasta: utACK ef5cde5 Tree-SHA512: f11f61bb5685c67f916d3151a4d6b0a42492bc09b54d97a7150381351da50fafd8d624f5ebb1cab843c6f9fa48fcb6f21c3f8a922079524afdc481aeb45edee8
3 participants
Issue being fixed or feature implemented
This PR is 8th in the achieving ultimate goal to activate old forks from block 1.
It helps to run unit and functional tests faster; it helps for platform's dev-environment to start faster.
What was done?
Activate V19 (basic BLS, evo nodes) from block 1 and related fixes. Depends on #6508 which contains critical fixes and changes to make this PR work at all.
Prior work for other forks: #6325 and other PRs.
How Has This Been Tested?
Run unit & functional tests. See also multiple updates for functional tests and optimization.
Breaking Changes
N/A; affect only Regtest
Checklist: