Skip to content

feat(harness): Add ForkMatrix for Parametrized Hardfork#1468

Draft
refcell wants to merge 1 commit intomainfrom
feat/fork-matrix
Draft

feat(harness): Add ForkMatrix for Parametrized Hardfork#1468
refcell wants to merge 1 commit intomainfrom
feat/fork-matrix

Conversation

@refcell
Copy link
Contributor

@refcell refcell commented Mar 18, 2026

Summary

Replaces #1462.

Adds ForkMatrix, a concise test helper for running the same assertion across every hardfork stage without hand-writing cumulative HardForkConfig structs.

A single FORK_PROGRESSION static drives all(): each entry is one fn(&mut HardForkConfig) setter; build() applies them in sequence to produce a cumulative snapshot after each step. Adding a new fork is a one-line change to the table. The filtered constructors (pre_isthmus, from_isthmus, from_granite) all derive from all() or its subset with a single retain predicate, so they update automatically.

Migrates the two operator-fee tests that previously fixed a single fork to run across the full relevant slice of the matrix.

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Mar 18, 2026

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@vercel
Copy link

vercel bot commented Mar 18, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
base Ignored Ignored Preview Mar 19, 2026 2:37pm

Request Review

@refcell refcell mentioned this pull request Mar 18, 2026
Comment on lines +65 to +83
pub fn from_granite() -> Self {
static PROGRESSION: &[(&str, ForkSetter)] = &[
("granite", |h| h.granite_time = Some(0)),
("holocene", |h| h.holocene_time = Some(0)),
("isthmus", |h| h.isthmus_time = Some(0)),
("jovian", |h| h.jovian_time = Some(0)),
];
Self::build(
PROGRESSION,
HardForkConfig {
regolith_time: Some(0),
canyon_time: Some(0),
delta_time: Some(0),
ecotone_time: Some(0),
fjord_time: Some(0),
..Default::default()
},
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

from_granite duplicates fork setters and the pre-Granite base config that already exist in FORK_PROGRESSION. If a fork setter changes (e.g., granite needs to set an additional field), or if a new fork is inserted between fjord and granite, this method silently diverges from the canonical progression.

Consider building from FORK_PROGRESSION directly to keep a single source of truth. For example, building from the full progression and then stripping pectra-blob-schedule and base-specific entries from the configs (not just filtering names):

pub fn from_granite() -> Self {
    let base = Self::build(
        &FORK_PROGRESSION[..5], // regolith..fjord
        HardForkConfig::default(),
    );
    let last_config = base.forks.last().map(|(_, c)| *c).unwrap_or_default();
    
    static OP_ONLY: &[(&str, ForkSetter)] = &[
        ("granite", |h| h.granite_time = Some(0)),
        ("holocene", |h| h.holocene_time = Some(0)),
        ("isthmus", |h| h.isthmus_time = Some(0)),
        ("jovian", |h| h.jovian_time = Some(0)),
    ];
    Self::build(OP_ONLY, last_config)
}

This still duplicates the post-granite setters (intentionally, since pectra-blob-schedule is excluded from the progression), but at least the pre-granite base is derived from the single source of truth.

Not blocking — the existing unit tests catch most divergences — but worth considering as the fork table grows.

Comment on lines +99 to +109
/// Runs a test closure once per configured fork, annotating any panic with the fork name.
pub fn run<F>(&self, mut test: F)
where
F: FnMut(&'static str, HardForkConfig),
{
for (name, config) in self.iter() {
if let Err(e) = panic::catch_unwind(AssertUnwindSafe(|| test(name, config))) {
Self::panic_with_fork_context(name, e);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

run stops at the first failing fork and re-panics with a new message, which discards the original panic's backtrace. When a test fails inside run, the developer sees the fork name and assertion message but loses the source location of the actual assert! that fired.

Two options to consider:

  1. Collect all failures and report them together, so one test run surfaces every broken fork instead of requiring fix-rerun cycles:
pub fn run<F>(&self, mut test: F)
where
    F: FnMut(&'static str, HardForkConfig),
{
    let mut failures = Vec::new();
    for (name, config) in self.iter() {
        if let Err(e) = panic::catch_unwind(AssertUnwindSafe(|| test(name, config))) {
            failures.push((name, e));
        }
    }
    if !failures.is_empty() {
        let msgs: Vec<_> = failures.iter().map(|(name, e)| {
            let msg = e.downcast_ref::<String>().map(String::as_str)
                .or_else(|| e.downcast_ref::<&str>().copied())
                .unwrap_or("non-string panic payload");
            format!("  `{name}`: {msg}")
        }).collect();
        panic!("fork matrix failures:\n{}", msgs.join("\n"));
    }
}
  1. Use resume_unwind instead of panic! to preserve the original backtrace (at the cost of losing the fork-name annotation in the panic message):
std::panic::resume_unwind(e);

The current fail-fast approach is a valid choice for a test helper — just noting the trade-off.

Comment on lines +113 to +116
let expected_format = matches!(
(fork_name, &l1_info),
("isthmus", L1BlockInfoTx::Isthmus(_)) | ("jovian", L1BlockInfoTx::Jovian(_))
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This matches! must be extended every time a new post-Isthmus OP fork is added. If a new fork (e.g., after Jovian) is added to FORK_PROGRESSION and included by from_isthmus(), this arm silently returns false and the test fails with a message that doesn't explain why the format is unexpected — only that it is.

Consider inverting the assertion to check that the format is not Ecotone (the pre-Isthmus format), which is the actual invariant being tested and doesn't require updating when new forks are added:

assert!(
    !matches!(l1_info, L1BlockInfoTx::Ecotone(_)),
    "{fork_name}: post-Isthmus L1 info must not use Ecotone format, got {l1_info:?}"
);

This asserts what the test actually cares about (operator fees are encoded, i.e., not Ecotone) without coupling to the exhaustive list of post-Isthmus variants.

@refcell refcell self-assigned this Mar 18, 2026
@refcell refcell added K-feature Kind: New feature A-actions Area: actions testing labels Mar 18, 2026
@refcell refcell marked this pull request as ready for review March 18, 2026 19:10
@refcell refcell enabled auto-merge March 18, 2026 19:11
Adds `ForkMatrix`, a concise test helper for running the same assertion
across every hardfork stage without hand-writing cumulative `HardForkConfig`
structs.

A single `FORK_PROGRESSION` static drives `all()`: each entry is one
`fn(&mut HardForkConfig)` setter; `build()` applies them in sequence to
produce a cumulative snapshot after each step. Adding a new fork is a
one-line change to the table. The filtered constructors (`pre_isthmus`,
`from_isthmus`, `from_granite`) all derive from `all()` or its subset
with a single `retain` predicate, so they update automatically.

Migrates the two operator-fee tests that previously fixed a single fork
to run across the full relevant slice of the matrix.

Co-authored-by: Emil Sørensen <esorense@users.noreply.github.com>
("pectra-blob-schedule", |h| h.pectra_blob_schedule_time = Some(0)),
("isthmus", |h| h.isthmus_time = Some(0)),
("jovian", |h| h.jovian_time = Some(0)),
("base-v1", |h| h.base.get_or_insert_with(BaseHardforkConfig::default).v1 = Some(0)),
Copy link
Contributor

Choose a reason for hiding this comment

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

HardForkConfig::base is typed BaseHardforkConfig (not Option<BaseHardforkConfig>), so get_or_insert_with won't compile — it's an Option method.

Same issue on line 57 where from_isthmus() calls h.base.is_none().

Either:

  • This PR depends on a separate change that wraps the field in Option, in which case that dependency should be noted in the PR description and merged first, or
  • The setter needs to assign directly:
("base-v1", |h| h.base = BaseHardforkConfig { v1: Some(0) }),

and the from_isthmus filter needs BaseHardforkConfig::is_empty:

Self::all().retain(|_, h| h.isthmus_time.is_some() && h.base.is_empty())

@github-actions
Copy link
Contributor

Review Summary

Blocking

Type mismatch: HardForkConfig::base is BaseHardforkConfig, not Option<BaseHardforkConfig>FORK_PROGRESSION line 27 calls get_or_insert_with() and from_isthmus() line 57 calls is_none() on the base field, but both are Option methods unavailable on the concrete BaseHardforkConfig type. This will fail to compile against the current main. If this PR depends on a separate change that wraps the field in Option, that dependency should be documented and merged first.

Non-blocking (from prior review, preserved for context)

  1. from_granite duplication (matrix.rs:65-83) — The method duplicates fork setters and the pre-Granite base config from FORK_PROGRESSION. If a setter changes, from_granite silently diverges. Unit tests mitigate the risk but the duplication is worth reducing as the fork table grows.

  2. run discards backtraces (matrix.rs:100-108) — catch_unwind + panic! annotates the fork name but loses the source location of the original assertion. Consider resume_unwind for backtrace preservation, or collecting all failures to surface every broken fork in one test run.

  3. matches! coupling in test (operator_fees.rs:113-116) — The expected_format check must be extended for every new post-Isthmus fork. Inverting to !matches!(l1_info, L1BlockInfoTx::Ecotone(_)) asserts the actual invariant (operator fees are encoded) without coupling to the exhaustive variant list.

Overall

The ForkMatrix design is clean — cumulative snapshots from a single progression table, builder-pattern retain, and run with fork-name-annotated panics. The test migration is a clear improvement. The type mismatch on the base field needs resolution before merge.

@refcell refcell marked this pull request as draft March 19, 2026 14:53
auto-merge was automatically disabled March 19, 2026 14:53

Pull request was converted to draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-actions Area: actions testing K-feature Kind: New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants