Skip to content

core/vm: add hardfork precompile continuity checks#2234

Merged
pratikspatil024 merged 2 commits into
developfrom
psp-pos-3598-precompile-continuity
May 20, 2026
Merged

core/vm: add hardfork precompile continuity checks#2234
pratikspatil024 merged 2 commits into
developfrom
psp-pos-3598-precompile-continuity

Conversation

@pratikspatil024
Copy link
Copy Markdown
Member

Summary

Adds Bor VM hardfork precompile-continuity coverage and repo-local review guidance so future VM hardfork reviews explicitly compare the active precompile profile across the previous fork, the new fork, and historical replay windows for already-produced blocks.

The new tests lock expected precompile profile transitions around Prague, Madhugiri, MadhugiriPro, Lisovo, LisovoPro, and Chicago, including mainnet and Amoy activation-boundary checks. This is intended to catch Pro-style regressions where a fork accidentally drops a retained precompile, keeps a retired precompile active after the intended retirement point, or breaks replay expectations for historical blocks.

Also updates AGENTS.md and .claude/rules/hardfork-rollout.md so standalone Bor reviews treat VM/precompile hardforks as hardfork-shaped and require an explicit intended precompile delta versus the previous fork.

Backtesting

Backtested the approach against the historical Pro-related review windows:

  • Madhugiri/MadhugiriPro path: the continuity checks flag missing P256 carry-forward.
  • Lisovo/LisovoPro path: the continuity checks flag KZG retention/removal boundary risk and historical replay expectations.

A full AI review fan-out against the historical hardfork-shaped diffs also produced the expected CRITICAL/HIGH findings for missing precompile continuity coverage and incorrect active precompile profiles.

Executed tests

go test ./core/vm -run "TestBorHardforkPrecompileContinuity|TestPrecompiledP256VerifyAlwaysAvailableInHFs|TestKZGPointEvaluationPrecompileRemoval|TestLisovoP256VerifyGasCost|TestLisovoCLZOpcode|TestPIP88PrecompileGasCosts"
git -C /Users/ppatil/Polygon/pos-team-workspace/repositories/bor diff --check

Rollout notes

Test and review-guidance only. No consensus behavior changes.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.70%. Comparing base (3350958) to head (9a75f5f).
⚠️ Report is 4 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2234      +/-   ##
===========================================
+ Coverage    52.33%   52.70%   +0.37%     
===========================================
  Files          885      885              
  Lines       155904   156673     +769     
===========================================
+ Hits         81587    82574     +987     
+ Misses       69080    68831     -249     
- Partials      5237     5268      +31     

see 41 files with indirect coverage changes
see 41 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pratikspatil024 pratikspatil024 marked this pull request as ready for review May 19, 2026 15:00
Copilot AI review requested due to automatic review settings May 19, 2026 15:00
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

marcello33
marcello33 previously approved these changes May 19, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds Bor VM hardfork precompile continuity coverage to prevent regressions where a new VM fork accidentally changes the active precompile profile (including historical replay windows), and updates repo-local review guidance to make these checks an explicit part of hardfork-shaped reviews.

Changes:

  • Introduces a precompile “profile matrix” test that snapshots active precompiles (and key implementation variants) across Prague → Madhugiri → MadhugiriPro → Lisovo → LisovoPro → Chicago.
  • Adds mainnet/Amoy activation-boundary assertions at N-1, N, and N+1 for the above Bor forks.
  • Updates hardfork rollout guidance (AGENTS.md, .claude/rules/hardfork-rollout.md) to require explicit review of precompile deltas and continuity expectations.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
core/vm/contracts_continuity_test.go Adds a fork-by-fork precompile continuity matrix + activation boundary tests to lock expected precompile availability/variants.
AGENTS.md Extends standalone hardfork review guidance to require precompile continuity comparisons and referencing the new tests.
.claude/rules/hardfork-rollout.md Adds a Bor-specific VM hardfork failure mode checklist emphasizing precompile continuity and boundary assertions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread AGENTS.md Outdated
Comment thread .claude/rules/hardfork-rollout.md Outdated
@claude
Copy link
Copy Markdown

claude Bot commented May 19, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

@pratikspatil024
Copy link
Copy Markdown
Member Author

@claude review once

@claude
Copy link
Copy Markdown

claude Bot commented May 20, 2026

Claude finished @pratikspatil024's task in 2m 51s —— View job


