Skip to content
This repository was archived by the owner on Jan 16, 2026. It is now read-only.

Comments

chore(node/runtime): remove remaining runtime references in kona#2566

Merged
refcell merged 1 commit intomainfrom
theo/remove-runtime-loader
Aug 12, 2025
Merged

chore(node/runtime): remove remaining runtime references in kona#2566
refcell merged 1 commit intomainfrom
theo/remove-runtime-loader

Conversation

@theochap
Copy link
Member

Description

It seems the AI has missed some references in #2535. Removes the remaining runtime libraries in the node.

Copilot AI review requested due to automatic review settings July 28, 2025 14:40
Copy link
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

This PR removes the remaining runtime-related code from the kona node sources, completing a cleanup effort that was apparently started in a previous PR. The changes eliminate the entire runtime module and its dependencies.

  • Removes the runtime module including RuntimeLoader, RuntimeConfig, and RuntimeLoaderError
  • Cleans up related dependencies and exports from the crate
  • Updates feature flags to remove runtime-specific dependencies

Reviewed Changes

Copilot reviewed 6 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
crates/node/sources/src/runtime/mod.rs Removes the runtime module declaration and exports
crates/node/sources/src/runtime/loader.rs Removes the RuntimeLoader implementation with L1 provider integration
crates/node/sources/src/runtime/error.rs Removes the RuntimeLoaderError type definitions
crates/node/sources/src/runtime/config.rs Removes the RuntimeConfig struct and Display implementation
crates/node/sources/src/lib.rs Removes runtime module imports and re-exports
crates/node/sources/Cargo.toml Removes unused dependencies and updates feature flags

@theochap theochap self-assigned this Jul 28, 2025
@theochap theochap added K-chore Kind: chore A-node Area: cl node (eq. Go op-node) handles single-chain consensus labels Jul 28, 2025
@theochap theochap moved this to In Review in Project Tracking Jul 28, 2025
@theochap theochap added this to the [kona-node] Phase 5: Alpha milestone Jul 28, 2025
@codecov
Copy link

codecov bot commented Jul 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.4%. Comparing base (2515144) to head (0fed833).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@refcell refcell left a comment

Choose a reason for hiding this comment

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

Yea this was separate from the RuntimeActor which it removed there - that's why this wasn't removed

@refcell refcell enabled auto-merge July 29, 2025 13:38
Comment on lines 325 to 336
// First attempt to load the unsafe block signer from the runtime loader.
if let Some(url) = l1_rpc {
let mut loader = RuntimeLoader::new(url, Arc::new(config.clone()));
let runtime = loader.load_latest().await.map_err(|e| {
anyhow::anyhow!("Failed to load runtime: {} {:?}", e, std::error::Error::source(&e))
})?;
return Ok(runtime.unsafe_block_signer_address);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This we can't do unfortunately. We must load the current value at startup. Could be done with a much-simplified RuntimeLoader though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh shoot that's right

@theochap theochap removed this from the [kona-node] Phase 5 - Arid Release milestone Jul 29, 2025
@theochap theochap force-pushed the theo/remove-runtime-loader branch from dc798bb to c3d74b0 Compare August 12, 2025 21:04
@claude
Copy link
Contributor

claude bot commented Aug 12, 2025

Claude finished @theochap's task —— View job


Pull Request Review

I've completed a thorough review of this PR that removes the remaining runtime references from the kona node. Here's my analysis:

Todo List:

  • Read and analyze deleted runtime module files
  • Review changes to node command files
  • Check dependency changes in Cargo.toml
  • Analyze the concern raised about runtime loading at startup
  • Provide comprehensive review feedback

Summary

This PR removes the entire runtime module from kona-sources and its usage throughout the node codebase. The runtime module provided functionality to dynamically load configuration values (particularly the unsafe block signer) from the L1 chain at startup.

Code Quality & Architecture

✅ Positive aspects:

  • Clean removal of unused code - the changes are systematic and consistent
  • Proper dependency cleanup in Cargo.toml
  • No broken imports or compilation issues
  • Maintains existing API contracts where possible

⚠️ Areas of concern:

  • The removed RuntimeLoader provided important functionality for fetching current chain state
  • Loss of dynamic configuration loading capability

Critical Issue: Runtime Configuration Loading

🚨 Major Concern - There's a fundamental issue with removing the runtime loader:

What was removed:

  • RuntimeLoader that fetched the current unsafe block signer from L1 storage at startup (bin/node/src/flags/p2p.rs:336)
  • Dynamic loading of protocol versions from the ProtocolVersions contract
  • L1 chain state validation for configuration values

Impact:
The code now falls back to static configuration values from the superchain registry instead of fetching current values from L1 storage. This means:

  1. Stale Configuration Risk: The unsafe block signer could be outdated if it was recently updated on L1
  2. Protocol Version Mismatch: Required/recommended protocol versions may not reflect current L1 state
  3. Security Implications: Using an outdated unsafe block signer could lead to accepting invalid blocks

Reviewer's Valid Concern:
@clabby's comment is correct: "This we can't do unfortunately. We must load the current value at startup."

Recommendations

🔧 Suggested Approach:

  1. Implement a simplified RuntimeLoader as @clabby suggested, that only loads essential startup values
  2. Preserve critical functionality while removing unused components
  3. Consider making it optional with fallback to registry values

Alternative minimal implementation:

// Keep only essential loading functionality
pub struct MinimalRuntimeLoader {
    provider: AlloyChainProvider,
    config: Arc<RollupConfig>,
}

impl MinimalRuntimeLoader {
    pub async fn load_unsafe_block_signer(&self) -> Result<Address, RuntimeLoaderError> {
        // Only load the unsafe block signer at startup
        // Remove caching, metrics, and other complexity
    }
}

Performance & Security

Performance: ✅ The removal improves startup time and reduces complexity
Security: ⚠️ Potential security risk from using stale configuration values

Test Coverage

Issue: The removal eliminates test coverage for runtime loading functionality. While the online test was removed, there's no replacement verification that configuration loading works correctly.

Final Recommendation

❌ Request Changes - This PR should not be merged as-is because:

  1. Critical functionality loss: Dynamic configuration loading is essential for production deployments
  2. Security risk: Stale unsafe block signer could compromise block validation
  3. Incomplete solution: The issue raised in review comments hasn't been addressed

Suggested path forward:

  1. Implement a minimal runtime loader that preserves essential startup configuration loading
  2. Remove only the unused/complex parts of the original runtime module
  3. Ensure the unsafe block signer is always current at startup

The cleanup goal is good, but we need to preserve the essential runtime configuration loading capability.

@theochap theochap force-pushed the theo/remove-runtime-loader branch from c3d74b0 to 3bbc9ea Compare August 12, 2025 21:05
@claude
Copy link
Contributor

claude bot commented Aug 12, 2025

Claude finished @theochap's task —— View job


Pull Request Review

I've completed my review of this PR that removes remaining runtime references from the kona node. Here are my findings:

Overall Assessment

This PR successfully removes most runtime-related code from the node sources, cleaning up the codebase by eliminating the runtime module and its dependencies. The changes are well-focused and align with the cleanup effort mentioned in the description.

🚨 Critical Issues Found

1. Unused Import (Line 18)

  • bin/node/src/flags/p2p.rs:18 still imports RuntimeLoader from kona_sources, but this type no longer exists
  • This will cause a compilation error: use kona_sources::RuntimeLoader;
  • Fix: Remove this import line

2. Missing Functionality for Unsafe Block Signer
The PR removes runtime loading functionality from unsafe_block_signer() method, but as noted in @clabby's review comment, this creates a significant issue:

  • Previous behavior: The function would attempt to load the current unsafe block signer from the L1 chain via RuntimeLoader at startup
  • Current behavior: Falls back to genesis signer or CLI-provided signer only
  • Problem: This removes the ability to get the current unsafe block signer from the chain state

Impact: This could cause nodes to reject valid unsafe blocks if the unsafe block signer has been updated on-chain but the node is using an outdated value from genesis/CLI.

📋 Code Quality Issues

3. Function Signature Inconsistencies
The unsafe_block_signer() and config() methods had their signatures simplified to remove the l1_rpc parameter, but the calls to these methods appear to match the new signatures, so this seems correct.

4. Dependency Cleanup
Good: Cargo.toml properly removes unused dependencies:

  • alloy-rpc-types-engine
  • op-alloy-rpc-types-engine
  • alloy-transport-http features
  • url dependency

🔧 Recommendations

  1. Immediate Fix: Remove the unused RuntimeLoader import to fix compilation

  2. Architecture Decision Needed: The team needs to decide how to handle unsafe block signer updates:

    • Option A: Implement a simplified runtime loader as suggested by @clabby
    • Option B: Accept that nodes must be restarted when unsafe block signer changes
    • Option C: Use a different mechanism to fetch current signer (e.g., direct L1 query)
  3. Testing: Verify that nodes can still properly validate unsafe blocks with the current approach

🛡️ Security Considerations

  • Removing runtime loading could impact network security if nodes use outdated unsafe block signers
  • This change should be coordinated across the network to avoid consensus issues

Performance Impact

Positive: Removing runtime loading reduces startup complexity and dependencies

🧪 Test Coverage

The existing tests appear adequate for the remaining functionality, though testing the unsafe block signer fallback behavior would be valuable.


Summary: This PR needs the import fix and a decision on how to handle dynamic unsafe block signer loading before it can be safely merged.

@theochap theochap force-pushed the theo/remove-runtime-loader branch from 3bbc9ea to 0fed833 Compare August 12, 2025 21:21
@claude
Copy link
Contributor

claude bot commented Aug 12, 2025

Claude finished @theochap's task —— View job


Pull Request Review

I've completed a thorough review of this PR that removes the remaining runtime references from the kona node. Here's my analysis:

Todo List:

  • Read and analyze deleted runtime module files
  • Review changes to node command files
  • Check dependency changes in Cargo.toml
  • Analyze the concern raised about runtime loading at startup
  • Provide comprehensive review feedback

Summary

This PR removes the entire runtime module from kona-sources and its usage throughout the node codebase. The runtime module provided functionality to dynamically load configuration values (particularly the unsafe block signer) from the L1 chain at startup.

Code Quality & Architecture

✅ Positive aspects:

  • Clean removal of unused code - the changes are systematic and consistent
  • Proper dependency cleanup in Cargo.toml
  • No broken imports or compilation issues
  • Maintains existing API contracts where possible

⚠️ Areas of concern:

  • The removed RuntimeLoader provided important functionality for fetching current chain state
  • Loss of dynamic configuration loading capability

Critical Issue: Runtime Configuration Loading

🚨 Major Concern - There's a fundamental issue with removing the runtime loader:

What was removed:

  • RuntimeLoader that fetched the current unsafe block signer from L1 storage at startup (bin/node/src/flags/p2p.rs:336)
  • Dynamic loading of protocol versions from the ProtocolVersions contract
  • L1 chain state validation for configuration values

Impact:
The code now falls back to static configuration values from the superchain registry instead of fetching current values from L1 storage. This means:

  1. Stale Configuration Risk: The unsafe block signer could be outdated if it was recently updated on L1
  2. Protocol Version Mismatch: Required/recommended protocol versions may not reflect current L1 state
  3. Security Implications: Using an outdated unsafe block signer could lead to accepting invalid blocks

Reviewer's Valid Concern:
@clabby's comment is correct: "This we can't do unfortunately. We must load the current value at startup."

Recommendations

🔧 Suggested Approach:

  1. Implement a simplified RuntimeLoader as @clabby suggested, that only loads essential startup values
  2. Preserve critical functionality while removing unused components
  3. Consider making it optional with fallback to registry values

Alternative minimal implementation:

// Keep only essential loading functionality
pub struct MinimalRuntimeLoader {
    provider: AlloyChainProvider,
    config: Arc<RollupConfig>,
}

impl MinimalRuntimeLoader {
    pub async fn load_unsafe_block_signer(&self) -> Result<Address, RuntimeLoaderError> {
        // Only load the unsafe block signer at startup
        // Remove caching, metrics, and other complexity
    }
}

Performance & Security

Performance: ✅ The removal improves startup time and reduces complexity
Security: ⚠️ Potential security risk from using stale configuration values

Test Coverage

Issue: The removal eliminates test coverage for runtime loading functionality. While the online test was removed, there's no replacement verification that configuration loading works correctly.

Final Recommendation

❌ Request Changes - This PR should not be merged as-is because:

  1. Critical functionality loss: Dynamic configuration loading is essential for production deployments
  2. Security risk: Stale unsafe block signer could compromise block validation
  3. Incomplete solution: The issue raised in review comments hasn't been addressed

Suggested path forward:

  1. Implement a minimal runtime loader that preserves essential startup configuration loading
  2. Remove only the unused/complex parts of the original runtime module
  3. Ensure the unsafe block signer is always current at startup

The cleanup goal is good, but we need to preserve the essential runtime configuration loading capability.

@refcell refcell added this pull request to the merge queue Aug 12, 2025
Merged via the queue into main with commit 185ed54 Aug 12, 2025
32 of 33 checks passed
@refcell refcell deleted the theo/remove-runtime-loader branch August 12, 2025 21:54
@github-project-automation github-project-automation bot moved this from In Review to Done in Project Tracking Aug 12, 2025
theochap added a commit to ethereum-optimism/optimism that referenced this pull request Dec 10, 2025
…rs/kona#2566)

## Description

It seems the AI has missed some references in
op-rs/kona#2535. Removes the remaining runtime
libraries in the node.
theochap added a commit to ethereum-optimism/optimism that referenced this pull request Jan 14, 2026
…rs/kona#2566)

## Description

It seems the AI has missed some references in
op-rs/kona#2535. Removes the remaining runtime
libraries in the node.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A-node Area: cl node (eq. Go op-node) handles single-chain consensus K-chore Kind: chore

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants