fix(l1): bal-devnet-2 fixes

[edg-l]

Summary

Fixes for running ethrex in bal-devnet-2 (Gloas/Amsterdam fork):

• Add engine_getPayloadV6 + engine_newPayloadV5 capability
• Fix EIP-7778 receipt gas tracking in block building: use post-refund gas_spent for receipt cumulative_gas_used, pre-refund gas_used for block gas accounting. Block re-execution already handled this correctly.
• Fix engine_getClientVersionV1 commit hash (use full SHA from build.rs)
• Add bal-devnet-2 kurtosis fixture with --syncmode=full workaround
• Update ethereum-package revision

Test plan

• make -C tooling/ef_tests/blockchain test passes (6158/6158)
• ethrex proposes and validates blocks post-gloas in bal-devnet-2 kurtosis network

[github-actions]

🤖 Kimi Code Review

Review Summary

This PR introduces Amsterdam fork support and related testing infrastructure. Here are the key findings:

Critical Issues

1. Potential Fork Confusion in genesis.rs (lines 463-497): The blob schedule logic appears inverted. The comment says "Amsterdam implies BPO2 blob params" but the code returns BPO2 for Amsterdam only when Amsterdam's schedule is NOT explicitly set. This seems backwards.

2. SELFDESTRUCT Gas Check Logic (lines 611-670): The two-phase gas check for Amsterdam has a potential issue - it checks base cost availability but doesn't actually charge it before proceeding to the full cost calculation. This could lead to gas accounting inconsistencies.

Security Concerns

1. Blobs Bundle Validation (lines 546-588): The test transaction_with_version_0_blobs_should_fail_on_amsterdam has incorrect logic - it claims Amsterdam comes after Osaka, but Amsterdam is actually before Osaka in the fork timeline. This could lead to incorrect blob validation.

Performance & Correctness

1. BlockAccessListRecorder Changes (lines 724-745): The change from in-place updates to always pushing new entries is correct for checkpoint/restore semantics but could impact performance due to increased memory usage. Consider adding a comment about potential memory implications.

2. u64 Deserialization (lines 233-249): The new deserialization logic handles both string and number formats correctly, which is good for compatibility.

Minor Issues

1. Typo in workflow (line 18): "clakiness" should be "flakiness"
2. Redundant condition (line 494): The Amsterdam check is redundant since it's already handled in the Amsterdam-specific branch above

Recommendations

1. Fix the fork ordering in the blobs test
2. Clarify the blob schedule inheritance logic in genesis.rs
3. Consider adding a memory usage warning for the BlockAccessListRecorder changes
4. Fix the typo in the workflow file

The PR appears to be well-structured overall, with good separation of concerns between the testing infrastructure and core logic changes.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Findings (ordered by severity)

1. Potential deserialization breakage in non-JSON formats
   In crates/common/serde_utils.rs (module u64::option_from_prefixed_hex), the change to Option<serde_json::Value> makes this deserializer JSON-specific. If this helper is used with non-JSON formats (e.g., RLP, CBOR, TOML, or custom serde backends), it will fail to deserialize because serde_json::Value is not supported by those formats.
   Suggestion: use an untagged enum that is format-agnostic. Example pattern:

   • #[derive(Deserialize)] #[serde(untagged)] enum U64Repr { Str(String), Num(u64) }
   • Then Option<U64Repr> and handle both cases.
     File: crates/common/serde_utils.rs (around option_from_prefixed_hex).

2. Possible semantic mismatch for block-level gas overflow
   In crates/vm/backends/levm/mod.rs, when record_bal is true, block gas overflow returns EvmError::Transaction(...). If the caller treats Transaction errors as transaction-local (vs block-invalid), this could mask a block-level error path.
   Suggestion: confirm how EvmError::Transaction is interpreted upstream for block validation. If block-level errors are expected to be distinct, consider a dedicated error variant or a block-level error type.
   File: crates/vm/backends/levm/mod.rs (two identical blocks around the pre-refund gas-limit check).

Notes / Minor

