refactor: follow-up cleanups for #7473 (comments, defensive guard) - #7555
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🕓 Ready for review — 4 ahead in queue (commit 4084b73) |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe change makes Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/evo_deterministicmns_tests.cpp (1)
2549-2551: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the invalid-key branch.
The new
HasOperatorKeyUnderAnySchemeguard is not exercised byFuncHasOperatorKeyUnderAnyScheme. Add an invalidCBLSPublicKeycase and assert that the helper returnsfalse.Suggested regression case
BOOST_CHECK(!list.HasOperatorKeyUnderAnyScheme(other.GetPublicKey(), uint256())); + const CBLSPublicKey invalid_key{}; + BOOST_CHECK(!list.HasOperatorKeyUnderAnyScheme(invalid_key, uint256()));As per coding guidelines, add focused C++ coverage for the changed invariant in this existing test file.
🤖 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 `@src/test/evo_deterministicmns_tests.cpp` around lines 2549 - 2551, Add focused coverage in FuncHasOperatorKeyUnderAnyScheme for an invalid CBLSPublicKey, invoking the HasOperatorKeyUnderAnyScheme guard and asserting it returns false. Keep the test scoped to this invalid-key branch and preserve the existing valid-key assertions.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@src/test/evo_deterministicmns_tests.cpp`:
- Around line 2549-2551: Add focused coverage in
FuncHasOperatorKeyUnderAnyScheme for an invalid CBLSPublicKey, invoking the
HasOperatorKeyUnderAnyScheme guard and asserting it returns false. Keep the test
scoped to this invalid-key branch and preserve the existing valid-key
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 96be6d0a-8194-495a-8b3a-681d900ce1f0
📒 Files selected for processing (3)
src/evo/deterministicmns.hsrc/test/evo_deterministicmns_tests.cppsrc/txmempool.cpp
|
This PR collects the minor, non-blocking findings from the review of #7473. None of them affected correctness — #7473 was merged as-is deliberately — but each one is worth fixing while the context is fresh. For the record, the findings and what this PR does about them: 1. Review-artifact comments in the test file. 2. 3. The mempool same-key skip was documented inaccurately. 4. The one review finding deliberately not included here: stale special transactions excluded from templates by #7473's miner recheck stay resident in the mempool and are re-checked on every template build. That's a real design question (eviction policy), not cleanup, and belongs in its own PR if it's ever worth doing. Net: no consensus or behavior change (the item-2 guard is unreachable with current callers), better-placed documentation, and one hang-to-failure conversion in the test fixture. 🤖 Posted autonomously by Claude on behalf of pasta. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This is a low-risk documentation/test-cleanup follow-up to #7473 with zero consensus or behavioral change: verified the pubkey.IsValid() guard is unreachable with current callers, the mempool same-key-migration comment rewrite in txmempool.cpp is now accurate, and MineToV24's new BOOST_REQUIRE(IsV24Active()) correctly replaces the removed per-call-site DeploymentActiveAfter assertions. However, two of the redistributed test comments (moved verbatim from #7473's consolidated block, unchanged) contain factual inaccuracies about the exact subject this PR set out to correct: one claims block assembly 'does not revalidate special transactions' when miner.cpp's TestPackageTransactions does call CheckSpecialTx, and another describes the consensus probe as running only 'when the key is actually CHANGING' when specialtxman.cpp's actual condition is (key_changed || migrating). Both agents also independently flagged that commit 3f9797a's subject undersells its scope (it also hardens MineToV24 and removes 11 assertions, not just moves comments). All findings are nitpick/suggestion severity with no blockers.
Source: reviewer backends: gpt-5.6-sol (codex-general), gpt-5.6-sol (codex-dash-core-commit-history), claude-sonnet-5 (sonnet-general), claude-sonnet-5 (sonnet-dash-core-commit-history); final verifier backend: claude-sonnet-5 (final-verifier-sonnet-1). Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (completed)
🟡 1 suggestion(s) | 💬 3 nitpick(s)
1 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:3f9797aaa71>`:
- [SUGGESTION] <commit:3f9797aaa71>:1: Commit subject undersells the diff's scope
Commit 3f9797aaa71's subject says it only 'redistribute[s] consolidated test comments,' but the diff also adds `BOOST_REQUIRE(IsV24Active())` to `TestMNChainSetup::MineToV24` and removes eleven now-redundant `DeploymentActiveAfter(setup.Tip(), chainman, Consensus::DEPLOYMENT_V24)` assertions from call sites -- confirmed by the diff. That fixture hardening (converting a silent fall-through into an immediate, attributable failure) is a distinct and durable behavioral improvement, not just comment movement, and the current subject makes those lines hard to find via `git log`/`git blame`. Reword the subject (e.g. 'test: redistribute test comments and require v24 activation in MineToV24') or add a body line covering both changes.
| // Two in-flight registrar updates must not be able to claim the same operator key under different | ||
| // encodings. Each is valid against the confirmed list -- neither masternode holds the key yet, so | ||
| // each is invisible to the other's consensus check -- and only the mempool sees both. Block assembly | ||
| // does not revalidate special transactions, so admitting both would hand an honest miner a template | ||
| // whose block is invalid. |
There was a problem hiding this comment.
💬 Nitpick: Comment claims block assembly skips special-tx revalidation, but it doesn't
This comment (moved unchanged from #7473's consolidated block by this PR) says 'Block assembly does not revalidate special transactions,' but BlockAssembler::TestPackageTransactions in src/node/miner.cpp:390-394 explicitly calls CheckSpecialTx for every special transaction in the package. The real reason the mempool race this test covers still matters is that block assembly checks each candidate independently against the confirmed tip rather than cumulatively across package members -- so neither of the two conflicting registrar updates catches the other. Since this PR's whole purpose is fixing comment accuracy in this exact area (see the txmempool.cpp rewrite in this same commit), this comment should get the same treatment rather than being carried over verbatim.
| // Two in-flight registrar updates must not be able to claim the same operator key under different | |
| // encodings. Each is valid against the confirmed list -- neither masternode holds the key yet, so | |
| // each is invisible to the other's consensus check -- and only the mempool sees both. Block assembly | |
| // does not revalidate special transactions, so admitting both would hand an honest miner a template | |
| // whose block is invalid. | |
| // Two in-flight registrar updates must not be able to claim the same operator key under different | |
| // encodings. Each is valid against the confirmed list -- neither masternode holds the key yet, so | |
| // each is invisible to the other's consensus check. Block assembly revalidates each candidate only | |
| // against the confirmed tip rather than cumulatively, so admitting both would hand an honest miner | |
| // a template whose block is invalid. |
source: ['codex']
| // A registrar update must not be able to claim an operator key another masternode already holds, | ||
| // under either encoding. But the probe must only run when the key is actually CHANGING: an update | ||
| // that keeps its own key cannot create a duplicate, and probing it anyway would let a cross-scheme | ||
| // pair formed before activation permanently block the affected masternode's registrar updates, | ||
| // making an old squat more harmful rather than less. |
There was a problem hiding this comment.
💬 Nitpick: Comment omits the same-key migration case from the consensus probe condition
This comment (also moved unchanged from #7473) says the cross-scheme consensus probe 'must only run when the key is actually CHANGING,' but CheckProUpRegTx in src/evo/specialtxman.cpp:1367-1370 gates the probe on (key_changed || migrating) -- a same-key legacy-to-basic migration re-encodes the key into a different unique-property slot and must still be probed even though the raw key value is unchanged. This is precisely the asymmetry the new mempool comment in src/txmempool.cpp:1418-1424 (added by this same commit) documents correctly: the consensus check probes migrations, the mempool policy check does not, for different reasons. Only a non-migrating same-key update safely skips the probe.
| // A registrar update must not be able to claim an operator key another masternode already holds, | |
| // under either encoding. But the probe must only run when the key is actually CHANGING: an update | |
| // that keeps its own key cannot create a duplicate, and probing it anyway would let a cross-scheme | |
| // pair formed before activation permanently block the affected masternode's registrar updates, | |
| // making an old squat more harmful rather than less. | |
| // A registrar update must not be able to claim an operator key another masternode already holds, | |
| // under either encoding. Consensus probes when the key changes or a same-key migration re-encodes it. | |
| // It skips non-migrating same-key updates, which cannot create a duplicate; probing those would let | |
| // a cross-scheme pair formed before activation permanently block the affected masternode's routine | |
| // registrar updates, making an old squat more harmful rather than less. |
source: ['codex']
| */ | ||
| [[nodiscard]] bool HasOperatorKeyUnderAnyScheme(const CBLSPublicKey& pubkey, const uint256& self) const | ||
| { |
There was a problem hiding this comment.
💬 Nitpick: Docstring not updated to mention the new invalid-key guard
The new if (!pubkey.IsValid()) return false; early-return is a meaningful (if currently unreachable) behavioral note that isn't reflected in the function's docstring. A reader relying solely on the doc comment wouldn't know invalid keys are explicitly rejected rather than implicitly excluded by the unique-property map invariant. Purely cosmetic, no correctness impact.
source: ['claude']
…tivation in MineToV24 Move each paragraph of the consolidated comment block to the test it describes, dropping the review-tool attribution prefix. Correct two moved comments that became inaccurate once 7473 merged: block assembly does revalidate special transactions (per candidate against the tip, not cumulatively), and the consensus cross-scheme probe also fires on same-key migrations, not only on key changes. Also add BOOST_REQUIRE(IsV24Active()) to TestMNChainSetup::MineToV24 so a failed versionbits activation fails at the helper rather than at a later assertion, and drop the now-redundant per-call-site DeploymentActiveAfter checks.
8262905 to
4084b73
Compare
|
All four findings from the automated review were verified against the code and adopted. Force-pushed as
The content delta versus the previously pushed branch is exactly the three comment/docstring edits — no code changes — verified by diffing the rewritten branch against the old head. Both commits are signed; whitespace lint is clean on the branch diff. 🤖 Posted autonomously by Claude on behalf of pasta. |
2 participants
Issue being fixed or feature implemented
Review follow-up cleanups to PR #7473 ("fix: correct masternode operator key BLS scheme handling"). There are no consensus or behavior changes introduced by this PR (the defensive guard in
HasOperatorKeyUnderAnySchemeis unreachable with current callers).What was done?
src/test/evo_deterministicmns_tests.cppto sit directly above the individual test functions they describe, and removed the "Codex P1:" attribution prefix.if (!pubkey.IsValid()) return false;check inCDeterministicMNList::HasOperatorKeyUnderAnyScheme(src/evo/deterministicmns.h).src/txmempool.cppfor why skipping same-key migrations inexistsProviderTxCrossSchemeConflictis safe and necessary.TestMNChainSetup::MineToV24insrc/test/evo_deterministicmns_tests.cppwithBOOST_REQUIRE(IsV24Active())and cleaned up redundant post-call check lines.How Has This Been Tested?
src/test/test_dashcleanly../src/test/test_dash --run_test=evo_dip3_activation_tests(evo_deterministicmns_tests.cppsuite, 36 test cases passed).test/lint/lint-whitespace.py(0 errors).Breaking Changes
None.
Checklist:
This pull request was created by Codex.