Skip to content

chore(upgrade-signal): read new ProtocolVersions schedule contract#3963

Open
PelleKrab wants to merge 5 commits into
mainfrom
pellekrab/upgrade-signal-protocol-versions
Open

chore(upgrade-signal): read new ProtocolVersions schedule contract#3963
PelleKrab wants to merge 5 commits into
mainfrom
pellekrab/upgrade-signal-protocol-versions

Conversation

@PelleKrab

Copy link
Copy Markdown
Contributor

Summary

Adds support for reading dynamic upgrade activation schedules from the L1 ProtocolVersions contract.

This updates the upgrade-signal utility to:

  • read the contract-backed upgrade schedule with getSchedule()
  • read the global minimumProtocolVersion()
  • map contract schedule entries onto known BaseUpgrade::CONTRACT_VARIANTS
  • reject schedules requiring a newer node protocol version than the binary supports
  • derive the node protocol version from the Cargo/GitHub release version
  • allow unreleased 0.0.0 dev builds to bypass the minimum-version rejection with U256::MAX

The Solidity ABI in this crate intentionally includes only the read surface used by nodes, not the
full contract interface.

Notes

CONTRACT_VARIANTS remains explicit because it is not the same as all BaseUpgrade variants. It
represents the contract-backed upgrade set in L1 schedule order and intentionally excludes non-
contract-backed upgrades like Bedrock and Zombie.

Tests

cargo fmt --check -p base-upgrade-signal
cargo test -p base-upgrade-signal --all-features
cargo clippy -p base-upgrade-signal --all-targets --all-features -- -D warnings

@cb-heimdall

cb-heimdall commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/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

@PelleKrab PelleKrab force-pushed the pellekrab/upgrade-signal-protocol-versions branch 2 times, most recently from 8b0a575 to b8b8020 Compare July 14, 2026 23:10
Comment thread crates/utilities/upgrade-signal/src/metrics.rs
Comment thread crates/utilities/upgrade-signal/src/config/mod.rs Outdated
Comment thread crates/utilities/upgrade-signal/src/contract.rs Outdated
PelleKrab and others added 4 commits July 15, 2026 09:28
Replace the per-upgrade IUpgradeSignal reads (getTimestamp/getProtocolVersion
by name) with the new ProtocolVersions interface from base/contracts#353:

- read the full id-ordered schedule with one getSchedule() call and the
  global minimumProtocolVersion() in a single pinned L1 block read
- map schedule entries onto the node hardfork ladder in activation
  (timestamp) order: id 0 (lowest timestamp) aligns with the oldest
  contract-backed hardfork; entries newer than the known ladder are
  logged and ignored, and unregistered hardforks produce no signal
- attach the global minimum protocol version to every mapped signal so the
  existing validation and sink pipeline is unchanged
- bump the supported node protocol version from 7 to packed semver 1.1.0
- drop the now-impossible TimestampOverflow error (timestamps are uint64
  onchain)
Co-authored-by: Codex <codex-noreply@coinbase.com>
Cache node_protocol_version() in a LazyLock and add tests for the
0.0.0 -> U256::MAX dev-build invariant. Note the packed-semver input
assumption on protocol_version_to_f64.

Generated with Claude Code

Co-Authored-By: Claude <noreply@anthropic.com>
The reader only issues eth_calls for getSchedule and minimumProtocolVersion;
nothing decodes TimestampSet or MinimumProtocolVersionUpdated. Remove them and
re-add if event-based reads are introduced later.

Generated with Claude Code

Co-Authored-By: Claude <noreply@anthropic.com>
@PelleKrab PelleKrab force-pushed the pellekrab/upgrade-signal-protocol-versions branch from 93c2d9d to 6581bce Compare July 15, 2026 16:43
Comment thread crates/utilities/upgrade-signal/src/contract.rs
…sion

- Document that map_schedule aligns entries by registration id (positional),
  not by activation timestamp; timestamps need not be monotonic (README + doc).
- Cache the Cargo-derived node protocol version in a LazyLock instead of
  recomputing on every call.

Generated with Claude Code

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Review Summary

The refactor from per-upgrade getTimestamp/getProtocolVersion calls to the new batch getSchedule() + global minimumProtocolVersion() contract interface is clean and well-structured.

What looks good:

  • The packed_protocol_version bit layout correctly matches the contract's major << 96 | minor << 64 | patch << 32 encoding, and protocol_version_to_f64 correctly inverts it
  • LazyLock for NODE_PROTOCOL_VERSION is the right pattern — computes once, expresses compile-time-fixed intent
  • The 0.0.0 → U256::MAX dev-build escape hatch is well-tested with both the zero→MAX and nonzero→identity cases
  • map_schedule positional zip is correct and handles all edge cases: partial schedules, excess contract entries, filtered upgrade IDs, empty timestamps
  • Error metric recording is no longer duplicated — read_schedule_tolerant delegates to read_schedule which records exactly once, then just adds the warning log
  • Removal of TimestampOverflow variant is justified since getSchedule returns uint64[] natively

No new findings. The inline comments from the previous review run cover the remaining minor items (metrics gauge behavior with U256::MAX, doc wording on positional mapping).

@github-actions

Copy link
Copy Markdown
Contributor

❌ base-std fork tests did not run

The build or setup step failed before any tests could execute. Check the workflow logs for details.

Dependency Ref Commit
base-std main 4658f1b7
base-anvil 0092692587d8d064dd2c6923ce26a682c58f3694 00926925

@jackchuma jackchuma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good - just one optional nit to address

Comment on lines +39 to +66
pub fn node_protocol_version() -> U256 {
*NODE_PROTOCOL_VERSION
}

/// Encodes a `major.minor.patch` version into the packed-semver `uint256` layout used by the
/// L1 `ProtocolVersions` contract: `major << 96 | minor << 64 | patch << 32`, with the
/// prerelease field left zero.
pub const fn packed_protocol_version(major: u32, minor: u32, patch: u32) -> U256 {
U256::from_limbs([(patch as u64) << 32, ((major as u64) << 32) | minor as u64, 0, 0])
}

/// Maps a packed Cargo version to the advertised node protocol version, promoting the
/// dev-build `0.0.0` (zero) to `U256::MAX` so no contract minimum can reject a dev build.
pub fn advertised_protocol_version(cargo_version: U256) -> U256 {
if cargo_version == U256::ZERO { U256::MAX } else { cargo_version }
}
}

/// Node protocol version derived from the compile-time Cargo package version, computed once.
static NODE_PROTOCOL_VERSION: LazyLock<U256> = LazyLock::new(|| {
UpgradeSignalDefaults::advertised_protocol_version(
UpgradeSignalDefaults::packed_protocol_version(
env!("CARGO_PKG_VERSION_MAJOR").parse::<u32>().expect("Cargo package major is numeric"),
env!("CARGO_PKG_VERSION_MINOR").parse::<u32>().expect("Cargo package minor is numeric"),
env!("CARGO_PKG_VERSION_PATCH").parse::<u32>().expect("Cargo package patch is numeric"),
),
)
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: LazyLock looks unnecessary here. CARGO_PKG_VERSION_* values are compile-time strings, and node_protocol_version() is only used while building upgrade-signal configuration, so caching the three parses adds global state without a meaningful payoff. Consider parsing and packing directly in node_protocol_version() instead; that removes the static and LazyLock import with no behavior change

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.

3 participants