feat: remove pre-'withdrawals' logic for enforcing 2 quorums only and related test changes #6661
Conversation
|
This pull request has conflicts, please rebase. |
knst
changed the title
knst
changed the title
knst
changed the title
knst
force-pushed
the
tests-withdrawals-improvements
branch
from
a85c976 to
21c9161
Compare
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
…ums to get a favour situation
knst
force-pushed
the
tests-withdrawals-improvements
branch
from
21c9161 to
56711d9
Compare
|
@coderabbitai review this PR please, I see no incremental reviews yet |
✅ Actions performedReview triggered.
|
|
Tip For best results, initiate chat on the files or code changes. @knst I can see that a review has already been triggered for this PR. As mentioned in the automated response, CodeRabbit operates as an incremental review system, which means:
This PR focuses on removing legacy pre-'withdrawals' logic that enforced 2-quorum restrictions, which is a good cleanup effort since the 'withdrawals' fork has been active for some time. The performance improvements in the test suite (reducing runtime from 166s to 139s median) are also beneficial. The review should appear shortly if it hasn't already. If you continue to experience issues with the review not appearing, you may want to check if automatic reviews are paused for this repository or if there are any configuration issues. |
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
🧹 Nitpick comments (4)
src/evo/assetlocktx.cpp (1)
125-127: Scan “active + 1” quorums unconditionally: good simplification; add a small guardThe unconditional rule matches the PR objective and simplifies the logic. As a minor defensive improvement, consider guarding against unexpected zero or negative values for signingActiveQuorumCount (even if current params never set it to 0) to avoid passing 0 to ScanQuorums.
Apply this diff to harden the computation:
- // We check all active quorums + 1 the latest inactive - const int quorums_to_scan = llmq_params_opt->signingActiveQuorumCount + 1; + // We check all active quorums + 1 (the latest inactive) + const int quorums_to_scan = + static_cast<int>(std::max<int>(1, llmq_params_opt->signingActiveQuorumCount)) + 1;test/functional/feature_asset_locks.py (3)
58-58: Avoid magic numbers: reuse mn_rr_heightYou introduce self.mn_rr_height = 560 but still hardcode 560 later. Prefer using the field consistently to prevent drift.
Apply this diff in test_mn_rr:
- self.activate_mn_rr(expected_activation_height=560) + self.activate_mn_rr(expected_activation_height=self.mn_rr_height)
375-383: New getassetunlockstatuses checks are valuable; consider asserting the tip height explicitlyThe added RPC-based status assertions look sound and provide better coverage for index state. Consider also asserting tip before/after to make the intent crystal clear (optional).
671-676: Loop to target the last active quorum is bounded—good; consider extracting helper (optional)The index < 511 bound prevents infinite loops. As a small readability win, consider a helper that tries N indices to find the last-active quorum to avoid duplicating this idiom.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/chainparams.cpp(1 hunks)src/evo/assetlocktx.cpp(1 hunks)test/functional/feature_asset_locks.py(10 hunks)
🧰 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/assetlocktx.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/assetlocktx.cpp
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/feature_asset_locks.py
🧠 Learnings (2)
📓 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.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
PR: dashpay/dash#6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
Applied to files:
src/evo/assetlocktx.cpp
🧬 Code Graph Analysis (1)
test/functional/feature_asset_locks.py (5)
test/functional/test_framework/messages.py (1)
CAssetUnlockTx(1225-1271)test/functional/test_framework/util.py (1)
assert_equal(69-74)test/functional/test_framework/test_framework.py (3)
get_node(1216-1221)activate_mn_rr(1581-1582)activate_by_name(1537-1576)test/functional/interface_zmq_dash.py (1)
send_tx(92-97)test/functional/feature_mnehf.py (1)
send_tx(105-120)
🪛 Flake8 (7.2.0)
test/functional/feature_asset_locks.py
[error] 367-367: continuation line under-indented for visual indent
(E128)
🔇 Additional comments (8)
src/chainparams.cpp (1)
823-826: REGTEST v23 window and thresholds update is consistentnWindowSize=250 with thresholds at 80% (200) and 60% (150) matches the stated intent and aligns with the updated functional test expectations (e.g., v23 activation at height 750).
test/functional/feature_asset_locks.py (7)
364-368: Good clarification on mempool vs consensus validation pathsThe comment explicitly explaining why mempool returns “not-verified” (lack of CreditPool/index context) is helpful and reduces confusion during test failures.
403-405: Spork reset sequencing looks correctResetting IS and quorum DKG sporks back to defaults after the negative checks keeps subsequent assertions deterministic. LGTM.
451-451: Reducing generate_batch to 25 improves runtime without changing semanticsDropping the batch size here is a sensible speedup given the updated quorum scanning rule and activation heights.
652-655: Explicit withdrawals activation height usage matches regtest paramsactivate_by_name('withdrawals', 600) aligns with consensus.WithdrawalsHeight=600 for regtest. Good consistency.
677-681: Post-deactivation checks match the “active + 1” ruleExplicitly verifying the signing quorum leaves the active set after mining one more quorum aligns with the implementation change in C++.
705-708: v23 activation at height 750 matches updated regtest window (250) semanticsThis keeps the test tightly aligned with the new window and threshold computations.
723-737: v23 path mirrors withdrawals path correctlyReplicating the inactive-but-valid and then too-old transitions for v23 provides symmetric coverage. Nice.
|
@knst I'll pause the automatic reviews for this PR. ✅ Actions performedReviews paused. |
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@UdjinM6 review is here; it has been triggered at some point, but not immediatelly posted. I restarted coderabbit and got the same answer as you "it's incremental and no action taken". |
3 participants
Issue being fixed or feature implemented
Fork
withdrawalsis activated long time ago. No more need to test functionality for pre-'withdrawals' fork when only 2 quorum could be used for signing messages.What was done?
Removed pre-'withdrawals' logic to enforce using only 2 quorums for signing of withdrawals from code and from functional test
feature_asset_locks.pyFunctional test
feature_asset_locks.pygenerate less quorums and less blocks which significantly shortened runtime of this test, 166->139seconds on median; up to 40 seconds if quorum has been generated non-favourable way.How Has This Been Tested?
Run in parallel 20 jobs
Breaking Changes
Removed pre-'withdrawals' requirement of using only 2 last quorums
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.