fix(mining): validate cumulative special transaction state per package - #7570
fix(mining): validate cumulative special transaction state per package#7570PastaPastaPasta wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (4)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. 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 across complete packages. Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change makes block-template package accounting atomic so invalid packages are skipped instead of aborting template construction; no actionable merge-blocking risk remains beyond normal checks and review. 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)
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 |
|
✅ Final review complete — no blockers (commit 4886b9f) |
![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)
PastaPastaPasta
added
the
backport-candidate-23.1.x
label
ChatGPT Codex Connector
| --------- | ||
|
|
||
| - Block template creation now skips Asset Unlock transaction packages that | ||
| exceed credit pool limits instead of failing to create a template. (#7570) |
There was a problem hiding this comment.
should explain reasoning I think, otherwise it doesn't make sense for external's readers who won't read code.
There was a problem hiding this comment.
Expanded in 005c618 to explain that Asset Unlock limits are cumulative across a package, so a package can exceed the block limit even though its transactions were accepted individually. The note now also states why skipping the package lets miners continue building the template.\n\n---\n🤖 Posted autonomously by Codex on behalf of pasta.
| const auto unlock_seven = CreateCreditPoolUnlockTx(1, 7 * COIN); | ||
| const auto unlock_four = CreateCreditPoolUnlockTx(2, 4 * COIN); | ||
| const auto* tip = m_node.chainman->ActiveChain().Tip(); | ||
| CCreditPoolDiff diff{CCreditPool{100 * COIN, 10 * COIN}, tip, Params().GetConsensus(), 0}; |
There was a problem hiding this comment.
nit: use m_node.chainman.GetConsensus() instead
There was a problem hiding this comment.
Updated in 005c618 to use m_node.chainman->GetConsensus(). This keeps the tip and consensus parameters sourced from the same chain manager.\n\n---\n🤖 Posted autonomously by Codex on behalf of pasta.
| } | ||
|
|
||
| // This transaction will make it in; reset the failed counter. | ||
| nConsecutiveFailed = 0; |
There was a problem hiding this comment.
why nConsecutiveFailed moved?
There was a problem hiding this comment.
The new package-level EHF and credit-pool checks can reject a package after TestPackageTransactions(). Leaving the reset at its old location would clear the size/sigops failure streak even when one of those checks rejects the package and nothing is added. Moving it after validPackage preserves the intended invariant: the counter is reset only when the package will actually be added to the block.\n\n---\n🤖 Posted autonomously by Codex on behalf of pasta.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This pull request has conflicts, please rebase. |
005c618 to
4886b9f
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The implementation is sound, and no correctness blockers were identified at the exact head. Both commit-history suggestions are confirmed: the later commits amend work introduced earlier in this unmerged stack and should be folded into their logical parent commits to keep each revision complete and bisectable.
Source: reviewer backend model gpt-5.6-sol (Codex general and dash-core-commit-history); final verifier backend model claude-opus-4-6. 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)
🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 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 `<commit:90b8a3b>`:
- [SUGGESTION] <commit:90b8a3b>:1: Squash the failed-streak correction into the main fix
Commit 90b8a3b448afc340fb431fdc3cc534f48934ffb2 moves the `nConsecutiveFailed` reset past the package-level rejection path introduced by 4f666b964083186588621e75716807da4a124e1b. Without that correction, the intermediate revision resets the failed-package streak even when the newly added EHF or credit-pool checks reject the package and no transaction is added. Fold this commit into 4f666b9 so the primary implementation is behaviorally complete and independently bisectable.
In `<commit:4886b9f>`:
- [SUGGESTION] <commit:4886b9f>:1: Fold the generic review-feedback commit into its targets
Commit 4886b9f7ef6d8a8f69e2842826b3e7ba8a9b1492 combines two amendments to earlier commits under the generic subject `chore: address review feedback`. The release-note expansion belongs in 109c8f28a375dc78eaa0125dfade3f62d5eb8b5a, while the test fixture's switch from `Params().GetConsensus()` to `m_node.chainman->GetConsensus()` belongs in 4f666b964083186588621e75716807da4a124e1b. Split and squash these changes into their respective parent commits so the history preserves logical changes rather than review chronology.
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If these PRs merge firstThis PR will likely need a rebase:
|
3 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.