feat: implement a fork to double withdrawals limit #6662
Conversation
WalkthroughThe changes introduce a new deployment condition, 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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ 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 (6)
src/evo/creditpool.h (1)
122-122: Add new withdrawal limitLimitAmountV23.
The constantLimitAmountV23 = 4000 * COINcorrectly extends the withdrawal limit tiers. Consider adding an inline comment referencingDEPLOYMENT_WITHDRAWALS2to clarify its usage context for future maintainers.src/evo/creditpool.cpp (1)
186-188: Withdrawal-limit math looks correct but consider guarding against future constant regressionsThe new branch correctly caps
currentLimitwith
max(0, min(currentLimit, LimitAmountV23 ‑ latelyUnlocked)), which keeps the result non-negative even if the sliding-window total already exceeds the v23 cap.Two small follow-ups you might want to consider:
- Add a short comment explaining why the
max(0, …)is required (negative values mean the full window is already consumed). This will help future maintainers avoid “fixing” it away.- Consider extracting the deployment/limit selection logic into a helper (e.g.
CalcWithdrawalsLimit) to avoid another cascade ofif/elseblocks when v24 comes along.src/chainparams.cpp (1)
220-228: DRY-up candidate – repeated parameter blocksThe per-network initialisation for
DEPLOYMENT_WITHDRAWALS2duplicates the 7-line block four times with only the start/timeout/window numbers changing.
Moving this into a small helper (e.g.InitWithdrawals2Deployment(consensus, start, timeout, window)) would eliminate copy-paste and lower the risk of propagating typos to only one network.test/functional/feature_asset_locks.py (3)
650-704: High duplication between v22 and v23 fork tests
test_withdrawals_forkandtest_withdrawals2_forkare nearly identical except for the activation height, cap (2 000 → 4 000 COIN) and index ranges. Extracting the common logic into a parametrised helper would shrink the file and make future caps trivial to add.- def test_withdrawals_fork(self, ...): - cap = 2000 * COIN - self._generic_withdrawals_fork('withdrawals', 1750, cap, 501) - - def test_withdrawals2_fork(self, ...): - cap = 4000 * COIN - self._generic_withdrawals_fork('withdrawals2', 1750, cap, 601) + def _generic_withdrawals_fork(self, name, act_height, cap, index_start, ...): + ...This would also prevent subtle drift between the two test suites.
706-763: Hard-coded 4 000 COIN magic numberThe test encodes the v23 cap (
4000 * COIN) directly. If the constantLimitAmountV23ever changes, the test will silently become incorrect. Suggest querying the node forgetblockchaininfo()["softforks"]["withdrawals2"]["limit"](once exposed) or importing the value from the C++ header viactypes/pybind11, or at least defining it once at the top of the file.WITHDRAWALS2_LIMIT = 4000 * COIN # TODO: keep in sync with LimitAmountV23
718-744: Potentially long-running while-loop in CIThe loop advances by mining whole quorums until
asset_unlock_txbecomes invalid; on slow CI machines this can add minutes. Consider imposing a hard iteration limit and failing early if the condition is never met to avoid indefinite hangs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
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/rpc/blockchain.cpp(1 hunks)test/functional/feature_asset_locks.py(3 hunks)test/functional/rpc_blockchain.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/evo/creditpool.cpp (2)
src/evo/creditpool.h (2)
block_index(136-136)block_index(139-140)src/chainparams.cpp (2)
Params(1382-1385)Params(1382-1382)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
🔇 Additional comments (6)
src/deploymentinfo.cpp (1)
18-21: New version bits deployment registration for withdrawals2 looks correct.
The entry for"withdrawals2"withgbt_force=truematches the corresponding consensus parameter addition insrc/consensus/params.hand aligns with the version bits registration elsewhere.src/consensus/params.h (1)
43-43: Register new deploymentDEPLOYMENT_WITHDRAWALS2in the enum.
The new enum value is placed correctly right afterDEPLOYMENT_WITHDRAWALS, and the existing note reminds to updateVersionBitsDeploymentInfo. TheValidDeploymentcheck will include the new entry.src/rpc/blockchain.cpp (1)
1545-1547: IncludeDEPLOYMENT_WITHDRAWALS2in RPC softfork reporting.
ThegetblockchaininfoRPC now processesDEPLOYMENT_WITHDRAWALS2alongside the other EHF deployments, ensuring the"withdrawals2"softfork appears with correct status and parameters.test/functional/rpc_blockchain.py (1)
228-238: Update expectedsoftforksto include"withdrawals2".
The functional test forgetblockchaininfonow asserts the presence and correct structure of the"withdrawals2"entry, aligning test expectations with the implementation changes.src/chainparams.cpp (2)
426-434: Missingmin_activation_heightcommentFor the first withdrawals fork (
DEPLOYMENT_WITHDRAWALS) we left the defaultmin_activation_height = 0.
If the intent forWITHDRAWALS2is the same, adding an explicit comment might avoid doubts later.
If we actually want an activation delay (like Taproot’s 2016-block rule in Bitcoin), setmin_activation_heightnow to avoid mainnet surprises.Also applies to: 607-615, 852-860
220-228:✅ Verification successful
Bit 12 re-use check
Bit 12 is now assigned to
DEPLOYMENT_WITHDRAWALS2. Please double-check that no other deployments or legacy soft-forks already use this bit (even in dead/test code). A quick repository-wide scan would avoid a hard-to-debug version-bits collision.
🏁 Script executed:
#!/usr/bin/env bash # Search all consensus deployments for bit = 12 (except the four blocks just introduced). fd -e cpp -e h | xargs grep -n "bit *= *12" | grep -v "WITHDRAWALS2" || trueLength of output: 75
No version-bits collision found for bit 12
A repository-wide search for otherbit = 12assignments (in .cpp/.h files) returned no matches besidesDEPLOYMENT_WITHDRAWALS2. You can safely proceed with using bit 12.
|
Recommend we don't make this change, the current daily limit of 2000 Dash is very sufficient and almost never gets hit. This just reduces the protections in place of a credit pool drain. https://mnowatch.org/evonodes/ |
|
@kxcd Please make conceptual objections here: dashpay/dips#165. This PR should be reserved for implementation details. |
… featureset 98749a7 feat: define BIP 9 fork `DEPLOYMENT_V23` for new version featureset (Kittywhiskers Van Gogh) Pull request description: ## Motivation Should handle the deployment needs for [dash#6662](#6662) and [dash#6665](#6665) as they're expected to be shipped in the same major version. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 98749a7 PastaPastaPasta: utACK 98749a7 Tree-SHA512: b61302a0bcf236f06661a4a53f1d7b3ab0f65659c1c9ab2fcb782457a25eb1e6bdc942647dca6a4e22686e0c7cc83405e099e1fa790cf007d2bee3eedc456d5f
|
This pull request has conflicts, please rebase. |
Issue being fixed or feature implemented
Implements dashpay/dips#165
What was done?
Bump the limit via new fork, add tests
How Has This Been Tested?
run test
Breaking Changes
n/a
Checklist: