chore(prague3): informational comments about prague3 validation rules#251
chore(prague3): informational comments about prague3 validation rules#251calbera wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR implements Prague3 historical-window validation in the Berachain consensus layer. It adds post-execution checks to block validation that enforce ERC20 transfer restrictions on blocked addresses and the BEX vault, plus validates InternalBalanceChanged events. Documentation is updated and test coverage is extended for Prague1/Prague3 activation boundaries. ChangesPrague3 Historical-Window Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Adds clarifying documentation around the Prague3 (historical-window) validation rules and introduces/organizes a dedicated historical genesis fixture, while also filling a missing unit-test case for Prague1 proposer pubkey validation.
Changes:
- Added a
tests/fixtures/historical/directory with a Prague3-specific genesis fixture and README describing its intent. - Updated the Prague3 e2e test to reference the historical fixture and added module-level documentation explaining the scenario.
- Expanded Prague3/Prague1 documentation in consensus/execution error definitions and added a unit test covering the “post-Prague1 proposer pubkey is accepted” case.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tests/fixtures/historical/README.md |
Documents the purpose and scope of “historical window” fixtures, including the Prague3 fixture. |
tests/fixtures/historical/eth-genesis-prague3.json |
Adds a Prague3-oriented genesis/chain-spec fixture used by e2e coverage. |
tests/e2e/prague3_empty_block_test.rs |
Points the Prague3 e2e test at the historical fixture and adds scenario documentation. |
src/node/evm/error.rs |
Adds explanatory comments to Prague3 execution errors (reachability / historical context). |
src/consensus/mod.rs |
Adds detailed Prague3 validation rationale comments and adds a missing unit test for post-Prague1 proposer pubkey acceptance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // which returns true only inside that historical window. Outside the window | ||
| // (i.e. on the live tip post-Prague4, and pre-Prague3) the gate short-circuits | ||
| // and none of the loops run. The path remains a hard requirement for any node | ||
| // re-executing the chain from genesis. |
| /// Prague3: Block contains invalid ERC20 transfer from/to blocked address. | ||
| /// Only reachable inside the historical Prague3 window | ||
| /// `[1762164459, 1762963200)` on mainnet; see `consensus/mod.rs`. | ||
| #[error( |
| /// Prague3: Block contains InternalBalanceChanged event from BEX vault. | ||
| /// Only reachable inside the historical Prague3 window | ||
| /// `[1762164459, 1762963200)` on mainnet; see `consensus/mod.rs`. | ||
| #[error("Prague3 violation: InternalBalanceChanged event from BEX vault {vault_address}")] |
| /// Prague3: Block contains ERC20 transfer from/to BEX vault. | ||
| /// Only reachable inside the historical Prague3 window | ||
| /// `[1762164459, 1762963200)` on mainnet; see `consensus/mod.rs`. | ||
| #[error("Prague3 violation: ERC20 transfer from/to BEX vault {vault_address}")] |
| //! Pins the Prague3 empty-block builder behavior that ran on Berachain mainnet | ||
| //! during the closed historical window `[1762164459, 1762963200)`. The genesis | ||
| //! fixture lives under `tests/fixtures/historical/` (see the README there) to | ||
| //! signal that it models a non-live fork window. The consensus rules being | ||
| //! exercised are documented in `src/consensus/mod.rs`. |
| Models the **Prague3 emergency window**, active on Berachain mainnet for | ||
| timestamps `[1762164459, 1762963200)` — 2025-11-03 to 2025-11-12, when Prague4 | ||
| ended the restrictions. Used by `tests/e2e/prague3_empty_block_test.rs` to | ||
| exercise the empty-block builder path that ran while Prague3 was the live tip. |
![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/consensus/mod.rs (1)
120-147: 💤 Low valueComment block clearly justifies the "not dead code" status and the scoping asymmetry.
The block usefully (a) anchors the historical window to concrete timestamps consistent with
error.rsvariants and the historical README, (b) explains why the gate is non-dead for genesis re-execution, and (c) documents whyTransferis intentionally unscoped whileInternalBalanceChangedand the deposit parser are scoped. This is exactly the kind of "why" rationale future maintainers and bug-bounty triagers will need.One small nit: the comment refers to
deposits.rswithout a path qualifier. If readers grep for it, they will likely land insrc/node/evm/deposits.rs(or wherever the deposit parser lives); a relative path (e.g.node/evm/deposits.rs) would make it unambiguous, mirroring the explicitconsensus/mod.rsreference used elsewhere.🤖 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/consensus/mod.rs` around lines 120 - 147, Update the inline comment under the "Prague3 historical-window validation" header in consensus/mod.rs to replace the ambiguous reference "deposits.rs" with the relative path "node/evm/deposits.rs" so readers who grep the repo land on the correct file; keep the surrounding rationale and wording intact except for this path clarification.
🤖 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/consensus/mod.rs`:
- Around line 120-147: Update the inline comment under the "Prague3
historical-window validation" header in consensus/mod.rs to replace the
ambiguous reference "deposits.rs" with the relative path "node/evm/deposits.rs"
so readers who grep the repo land on the correct file; keep the surrounding
rationale and wording intact except for this path clarification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad876d1d-d856-4588-acfb-6d864057e3c4
📒 Files selected for processing (5)
src/consensus/mod.rssrc/node/evm/error.rstests/e2e/prague3_empty_block_test.rstests/fixtures/historical/README.mdtests/fixtures/historical/eth-genesis-prague3.json
Also adds unit test for the missing case of accepts_post_prague1_proposer_pubkey
Summary by CodeRabbit
New Features
Tests
Documentation