• crates/vm/levm/src/opcode_handlers/system.rs: The two-phase gas check for SELFDESTRUCT looks correct for EIP-7928 semantics and balances recording order. No immediate issues found.
• crates/common/types/block_access_list.rs: Always-push + dedup approach is consistent with the checkpoint/restore explanation. The BTreeMap dedup keeps ordering by block_access_index, which seems fine, but it is extra work per slot; acceptable unless profiling shows hot-path regression.

If you want, I can check upstream error handling for EvmError::Transaction to validate Item 2.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Now I have a comprehensive understanding of the changes. Let me compile my review.

---

PR Review: bal-devnet-2 Tracking Branch (Amsterdam Fork Support)

Overview

This PR adds Amsterdam fork support and updates Hive testing infrastructure. The changes span CI configuration, EVM execution logic (EIP-7928 BAL tracking), and documentation. This is a draft tracking branch with 651 additions and 92 deletions across 14 files.

Critical Issues

1. Type Coercion Safety Issue in serde_utils.rs

File: crates/common/serde_utils.rs:203-222

The deserialization now accepts both string and number formats for Option<u64>:

let value: Option<serde_json::Value> = Option::deserialize(d)?;
match value {
    Some(serde_json::Value::String(s)) if !s.is_empty() => { ... }
    Some(serde_json::Value::Number(n)) => n
        .as_u64()
        .ok_or_else(|| D::Error::custom("Failed to deserialize u64 value"))
        .map(Some),
    _ => Ok(None),
}

Issue: as_u64() silently fails for negative numbers and floats. For blockchain operations, this could silently accept invalid data.

Recommendation: Add explicit validation to reject negative numbers and floats:

Some(serde_json::Value::Number(n)) => {
    if !n.is_u64() {
        return Err(D::Error::custom("Value must be a non-negative integer"));
    }
    n.as_u64()
        .ok_or_else(|| D::Error::custom("Failed to deserialize u64 value"))
        .map(Some)
}

2. Potential Integer Overflow in BAL Index

File: crates/vm/backends/levm/mod.rs:110-111

#[allow(clippy::cast_possible_truncation)]
db.set_bal_index((tx_idx + 1) as u16);

Issue: If a block contains more than 65,535 transactions, this cast will silently truncate. While unlikely in practice, the EIP-7928 spec defines block_access_index as uint16, so this is a theoretical constraint violation.

Recommendation: Add an explicit check:

let bal_index = u16::try_from(tx_idx + 1)
    .map_err(|_| EvmError::Transaction("Transaction index exceeds uint16 maximum".into()))?;
db.set_bal_index(bal_index);

3. Blob Version Validation Logic May Be Incomplete

File: crates/common/types/blobs_bundle.rs:232-268

The test transaction_with_version_0_blobs_should_fail_on_amsterdam validates that version 0 blobs are rejected on Amsterdam, but the actual validation logic isn't shown in this diff.

Recommendation: Ensure that:

• Version 0 blobs are only valid on Osaka fork
• Version 1+ blobs are validated correctly for Amsterdam
• The validation happens before gas is consumed

Amsterdam Fork (EIP-7928) Implementation Review

4. SELFDESTRUCT Two-Phase Gas Check ✓

File: crates/vm/levm/src/opcode_handlers/system.rs:614-647

The implementation correctly implements EIP-7928's two-phase gas accounting:

1. Phase 1: Check base cost (SELFDESTRUCT_STATIC + cold access) without charging
2. Record BAL state between phases
3. Phase 2: Charge full cost including NEW_ACCOUNT

Concerns:

• The implementation is fork-gated (Fork::Amsterdam), which is correct.
• However, there's code duplication between the Amsterdam and pre-Amsterdam paths. The BAL recording logic is identical in both branches (lines 629-638 vs 658-667).

Recommendation: Consider extracting the BAL recording logic into a helper function to avoid duplication and reduce maintenance burden.

5. CREATE Transaction BAL Recording ✓

File: crates/vm/levm/src/execution_handlers.rs:125-129

// EIP-7928: Record contract address in BAL before collision check.
if let Some(recorder) = self.db.bal_recorder.as_mut() {
    recorder.record_touched_address(new_contract_address);
}

This correctly records the contract address before the collision check, as specified by EIP-7928. Good implementation.

6. Block Access List Storage Write Deduplication

File: crates/common/types/block_access_list.rs:727-740, 882-893

The change from in-place updates to always-push is correctly motivated:

// Always push a new entry instead of updating in-place.
// This is necessary for correct checkpoint/restore semantics:
// restore() truncates the vector by length, so in-place updates
// would corrupt values that should be preserved after a revert.

The deduplication happens at serialization time (lines 887-893), keeping the last write per block_access_index. This is correct per EIP-7928.

Concern: Using BTreeMap for deduplication adds O(n log n) complexity. For typical transaction counts, this is acceptable, but consider documenting the performance characteristics.

Fork Configuration Issues

7. Blob Schedule Fallback Logic

File: crates/common/types/genesis.rs:465-504

The new fallback logic has an issue:

// Amsterdam implies BPO2 blob params when no explicit schedule is set.
if self.is_bpo2_activated(block_timestamp) || self.is_amsterdam_activated(block_timestamp) {
    Some(self.blob_schedule.bpo2)
}

Issue: Line 491 returns bpo2 for both BPO2 and Amsterdam when no explicit schedule is set. However, if Amsterdam is activated but BPO2 is not, this could still incorrectly return BPO2 params.

Recommendation: Split these conditions:

if self.is_bpo2_activated(block_timestamp) {
    Some(self.blob_schedule.bpo2)
} else if self.is_amsterdam_activated(block_timestamp) {
    // Amsterdam defaults to BPO2 params when not explicitly configured
    Some(self.blob_schedule.bpo2)
}

This makes the logic clearer and ensures BPO2 activation is checked first.

8. Gas Limit Overflow Error Messages

File: crates/vm/backends/levm/mod.rs:91-100, 407-416

The error messages for Amsterdam vs pre-Amsterdam are different but both use record_bal as the gate:

if record_bal {
    return Err(EvmError::Transaction(format!(
        "Block gas used overflow: pre-refund gas used {} plus transaction gas limit {} exceeds block gas limit {}",
        ...
    )));
}

Issue: record_bal is a runtime flag that may not perfectly align with fork activation. Consider using explicit fork comparison:

if self.env.config.fork >= Fork::Amsterdam {
    // Amsterdam-specific error
} else {
    // Legacy error
}

This makes the fork-specific behavior explicit and less dependent on secondary flags.

Testing & CI Changes

9. Hive Test Skipping ✓

File: .github/scripts/check-hive-results.sh:9-18

The skip patterns are well-documented with GitHub issue references:

# Known failing tests due to hive framework bugs (not ethrex issues)
# See: https://github.com/ethereum/hive/issues/1382
SKIP_PATTERNS=(
  "Invalid Missing Ancestor Syncing ReOrg, Timestamp, EmptyTxs=False, CanonicalReOrg=False, Invalid P8"
  ...
)

This is a reasonable temporary measure. Ensure these are tracked and re-enabled when the Hive issues are resolved.

10. Workflow Fixture Configuration

File: .github/workflows/daily_hive_report.yaml:172-182

The conditional fixture selection is correct:

if [[ "$SIM_LIMIT" == *"fork_Amsterdam"* ]]; then
  FLAGS+=" --sim.buildarg fixtures=https://github.com/ethereum/execution-spec-tests/releases/download/bal@v5.1.0/fixtures_bal.tar.gz"
  FLAGS+=" --sim.buildarg branch=devnets/bal/2"
else
  FLAGS+=" --sim.buildarg fixtures=...fixtures_develop.tar.gz"
  FLAGS+=" --sim.buildarg branch=forks/osaka"
fi

Minor Issue: Line 134 has a typo: clakiness should be flakiness.

Documentation

11. Excellent Documentation Update ✓

File: docs/developers/l1/testing/hive.md

The documentation has been significantly improved from 45 lines to 415 lines, covering:

• Prerequisites with installation instructions
• Multiple test execution modes
• Amsterdam fork-specific testing
• Troubleshooting guide
• Repository version configuration

This is a high-quality improvement that will greatly help developers.

Minor Issues

12. Whitespace-Only Change

File: crates/networking/rpc/engine/payload.rs:379

Empty line addition. Not significant.

13. Hive Version Update

File: .github/workflows/pr-main_l1.yaml:194-195

-hive_version: 7709e5892146c793307da072e1593f48039a7e4b
+hive_version: 0921fb7833e3de180eacdc9f26de6e51dcab0dba

Ensure this commit includes Amsterdam fork support. No other concerns.

Summary

Severity Breakdown

High Priority:

1. Type coercion safety in u64 deserialization (Item 1)
2. BAL index integer overflow (Item 2)

Medium Priority:
3. Blob schedule fallback logic ambiguity (Item 7)
4. Gas limit error message fork detection (Item 8)
5. Blob validation completeness verification (Item 3)

Low Priority:
6. Code duplication in SELFDESTRUCT (Item 4)
7. Performance documentation for BAL deduplication (Item 6)
8. Typo in workflow comment (Item 10)

Strengths

• EIP-7928 BAL tracking is well-implemented with clear comments
• Two-phase gas check for SELFDESTRUCT follows the spec correctly
• CREATE collision check correctly records addresses before validation
• Documentation improvements are excellent
• Test skipping is properly documented with upstream issue references

Recommendations

1. Address the high-priority type safety issues before merging
2. Consider extracting common BAL recording logic to reduce duplication
3. Add explicit fork checks instead of relying on secondary flags where possible
4. Verify blob version validation is complete across all code paths
5. Add unit tests for edge cases (large transaction counts, number type coercion)

The code demonstrates a solid understanding of EIP-7928 requirements and implements Amsterdam fork changes correctly in most areas. The main concerns are around edge case handling and type safety in deserialization.

---

Automated review by Claude (Anthropic) · custom prompt

[github-actions]

Lines of code report

Total lines added: 62
Total lines removed: 4
Total lines changed: 66

Detailed view

+--------------------------------------------------------------+-------+------+
| File                                                         | Lines | Diff |
+--------------------------------------------------------------+-------+------+
| ethrex/crates/blockchain/payload.rs                          | 710   | +4   |
+--------------------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/block_producer/payload_builder.rs | 234   | +2   |
+--------------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/engine/client_version.rs        | 58    | -4   |
+--------------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/engine/mod.rs                   | 63    | +1   |
+--------------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/engine/payload.rs               | 973   | +50  |
+--------------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/rpc.rs                          | 939   | +1   |
+--------------------------------------------------------------+-------+------+
| ethrex/crates/vm/backends/levm/mod.rs                        | 811   | +4   |
+--------------------------------------------------------------+-------+------+

[github-actions]

Benchmark Results Comparison

No significant difference was registered for any benchmark run.

Detailed Results

Benchmark Results: BubbleSort

Command	Mean [s]	Min [s]	Max [s]	Relative
main_revm_BubbleSort	2.831 ± 0.038	2.800	2.922	1.01 ± 0.02
main_levm_BubbleSort	2.849 ± 0.024	2.836	2.915	1.01 ± 0.01
pr_revm_BubbleSort	2.816 ± 0.020	2.793	2.849	1.00
pr_levm_BubbleSort	2.845 ± 0.007	2.834	2.857	1.01 ± 0.01

Benchmark Results: ERC20Approval

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
main_revm_ERC20Approval	922.0 ± 10.7	913.7	943.5	1.00 ± 0.02
main_levm_ERC20Approval	1029.4 ± 3.6	1023.0	1033.0	1.12 ± 0.01
pr_revm_ERC20Approval	921.6 ± 10.2	914.5	946.3	1.00
pr_levm_ERC20Approval	1030.4 ± 5.1	1025.4	1041.5	1.12 ± 0.01

Benchmark Results: ERC20Mint

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
main_revm_ERC20Mint	120.3 ± 0.8	119.6	122.1	1.00
main_levm_ERC20Mint	151.9 ± 0.4	151.3	152.5	1.26 ± 0.01
pr_revm_ERC20Mint	120.6 ± 1.1	119.6	122.4	1.00 ± 0.01
pr_levm_ERC20Mint	151.9 ± 0.9	151.0	153.8	1.26 ± 0.01

Benchmark Results: ERC20Transfer

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
main_revm_ERC20Transfer	217.4 ± 1.2	215.8	219.1	1.00 ± 0.02
main_levm_ERC20Transfer	263.5 ± 4.9	260.1	276.1	1.21 ± 0.03
pr_revm_ERC20Transfer	217.0 ± 3.2	215.2	225.9	1.00
pr_levm_ERC20Transfer	262.1 ± 1.7	260.1	265.5	1.21 ± 0.02

Benchmark Results: Factorial

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
main_revm_Factorial	184.3 ± 4.8	181.5	196.5	1.00 ± 0.03
main_levm_Factorial	215.1 ± 7.0	208.2	227.3	1.17 ± 0.04
pr_revm_Factorial	183.8 ± 3.6	181.7	193.3	1.00
pr_levm_Factorial	212.6 ± 6.9	208.5	229.4	1.16 ± 0.04

Benchmark Results: FactorialRecursive

Command	Mean [s]	Min [s]	Max [s]	Relative
main_revm_FactorialRecursive	1.347 ± 0.045	1.268	1.417	1.00
main_levm_FactorialRecursive	7.050 ± 0.029	7.009	7.097	5.23 ± 0.18
pr_revm_FactorialRecursive	1.358 ± 0.036	1.284	1.416	1.01 ± 0.04
pr_levm_FactorialRecursive	7.038 ± 0.029	6.982	7.083	5.23 ± 0.18

Benchmark Results: Fibonacci

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
main_revm_Fibonacci	152.9 ± 1.5	151.2	156.6	1.01 ± 0.01
main_levm_Fibonacci	187.9 ± 4.8	184.2	200.4	1.24 ± 0.03
pr_revm_Fibonacci	151.4 ± 0.3	150.8	151.7	1.00
pr_levm_Fibonacci	189.1 ± 5.1	184.9	198.8	1.25 ± 0.03

Benchmark Results: FibonacciRecursive

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
main_revm_FibonacciRecursive	702.5 ± 10.7	690.6	729.7	1.15 ± 0.02
main_levm_FibonacciRecursive	612.3 ± 7.6	606.3	632.5	1.00 ± 0.02
pr_revm_FibonacciRecursive	697.5 ± 4.8	691.5	705.2	1.14 ± 0.01
pr_levm_FibonacciRecursive	609.4 ± 5.4	601.6	619.9	1.00

Benchmark Results: ManyHashes

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
main_revm_ManyHashes	7.8 ± 0.1	7.7	7.9	1.01 ± 0.01
main_levm_ManyHashes	9.2 ± 0.2	9.1	9.6	1.18 ± 0.02
pr_revm_ManyHashes	7.8 ± 0.1	7.7	7.9	1.00
pr_levm_ManyHashes	9.2 ± 0.2	9.0	9.8	1.19 ± 0.03

Benchmark Results: MstoreBench

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
main_revm_MstoreBench	255.3 ± 2.0	253.7	260.6	1.23 ± 0.02
main_levm_MstoreBench	207.0 ± 2.1	205.1	212.5	1.00
pr_revm_MstoreBench	256.0 ± 3.1	253.8	263.7	1.24 ± 0.02
pr_levm_MstoreBench	212.1 ± 14.8	205.3	254.0	1.02 ± 0.07

Benchmark Results: Push

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
main_revm_Push	242.3 ± 5.9	239.8	258.9	1.09 ± 0.03
main_levm_Push	223.2 ± 1.4	221.7	225.5	1.00
pr_revm_Push	241.0 ± 0.5	240.2	241.7	1.08 ± 0.01
pr_levm_Push	224.4 ± 3.8	222.1	234.9	1.01 ± 0.02

Benchmark Results: SstoreBench_no_opt

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
main_revm_SstoreBench_no_opt	142.0 ± 2.5	140.6	148.8	1.78 ± 0.04
main_levm_SstoreBench_no_opt	79.7 ± 1.0	78.9	82.2	1.00
pr_revm_SstoreBench_no_opt	142.1 ± 2.0	140.4	147.4	1.78 ± 0.03
pr_levm_SstoreBench_no_opt	79.8 ± 0.9	78.8	81.5	1.00 ± 0.02