Code Review: Hardfork Precompile Continuity Checks

  • Gather context and read changed files
  • Review core/vm/contracts_continuity_test.go
  • Review .claude/rules/hardfork-rollout.md changes
  • Review AGENTS.md changes
  • Cross-check precompile maps against production code
  • Post final review

Summary

Good PR — the test design is sound and the expectations are accurate. One documentation wording issue (also flagged by Copilot) is worth fixing.

Precompile Profile Verification

I manually cross-checked every test profile against the production precompile maps in core/vm/contracts.go:127-292. All six profiles match exactly:

Fork KZG (0x0a) P256 (0x0100) ModExp flags pip88 Production map matches?
Prague absent eip7951=false eip7823=false, eip7883=false false yes (L127-145)
Madhugiri present absent eip7823=true, eip7883=true false yes (L184-202)
MadhugiriPro present eip7951=false eip7823=true, eip7883=true false yes (L206-225)
Lisovo present eip7951=true eip7823=true, eip7883=true false yes (L229-248)
LisovoPro absent eip7951=true eip7823=true, eip7883=true false yes (L252-270)
Chicago absent eip7951=true eip7823=true, eip7883=true true yes (L274-292)

The precompile transition chain is correctly captured: Prague→Madhugiri drops P256 and adds KZG; MadhugiriPro re-adds P256; Lisovo upgrades P256 to eip7951; LisovoPro drops KZG; Chicago adds pip88 gas changes.

Test Design

The two-layer approach is well designed:

  1. Profile matrix (TestBorHardforkPrecompileContinuityProfiles) — tests the mapping from isolated fork flag → precompile set. Uses single-flag params.Rules{} structs, which correctly matches the activePrecompiledContracts switch-case ordering.

  2. Boundary tests (TestBorHardforkPrecompileContinuityAtNetworkBoundaries) — tests real activation blocks at N-1/N/N+1 using config.Rules(block, true, 0) with full multi-flag semantics. Correctly handles the mainnet same-block Madhugiri/MadhugiriPro and Lisovo/LisovoPro activations by only testing the Pro boundary there.

The describePrecompile type switch at contracts_continuity_test.go:285-328 is a good design choice — it will Fatalf on unhandled types, so adding a new precompile struct forces the matrix to be updated.

Finding: Documentation Wording (low severity)

Files: .claude/rules/hardfork-rollout.md:28, AGENTS.md:184

Both files say "P256 needed to be carried forward after Madhugiri" / "P256 must be carried forward after Madhugiri". This is ambiguous because the test matrix (and production code) shows P256 is absent in Madhugiri and first appears in MadhugiriPro. A reviewer reading this guidance could incorrectly expect P256 in Madhugiri.

Copilot flagged the same issue. Suggested rewording for both locations:

P256 was dropped when creating Madhugiri (from Prague) and re-added in MadhugiriPro; future forks must carry it forward from MadhugiriPro onward.

Fix this →

No Issues Found

  • No bugs in the test logic
  • No security concerns (test and documentation only, no consensus behavior changes)
  • Precompile profiles accurately reflect production code
  • Boundary tests correctly cover both Amoy (staggered activations) and mainnet (same-block activations)
  • CLAUDE.md compliance: test follows project patterns, no unnecessary comments, focused change

|

@sonarqubecloud
Copy link
Copy Markdown

@pratikspatil024
Copy link
Copy Markdown
Member Author

@claude Addressed the documentation wording finding in 9a75f5f39: the guidance now says P256 is carried forward from MadhugiriPro onward, and explicitly notes that mainnet activated Madhugiri and MadhugiriPro at the same block so only the MadhugiriPro boundary is externally observable there.

@claude
Copy link
Copy Markdown

claude Bot commented May 20, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

@claude
Copy link
Copy Markdown

claude Bot commented May 20, 2026

Claude finished @pratikspatil024's task in 30s —— View job


Verifying Documentation Wording Fix

  • Understand the request — confirm fix for P256/MadhugiriPro wording
  • Verify .claude/rules/hardfork-rollout.md wording
  • Verify AGENTS.md wording
  • Confirm fix addresses the finding

Verified commit 9a75f5f. Both files now correctly say "P256 … carried forward from MadhugiriPro onward" (instead of the previous "after Madhugiri"), and both include the mainnet same-block activation nuance. The wording matches the production precompile maps and the test matrix in contracts_continuity_test.go. The previous review finding is fully resolved.

@pratikspatil024 pratikspatil024 merged commit 2b6be4c into develop May 20, 2026
27 of 28 checks passed
@pratikspatil024 pratikspatil024 deleted the psp-pos-3598-precompile-continuity branch May 20, 2026 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants