Skip to content

Conversation

@georgehao
Copy link
Member

@georgehao georgehao commented Dec 12, 2025

1. Purpose or design rationale of this PR

insert bytecode into witness for EXTCODEHASH opcode

2. PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • build: Changes that affect the build system or external dependencies (example scopes: yarn, eslint, typescript)
  • ci: Changes to our CI configuration files and scripts (example scopes: vercel, github, cypress)
  • docs: Documentation-only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that doesn't fix a bug, or add a feature, or improves performance
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • test: Adding missing tests or correcting existing tests

3. Deployment tag versioning

Has the version in params/version.go been updated?

  • This PR doesn't involve a new deployment, git tag, docker image tag, and it doesn't affect traces
  • Yes

4. Breaking change label

Does this PR have the breaking-change label?

  • This PR is not a breaking change
  • Yes

Summary by CodeRabbit

Bug Fixes

  • Enhanced consistency in code recording during state operations to ensure uniform behavior across related functions when tracking code data.

✏️ Tip: You can customize this high-level summary in your review settings.

@georgehao georgehao requested review from Thegaram and lispc December 12, 2025 08:31
@coderabbitai
Copy link

coderabbitai bot commented Dec 12, 2025

Walkthrough

The change adds a call to AddCode() in the GetKeccakCodeHash function to record state object code into the witness, mirroring behavior already present in GetCode and GetCodeSize methods for consistency.

Changes

Cohort / File(s) Summary
Witness consistency in GetKeccakCodeHash
core/state/statedb.go
Added AddCode(stateObject.Code(s.db)) call when a state object exists in GetKeccakCodeHash to record code into the witness, aligning with existing patterns in GetCode and GetCodeSize.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

  • Single function modification following an established pattern
  • Direct consistency alignment with nearby code paths

Possibly related PRs

Suggested reviewers

  • lightsing
  • colinlyguo

Poem

🐰 A witness must see all the code,
So keccak hashes down the road,
Now consistent, tried and true,
Every function plays its part too! ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title check ⚠️ Warning The title clearly describes the main change—adding bytecode to the witness for EXTCODEHASH—but does not follow the required conventional commits format with a proper type prefix. Prefix the title with a conventional commits type (e.g., 'feat:' or 'fix:') as required by the repository's PR template.
Description check ⚠️ Warning The description includes the template structure but lacks substantive content; section 1 merely repeats the title, and no checkboxes are selected to indicate PR type, versioning status, or breaking change status. Complete the description by providing design rationale in section 1, selecting appropriate checkboxes for PR type and versioning in sections 2-4, and removing or clarifying the unconventional title section.
✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/bytecode_to_witness

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 88dcc09 and 23721c1.

📒 Files selected for processing (1)
  • core/state/statedb.go (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-11T12:09:04.380Z
Learnt from: Thegaram
Repo: scroll-tech/go-ethereum PR: 1193
File: core/state/statedb.go:170-174
Timestamp: 2025-06-11T12:09:04.380Z
Learning: `StateDB.Witness()` returns the live *stateless.Witness pointer because downstream code (`generateWitness`) needs to push data into it, and that path is executed sequentially, so concurrency safety is not a concern.

Applied to files:

  • core/state/statedb.go
🧬 Code graph analysis (1)
core/state/statedb.go (1)
core/state/state_object.go (1)
  • Code (38-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: test
  • GitHub Check: Analyze (go)
🔇 Additional comments (1)
core/state/statedb.go (1)

336-345: The suggested refactor is ineffective and inconsistent with existing code.

The CodeSize check does not prevent DB reads—both Code() and CodeSize() perform identical cache and empty-hash checks before accessing the database. The if len(code) > 0 guard is redundant since Witness.AddCode() already returns early for empty slices (line 84-85 in core/stateless/witness.go).

Additionally, GetCode() (line 312) and GetCodeSize() (line 321) already follow the same pattern of calling stateObject.Code(s.db) before the witness check, yet the suggested optimization is only applied to GetKeccakCodeHash(). If the optimization were valid, it should be applied consistently across all three methods.

The code as-is is correct: calling Code() to populate the witness is necessary and unavoidable when the witness is enabled. The pattern aligns with GetCode() and GetCodeSize() for consistency.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@Thegaram Thegaram left a comment

Choose a reason for hiding this comment

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

Looks good. Please also bump version.

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.

4 participants