Skip to content

feat(geth): update go-ethereum version to v1.16.9, enable Osaka hard fork#957

Open
XinyuCRO wants to merge 8 commits into
developfrom
feat/geth
Open

feat(geth): update go-ethereum version to v1.16.9, enable Osaka hard fork#957
XinyuCRO wants to merge 8 commits into
developfrom
feat/geth

Conversation

@XinyuCRO
Copy link
Copy Markdown

@XinyuCRO XinyuCRO commented May 19, 2026

@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

❌ Patch coverage is 61.70213% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.21%. Comparing base (f959eb9) to head (3853b84).
⚠️ Report is 4 commits behind head on develop.

Files with missing lines Patch % Lines
x/evm/statedb/statedb.go 16.66% 5 Missing ⚠️
rpc/backend/utils.go 0.00% 2 Missing ⚠️
x/evm/keeper/migrations.go 0.00% 2 Missing ⚠️
x/evm/keeper/set_code_authorizations.go 0.00% 2 Missing ⚠️
x/evm/statedb/statedb_hooked.go 84.61% 2 Missing ⚠️
x/evm/types/chain_config.go 60.00% 1 Missing and 1 partial ⚠️
rpc/types/simulate.go 0.00% 1 Missing ⚠️
rpc/types/types.go 0.00% 1 Missing ⚠️
x/evm/types/msg.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #957      +/-   ##
===========================================
+ Coverage    44.15%   44.21%   +0.06%     
===========================================
  Files          196      197       +1     
  Lines        16896    16927      +31     
===========================================
+ Hits          7461     7485      +24     
- Misses        8645     8652       +7     
  Partials       790      790              
Files with missing lines Coverage Δ
x/evm/migrations/v8/migrate.go 100.00% <100.00%> (ø)
x/evm/types/tx_args.go 97.87% <100.00%> (ø)
rpc/types/simulate.go 82.78% <0.00%> (ø)
rpc/types/types.go 7.89% <0.00%> (ø)
x/evm/types/msg.go 57.73% <0.00%> (ø)
rpc/backend/utils.go 49.71% <0.00%> (ø)
x/evm/keeper/migrations.go 28.57% <0.00%> (ø)
x/evm/keeper/set_code_authorizations.go 29.62% <0.00%> (ø)
x/evm/statedb/statedb_hooked.go 42.28% <84.61%> (+3.71%) ⬆️
x/evm/types/chain_config.go 58.04% <60.00%> (+0.07%) ⬆️
... and 1 more
🚀 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.

@XinyuCRO XinyuCRO marked this pull request as ready for review May 21, 2026 06:01
@XinyuCRO XinyuCRO requested a review from a team as a code owner May 21, 2026 06:01
@XinyuCRO XinyuCRO requested review from ApacCronos and calvinaco and removed request for a team May 21, 2026 06:01
@JayT106
Copy link
Copy Markdown

JayT106 commented May 21, 2026

@claude review PR

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2026

Claude finished @JayT106's task in 7m 35s —— View job


PR Review: feat(geth): update go-ethereum version to v1.16.9, enable Osaka hard fork

Tasks:

  • Read repository CLAUDE.md for guidelines
  • Analyze PR diff and changed files
  • Review core changes (go-ethereum upgrade, Osaka hard fork)
  • Check for state-breaking changes and consensus impact
  • Review new files and migrations
  • Review test coverage
  • Post final review feedback

Summary

This PR upgrades go-ethereum from v1.15.11 → v1.16.9 and enables the Osaka hard fork. Overall the approach is consistent with prior fork integrations (Shanghai → Cancun → Prague → Osaka). Several issues are worth addressing before merging.


⚠️ State-Breaking / Consensus-Breaking Changes

This PR is a consensus-breaking upgrade and must be coordinated with an upgrade plan.

In-Place Migration (v7 → v8):

  • x/evm/module.go:65 correctly bumps ConsensusVersion() to 8
  • x/evm/migrations/v8/migrate.go sets OsakaTime = 0, immediately activating Osaka for all chains on upgrade
  • Migration is registered at x/evm/module.go:150 as Migrate7to8
  • The upgrade handler at evmd/upgrades.go:26-30 calls RunMigrations, which will trigger the v8 migration

Impact: All validators must upgrade to this version before the sdk53 upgrade height or the chain will fork. This follows the same pattern as Prague/Cancun activation. ✓


Issues Found

1. P256VERIFY Test Vector Byte Misalignment — tests/integration_tests/test_call.py

The P256VERIFY_VALID_VECTOR dict has hex strings of incorrect lengths for three fields:

  • signature_s: 63 chars (should be 64 = 32 bytes)
  • public_key_x: 63 chars (should be 64 = 32 bytes)
  • public_key_y: 66 chars (should be 64 = 32 bytes)

The combined total (64+64+63+63+66 = 320 chars = 160 bytes) happens to be the correct total for P256VERIFY_INPUT_LENGTH, so bytes.fromhex("".join(vector.values())) won't raise an error and the length assertion at line 172 will pass. However, after concatenation, the field boundaries are misaligned — the precompile will receive mathematically wrong values for sig_s, pk_x, and pk_y, and signature verification is expected to fail.

The correct hex-encoded 32-byte values should have leading zeros where needed. Fix this →

2. Silently Discarded Error — rpc/backend/utils.go:257

reward, _ := tx.EffectiveGasTip(blockBaseFee)

The error from EffectiveGasTip is silently discarded. While the reward == nil guard below handles the nil case, the error carries information about why the tip is uncalculable (e.g., max fee cap below base fee). This matches the old EffectiveGasTipValue behavior, but consider whether log-level error surfacing would be useful in debug scenarios.

3. GetStateAndCommittedState — Missing HookedStateDB delegation test

statedb_hooked.go:78-80 adds GetStateAndCommittedState but statedb_hooked_test.go doesn't appear to test this new delegation. Given the coverage report flags statedb_hooked.go at 40%, adding a test case for this would be low-effort and improves confidence.


Design Observations (Non-blocking)

SetCode hook refactor (statedb_hooked.go:184-198):
The new implementation:

  1. Adds tracing.CodeChangeReason parameter — correct propagation of the upstream geth change
  2. Uses crypto.Keccak256Hash(prev) instead of ethtypes.EmptyCodeHash for empty prev code — functionally equivalent since keccak("") == EmptyCodeHash
  3. Guards with prevHash != codeHash to avoid spurious hook calls on unchanged code — this is a behavior improvement
  4. Supports OnCodeChangeV2 hook with reason argument — good forward compatibility

SkipFromEOACheckSkipTransactionChecks (msg.go:325, tx_args.go:232,327):
This is a go-ethereum v1.16 rename. The new field is semantically broader (covers more than just EOA sender checks) but the boolean usage is identical. ✓

Migration consistency:
v8 migration correctly follows the v7 pattern: read params, set new field to zero (immediate activation), validate, write back. The MigrateStoreEmptyStore test will actually panic (not return an error) because cdc.MustUnmarshal(nil, ...) panics on nil input. The test require.Error(t, err) won't catch a panic — this is a pre-existing pattern from v7, so not introduced here, but worth noting.


Test Coverage (from Codecov)

Files below 30% patch coverage for new code:

  • x/evm/keeper/migrations.go — 0% (2 lines)
  • x/evm/keeper/set_code_authorizations.go — 0% (2 lines, just the SetCode call-site changes)
  • rpc/types/simulate.go — 0% (1 line)
  • rpc/types/types.go — 0% (1 line)

These are mostly one-liner API-compatibility changes, so the low coverage is understandable. The migration itself has 100% coverage via migrate_test.go. ✓


Verdict

The PR is well-structured and follows established patterns. The P256VERIFY test vector alignment bug (point 1) is the most significant issue — the integration test will likely fail or produce incorrect results once Osaka is active. This should be fixed before merge.

Comment thread tests/integration_tests/test_call.py Outdated
Comment thread tests/integration_tests/test_call.py Outdated
Comment thread tests/integration_tests/test_call.py Outdated
Comment thread rpc/backend/utils.go Outdated
Comment thread x/evm/types/msg.go
Comment thread x/evm/statedb/statedb_hooked.go Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants