fix(mining): validate cumulative special transaction state per package - #7570
fix(mining): validate cumulative special transaction state per package#7570PastaPastaPasta wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughBlock template creation now validates Asset Lock/Unlock transactions at package scope. Credit-pool state rolls back when any transaction in a package fails. EHF signal duplicates are checked before package acceptance. Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BlockAssembler
participant TransactionPackage
participant CCreditPoolDiff
participant BlockTemplate
BlockAssembler->>TransactionPackage: sort ancestor package
BlockAssembler->>CCreditPoolDiff: validate Asset Lock/Unlock transactions
CCreditPoolDiff-->>BlockAssembler: accept or reject package atomically
BlockAssembler->>BlockTemplate: include valid package
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit 76bb80d) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 076c8c6efd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| bool CCreditPoolDiff::ProcessLockUnlockTransactions(const std::vector<CTransactionRef>& txs, TxValidationState& state) | ||
| { | ||
| auto initialIndexes = newIndexes; |
There was a problem hiding this comment.
When a template contains many independent Asset Unlock transactions, this copies every index accumulated from all previously accepted packages before processing each subsequent package. Because newIndexes grows by one per unlock, assembling an unlock-heavy block now performs O(n²) node allocations and hash insertions, which can substantially delay repeated getblocktemplate calls for blocks containing thousands of withdrawals. Track only the indexes inserted by the current package and erase those on rollback, rather than cloning the entire set.
AGENTS.md reference: AGENTS.md:L172-L172
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The package-level credit-pool and EHF accounting is logically sound, and the new tests cover the intended rollback behavior. One in-scope performance issue remains: cloning the cumulative unlock-index set for every package makes unlock-heavy block-template construction quadratic while holding both cs_main and the mempool lock.
Source: reviewer backends: gpt-5.6-sol (Codex general) and gpt-5.6-sol (Codex dash-core-commit-history); final verifier backend: gpt-5.6-sol (Codex). openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s)
🤖 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/evo/creditpool.cpp`:
- [SUGGESTION] src/evo/creditpool.cpp:325-340: Avoid copying all accepted unlock indexes for every package
`newIndexes` contains the indexes from every Asset Unlock already accepted into the candidate block, so copying the entire set before each package causes O(n²) hash-node allocations across independent unlock packages. A 2 MB template can contain thousands of small Asset Unlock transactions because the withdrawal limit constrains their total amount rather than their count. Once the amount limit is exhausted, each additional unlock package still copies all previously accepted indexes before immediately failing. This work occurs inside `CreateNewBlock()` while both `cs_main` and the mempool lock are held. Record only the indexes inserted by this invocation and erase those during rollback; the amount fields can continue using scalar snapshots.
| bool CCreditPoolDiff::ProcessLockUnlockTransactions(const std::vector<CTransactionRef>& txs, TxValidationState& state) | ||
| { | ||
| auto initialIndexes = newIndexes; | ||
| const auto initialLocked = sessionLocked; | ||
| const auto initialUnlocked = sessionUnlocked; | ||
|
|
||
| for (const auto& tx : txs) { | ||
| if (ProcessLockUnlockTransaction(*tx, state)) continue; | ||
|
|
||
| newIndexes = std::move(initialIndexes); | ||
| sessionLocked = initialLocked; | ||
| sessionUnlocked = initialUnlocked; | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Avoid copying all accepted unlock indexes for every package
newIndexes contains the indexes from every Asset Unlock already accepted into the candidate block, so copying the entire set before each package causes O(n²) hash-node allocations across independent unlock packages. A 2 MB template can contain thousands of small Asset Unlock transactions because the withdrawal limit constrains their total amount rather than their count. Once the amount limit is exhausted, each additional unlock package still copies all previously accepted indexes before immediately failing. This work occurs inside CreateNewBlock() while both cs_main and the mempool lock are held. Record only the indexes inserted by this invocation and erase those during rollback; the amount fields can continue using scalar snapshots.
| bool CCreditPoolDiff::ProcessLockUnlockTransactions(const std::vector<CTransactionRef>& txs, TxValidationState& state) | |
| { | |
| auto initialIndexes = newIndexes; | |
| const auto initialLocked = sessionLocked; | |
| const auto initialUnlocked = sessionUnlocked; | |
| for (const auto& tx : txs) { | |
| if (ProcessLockUnlockTransaction(*tx, state)) continue; | |
| newIndexes = std::move(initialIndexes); | |
| sessionLocked = initialLocked; | |
| sessionUnlocked = initialUnlocked; | |
| return false; | |
| } | |
| return true; | |
| } | |
| bool CCreditPoolDiff::ProcessLockUnlockTransactions(const std::vector<CTransactionRef>& txs, TxValidationState& state) | |
| { | |
| const auto initialLocked = sessionLocked; | |
| const auto initialUnlocked = sessionUnlocked; | |
| std::vector<uint64_t> packageIndexes; | |
| packageIndexes.reserve(txs.size()); | |
| for (const auto& tx : txs) { | |
| const bool isUnlock = tx->IsSpecialTxVersion() && tx->nType == TRANSACTION_ASSET_UNLOCK; | |
| if (ProcessLockUnlockTransaction(*tx, state)) { | |
| if (isUnlock) { | |
| const auto payload = GetTxPayload<CAssetUnlockPayload>(*tx); | |
| assert(payload); | |
| packageIndexes.emplace_back(payload->getIndex()); | |
| } | |
| continue; | |
| } | |
| for (const uint64_t index : packageIndexes) { | |
| newIndexes.erase(index); | |
| } | |
| sessionLocked = initialLocked; | |
| sessionUnlocked = initialUnlocked; | |
| return false; | |
| } | |
| return true; | |
| } |
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Avoid copying all accepted unlock indexes for every package no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
changed the title
929b5a4 to
7dbdfd0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/functional/feature_asset_locks.py (1)
819-820: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the continuation indentation.
Flake8 reports E128 for both
result_expectedarguments. Use a valid hanging indent at both call sites.
test/functional/feature_asset_locks.py#L819-L820: indentresult_expectedas a hanging argument.test/functional/feature_asset_locks.py#L834-L835: indentresult_expectedas a hanging argument.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/feature_asset_locks.py` around lines 819 - 820, Fix the hanging indentation of the result_expected argument in both self.check_mempool_result call sites: test/functional/feature_asset_locks.py lines 819-820 and 834-835. Align each continuation with a valid hanging-indent style so Flake8 no longer reports E128; no behavioral changes are needed.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/functional/feature_asset_locks.py`:
- Around line 819-820: Fix the hanging indentation of the result_expected
argument in both self.check_mempool_result call sites:
test/functional/feature_asset_locks.py lines 819-820 and 834-835. Align each
continuation with a valid hanging-indent style so Flake8 no longer reports E128;
no behavioral changes are needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a546b6f7-3e30-4e84-8587-a028e39c71c2
📒 Files selected for processing (4)
src/evo/creditpool.cppsrc/evo/creditpool.hsrc/test/evo_assetlocks_tests.cpptest/functional/feature_asset_locks.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact-head implementation validates special-transaction packages atomically and now rolls back only unlock indexes inserted by the failing package, preserving previously accepted state without copying the cumulative index set. The prior performance finding is fixed, and no new in-scope correctness issues were identified.
Source: reviewer backends: gpt-5.6-sol (Codex general) and gpt-5.6-sol (Codex dash-core-commit-history); final verifier backend: gpt-5.6-sol (Codex). openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head 76bb80d, package-level credit-pool and EHF accounting is applied after non-mutating package checks and committed only for accepted packages. The rollback logic preserves previously accepted unlock indexes while restoring package-local amounts, and the added unit and functional coverage exercises the intended regression paths; no in-scope defects were confirmed.
Source: reviewer backend model gpt-5.6-sol (Codex general); final verifier backend model gpt-5.6-sol (Codex verifier). openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
2 participants
Issue being fixed or feature implemented
This pull request is based directly on develop and does not depend on another pull request.
What was done?
How Has This Been Tested?
Breaking Changes
None. Consensus validation and transaction serialization are unchanged; this changes block-template package selection so invalid packages are skipped instead of poisoning or aborting template construction.
Checklist:
This pull request was created by Codex.