Repository navigation
fix: check credit pool amount accumulation - #7816
PastaPastaPasta merged 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. WalkthroughCredit-pool lock and unlock processing now uses checked addition and rejects overflowing totals. Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to The change adds overflow checks to credit-pool accounting and rejects unrepresentable totals instead of silently overflowing. No actionable merge-blocking risk was found. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
PastaPastaPasta
changed the title
|
✅ Final review complete — no blockers (commit 3ef0555) · triage: low |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the complete change at c9f0345 and its surrounding validation, mining callers, arithmetic helper, and tests; no actionable in-scope defects were found. Checked additions preserve accounting state on rejection, withdrawal-first final accounting preserves representable net balances under validated amount invariants, and existing validation/RPC exception handlers cover the new overflow error. This was static verification only: the supplied CI snapshot confirms successful completed builds and lint checks, but four test jobs and the ASan/multiprocess builds remain pending, so the claim that full CI is green is not supported by that snapshot.
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff is a small, contained arithmetic-safety fix in credit-pool accounting using CheckedAdd and existing error paths, not a large or intricate change despite touching consensus-sensitive amounts. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 97% left, weekly 96% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
thepastaclaw
added
the
pastaclaw:approved
c9f0345 to
9fe3921
Compare
thepastaclaw
removed
the
pastaclaw:approved
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
Verified the complete diff at 9fe3921. No blocking correctness issue was found: one boundary-test suggestion is valid, while the claimed loss of withdrawal-index rollback coverage is contradicted by the source. This was a static review; no builds or tests were run, and the supplied CI snapshot shows successful formatting checks with build jobs still queued.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff is a small, contained arithmetic-safety fix in CCreditPoolDiff and withdrawal summation with focused regression coverage, not a large or intricate change despite touching consensus accounting. - Phase 1 reviewers: not run (skipped for throughput: 22 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/evo_assetlocks_tests.cpp`:
- [SUGGESTION] src/test/evo_assetlocks_tests.cpp:901-920: Cover final-balance overflow and withdrawal-first boundary arithmetic
This test starts with an empty pool and reaches the sessionLocked limit, so the rejected deposit exercises Lock's overflow check but never the new overflow exception in GetTotalLocked. It also does not distinguish withdrawal-first arithmetic from deposit-first arithmetic. Add public-interface scenarios with a starting balance at the CAmount maximum and zero Platform reward: a small deposit should process successfully but make GetTotalLocked throw; an equal deposit and withdrawal within currentLimit should instead return the maximum balance after both transactions, in either transaction order. These directly cover the newly introduced final-balance behavior without requiring a funded-block fixture.
| BOOST_FIXTURE_TEST_CASE(credit_pool_lock_arithmetic, TestChain100Setup) | ||
| { | ||
| LOCK(cs_main); | ||
| const auto* tip = m_node.chainman->ActiveChain().Tip(); | ||
| CCreditPoolDiff diff{CCreditPool{}, tip, m_node.chainman->GetConsensus(), 0}; | ||
| const CAmount maximum = std::numeric_limits<CAmount>::max(); | ||
| const auto lock_max_money = CreateCreditPoolLockTx(MAX_MONEY); | ||
| for (CAmount i = 0; i < maximum / MAX_MONEY; ++i) { | ||
| TxValidationState state; | ||
| BOOST_REQUIRE(diff.ProcessLockUnlockTransaction(*lock_max_money, state)); | ||
| if (i == 1) BOOST_CHECK_EQUAL(diff.GetTotalLocked(), 2 * MAX_MONEY); | ||
| } | ||
| TxValidationState remainder_state; | ||
| BOOST_REQUIRE(diff.ProcessLockUnlockTransaction(*CreateCreditPoolLockTx(maximum % MAX_MONEY), remainder_state)); | ||
| BOOST_CHECK_EQUAL(diff.GetTotalLocked(), maximum); | ||
|
|
||
| TxValidationState overflow_state; | ||
| BOOST_CHECK(!diff.ProcessLockUnlockTransaction(*CreateCreditPoolLockTx(1), overflow_state)); | ||
| BOOST_CHECK_EQUAL(overflow_state.GetRejectReason(), "failed-creditpool-lock-overflow"); | ||
| BOOST_CHECK_EQUAL(diff.GetTotalLocked(), maximum); |
There was a problem hiding this comment.
🟡 Suggestion: Cover final-balance overflow and withdrawal-first boundary arithmetic
This test starts with an empty pool and reaches the sessionLocked limit, so the rejected deposit exercises Lock's overflow check but never the new overflow exception in GetTotalLocked. It also does not distinguish withdrawal-first arithmetic from deposit-first arithmetic. Add public-interface scenarios with a starting balance at the CAmount maximum and zero Platform reward: a small deposit should process successfully but make GetTotalLocked throw; an equal deposit and withdrawal within currentLimit should instead return the maximum balance after both transactions, in either transaction order. These directly cover the newly introduced final-balance behavior without requiring a funded-block fixture.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Declining: a pool already at the CAmount maximum is unreachable (pool.locked is bounded by real supply), so testing GetTotalLocked()'s throw from that state would pin the evaluation order of an internal sum rather than observable behavior. The reachable overflow, stacked unfunded deposits, is covered by credit_pool_lock_arithmetic.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Still applies (re-reviewed at 3ef05555): You're right that the proposed maximum starting pool is unreachable, and that fixture request was incorrect. A modest nonzero pool still separates the final-balance overflow path from the session overflow covered here; checking the getter's exception and successful withdrawal cancellation would test observable public-interface behavior rather than mandate an internal evaluation order.
| BOOST_FIXTURE_TEST_CASE(credit_pool_package_atomicity, TestChain100Setup) | ||
| { | ||
| LOCK(cs_main); | ||
| const auto lock_two = CreateCreditPoolLockTx(2 * COIN); |
There was a problem hiding this comment.
seems unrelated; why does it changed?
There was a problem hiding this comment.
You're right, it's unrelated: package rollback isn't touched by this PR. Reverted those credit_pool_package_atomicity edits in 3ef0555; the test is back to develop's version.
🤖 Posted autonomously by Claude on behalf of pasta.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
9fe3921 to
3ef0555
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete PR diff at the exact head and found no blocking correctness issues. One non-blocking coverage suggestion remains: the current regression checks session-deposit overflow but not final-balance overflow or representable cancellation at the boundary. This was a static review; no builds or tests were run, and the supplied CI snapshot still shows checks queued, pending, or in progress.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff is a small, contained arithmetic-safety fix in creditpool.cpp using an existing checked-add helper, with a focused regression test, rather than a large or intricate change despite touching consensus accounting. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 99% left, weekly 88% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/evo_assetlocks_tests.cpp`:
- [SUGGESTION] src/test/evo_assetlocks_tests.cpp:901-920: Cover final-balance overflow and withdrawal-first boundary arithmetic
(existing thread: https://github.com/dashpay/dash/pull/7816#discussion_r4209694158)
The original request for a starting pool at the CAmount maximum was incorrect: funded accounting cannot produce that pool. The coverage gap nevertheless exists with a modest nonzero starting pool. Lock() checks only sessionLocked, so accepted bookkeeping can leave that subtotal representable while pool.locked + sessionLocked is not; the empty starting pool in this test never exercises GetTotalLocked()'s new overflow_error branch. CheckCreditPoolDiffForBlock calls the getter before input funding is checked, making this a distinct defensive arithmetic path within the PR's scope. Extend the public-interface boundary coverage with a modest nonzero pool and zero reward: check that an unrepresentable final balance throws, and that a withdrawal within currentLimit allows a representable final net balance despite an unrepresentable deposit-first intermediate sum, in either transaction order. These scenarios exercise the getter's observable exception and result, without asserting that the deposits form a valid funded block.
3 participants
Issue being fixed or feature implemented
Credit-pool deposit accounting uses unchecked signed addition.
ConnectBlockrunsProcessSpecialTxsInBlock(and with it the credit-pool diff) beforeCheckTxInputs, so asset-lock amounts are accumulated before their funding is checked. A block that stacks enough unfundedMAX_MONEYasset locks overflowssessionLocked, which is signed-overflow undefined behaviour and terminates trapping builds. Checking representability before mutation turns this into a normal validation failure and keeps correct accounting for representable deposit/withdrawal flows.What was done?
Use the existing
CheckedAddhelper for session deposit and withdrawal amounts and the withdrawal output sum. Assign session amounts only after the candidate sum passes. Calculate the final balance with checked arithmetic, subtracting withdrawals before adding deposits and the Platform reward. An unrepresentable final balance throws through the existing validation/mining error paths.The
CAmountgetter API, withdrawal limits and atomic package rollback remain intact. No aggregate or finalMAX_MONEYrule was added, and the final net balance is checked after processing rather than after each deposit.How Has This Been Tested?
make -C src dashd test/test_dashpassed../src/test/test_dash --run_test=evo_assetlocks_testspassed. The newcredit_pool_lock_arithmeticcase accumulates individually in-range deposits to theCAmountboundary through the publicProcessLockUnlockTransactionAPI and checks that the next deposit is rejected and leaves the diff unchanged. The existing suite still covers ordinary asset locks and package rollback.The fixture exercises the public bookkeeping interface; it is not a funded-block proof, and full-node exploitability was not reproduced.
Breaking Changes
Successful representable accounting retains its mathematical result. Unrepresentable sums now report an error. No monetary cap or activation rule was added.
Checklist: