test: batch DIP0008 activation with slow_mode=False - #7501
Conversation
|
✅ Final review complete — no blockers (commit ccc3858) |
3bb0a3a to
52b2e58
Compare
Changed-test CI timing comparisonTest: Develop baseline:
The changed test was faster in all five applicable CI configurations, saving 1m 05s across them in this pair of runs. These are the functional test runner's per-test durations from the CI logs, not whole-job or whole-suite runtimes. The changed test passed in every applicable configuration. The PR's overall SQLite job failure occurred elsewhere and does not affect these test timings. |
![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: 52b2e58e82
ℹ️ 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".
| n.invalidateblock(oldhash) | ||
| self.sync_all() | ||
| # Smaller batches keep the replacement chain synchronized on slow CI builds after invalidateblock. | ||
| first_quorum = self.test_dip8_quorum_merkle_root_activation(False, True) |
There was a problem hiding this comment.
When the post-invalidateblock activation runs, this True argument flows through to activate_by_name, where slow_mode=True selects batches of 10 rather than 50. Consequently, this path retains the previous batching behavior despite the change description stating that both DIP0008 activation paths use slow_mode=False; only the first call is accelerated. Pass False or omit the second argument to apply the intended optimization to both paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentional for the post-invalidateblock path — smaller batches keep the replacement chain synchronized on slow CI. PR description updated accordingly.
WalkthroughThe DIP8 coinbase merkle roots functional test now omits the explicit Estimated code review effort: 1 (Trivial) | ~3 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/functional/feature_dip4_coinbasemerkleroots.py`:
- Around line 122-123: Update the first_quorum call to
test_dip8_quorum_merkle_root_activation so its second argument is False,
matching the fast activation mode used by the other DIP8 path and preserving the
intended batch size.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b611186-3d72-444b-bd5c-72aa60921251
📒 Files selected for processing (1)
test/functional/feature_dip4_coinbasemerkleroots.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This one-commit PR wires the previously-dead slow_mode parameter through to activate_by_name, letting the first (pre-reorg) DIP0008 activation use 50-block batches instead of the old default of 10. The second (post-invalidateblock) activation explicitly keeps slow_mode=True with a new comment justifying it, which is a sound and safe choice, but it directly contradicts the commit message's claim of using batch-50 "on both DIP0008 activation paths." That's a real, verifiable inconsistency between the stated rationale and the actual diff, though it has no functional or test-safety impact.
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— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 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 `test/functional/feature_dip4_coinbasemerkleroots.py`:
- [SUGGESTION] test/functional/feature_dip4_coinbasemerkleroots.py:122-123: Commit message claims batch-50 on both activation paths, but the second path still passes slow_mode=True
The commit message says this change makes both DIP0008 activation call sites use batch-50, but the code tells a different story: `test_dip8_quorum_merkle_root_activation(False, True)` explicitly passes `slow_mode=True`, which `activate_by_name` maps to `batch_size = 10` (see test_framework.py:1571). Only the first call at line 118 (which relies on the new `slow_mode=False` default) actually gets batch-50; the second, post-`invalidateblock` call intentionally keeps the old slow batching, as the added comment on line 122 explains. Keeping slow mode here is a reasonable and probably correct choice for CI stability after a reorg, but the commit message overstates the change ("use batch-50 on both DIP0008 activation paths") in a way that could mislead anyone reading history later. Update the commit message/PR description to say batch-50 now applies to the first (pre-reorg) activation only, while the post-reorg path intentionally remains slow.
|
CI note: failures in
Leaving the PR branch as-is; no commit, push, rebase, empty commit, or CI retrigger was performed. |
Wire the existing slow_mode parameter through to activate_by_name and use batch-50 on both DIP0008 activation paths. activate_by_name already syncs after every batch and disables SPORK_17, so batch-50 matches the former batch-10 behavior without the post-reorg catch-up risk that once justified slow_mode=True.
52b2e58 to
5cbf2d6
Compare
|
Follow-up on review feedback: the post- Amended the single feature commit so both activation call sites now use the default Final head: Validation on final head:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cbf2d6d65
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| n.invalidateblock(oldhash) | ||
| self.sync_all() | ||
| first_quorum = self.test_dip8_quorum_merkle_root_activation(False, True) | ||
| first_quorum = self.test_dip8_quorum_merkle_root_activation(False) |
There was a problem hiding this comment.
In the post-invalidateblock activation path, omitting the second argument makes test_dip8_quorum_merkle_root_activation use its default slow_mode=False, which activate_by_name turns into 50-block batches. On slow CI this replacement-chain path was intentionally kept on 10-block batches to stay synchronized, so this can reintroduce the flaky activation failure; pass True here so only the first activation uses fast batching. Fresh evidence is that the exact-head diff removed the only explicit True from this call.
Useful? React with 👍 / 👎.
|
Moving this back to draft after exact-head CI exposed a branch-owned blocker. On Next repair task: preserve the speedup only where block propagation is reliable—either choose a smaller/conditional activation batch for slow CI configurations or revert the fast path—then rerun the exact test under normal, TSAN, and multiprocess builds before marking ready again. |
The cross-node invalidateblock in run_test() leaves every other node with the
invalidated, higher-work tip recorded as node0's pindexBestKnownBlock.
UpdateBlockAvailability only advances that pointer on >= chain work, so the
lower-work replacement chain never displaces it, and FindNextBlocksToDownload
walks toward the invalidated chain and returns at !IsValid(BLOCK_VALID_TREE)
without requesting anything. HeadersDirectFetchBlocks is then the only way in,
and it bails out ("Large reorg, won't direct fetch") on forks deeper than
MAX_BLOCKS_IN_TRANSIT_PER_PEER, so a batch of 50 leaves the masternodes stranded
and sync_blocks() times out.
Batch-10 stays inside the direct-fetch window, which is why this path had always
used it. The pre-existing quorum path has no invalidated tip in peer state, uses
the ordinary parallel download, and keeps batch-50; state that explicitly at the
call site, since the helper and activate_by_name default slow_mode oppositely.
Co-Authored-By: Claude <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The current code correctly uses batch-50 for the pre-invalidation activation and batch-10 after invalidateblock; the networking implementation confirms that the retained higher-work best-known pointer, invalid-tree download walk, and 16-block direct-fetch limit require the smaller replacement-chain batch. Carried-forward prior findings: none—the exact prior commit-message mismatch is OUTDATED; new latest-delta findings: two suggestions covering the stale PR description and the known CI-unsafe intermediate commit.
Source: reviewers gpt-5.6-sol (Codex, 2 completed) and claude-sonnet-5 (Sonnet, 2 completed/1 failed); final verifier gpt-5.6-sol (Codex fallback for the Sonnet verifier); openclaw-agent is orchestration-only.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (completed)
🟡 2 suggestion(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 `test/functional/feature_dip4_coinbasemerkleroots.py`:
- [SUGGESTION] test/functional/feature_dip4_coinbasemerkleroots.py:122-126: PR description still claims batch-50 on both activation paths
The current code intentionally passes `slow_mode=True` after `invalidateblock`, which `activate_by_name` maps to batch size 10. This is justified by the networking code: `UpdateBlockAvailability` does not replace the invalidated higher-work `pindexBestKnownBlock` with lower-work replacement headers, `FindNextBlocksToDownload` returns when its walk reaches the invalid tree, and `HeadersDirectFetchBlocks` rejects replacement forks deeper than `MAX_BLOCKS_IN_TRANSIT_PER_PEER` (16). The PR description still says both paths use `slow_mode=False` and identifies `5cbf2d6d652` as the final tested head, although the current head is `ccc385800a0`. Earlier replies about updating the description were superseded when `5cbf2d6d652` switched both paths to batch-50; the corrective commit restored batch-10 without refreshing the metadata. Update the description to state the intentional batch-50/batch-10 split and refresh or remove the obsolete final-head test claim.
In `<commit:ccc385800a0>`:
- [SUGGESTION] <commit:ccc385800a0>:1: Squash the post-invalidateblock repair into the activation commit
Commit `5cbf2d6d652` changes the post-`invalidateblock` path to batch-50 and states that the former catch-up concern no longer applies, but exact-head TSAN and multiprocess CI repeatedly timed out with that revision. Commit `ccc385800a0` exists to reverse that immediately preceding change and explain the networking constraint. Keeping both leaves a known test-failing revision available to `git bisect` and preserves contradictory safety narratives, while the repository guidance says commits should generally pass tests and make sense independently. Squash the repair into the activation commit and preserve the direct-fetch explanation in the resulting commit message and code comment.
| # The other nodes still record the invalidated (higher work) tip as node0's best known | ||
| # block, which leaves headers direct fetch as the only way to reach them. That gives up on | ||
| # forks deeper than MAX_BLOCKS_IN_TRANSIT_PER_PEER (16), so slow_mode's batch of 10 is | ||
| # what keeps the replacement chain reachable here. | ||
| first_quorum = self.test_dip8_quorum_merkle_root_activation(False, slow_mode=True) |
There was a problem hiding this comment.
🟡 Suggestion: PR description still claims batch-50 on both activation paths
The current code intentionally passes slow_mode=True after invalidateblock, which activate_by_name maps to batch size 10. This is justified by the networking code: UpdateBlockAvailability does not replace the invalidated higher-work pindexBestKnownBlock with lower-work replacement headers, FindNextBlocksToDownload returns when its walk reaches the invalid tree, and HeadersDirectFetchBlocks rejects replacement forks deeper than MAX_BLOCKS_IN_TRANSIT_PER_PEER (16). The PR description still says both paths use slow_mode=False and identifies 5cbf2d6d652 as the final tested head, although the current head is ccc385800a0. Earlier replies about updating the description were superseded when 5cbf2d6d652 switched both paths to batch-50; the corrective commit restored batch-10 without refreshing the metadata. Update the description to state the intentional batch-50/batch-10 split and refresh or remove the obsolete final-head test claim.
source: ['claude', 'codex']
1 participant
Issue being fixed or feature implemented
feature_dip4_coinbasemerkleroots.pyactivates DIP0008 with the defaultslow_mode=Truepath, which mines activation blocks in batches of 10. That is slower than necessary:activate_by_namealready disables SPORK_17 for the whole activation window and syncs after every batch.What was done?
Pass the existing
slow_modeparameter through toactivate_by_nameand useslow_mode=False(batch size 50) on both DIP0008 activation paths, including the post-invalidateblockpath.Coverage is preserved:
DIP0008_HEIGHT).sync_blocks().How Has This Been Tested?
python3 -m py_compile test/functional/feature_dip4_coinbasemerkleroots.pypython3 test/lint/lint-python.py test/functional/feature_dip4_coinbasemerkleroots.pygit diff --check upstream/develop...HEADfeature_dip4_coinbasemerkleroots.pyon final head (5cbf2d6d652) — passed (89s)Breaking Changes
None.
Checklist: