fix(evm): Update the access list with the correct precompile #3000
Conversation
WalkthroughThe changes in this pull request primarily involve updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment. 8000
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/evm/keeper/precompiles.go (1)
60-61: Consider documenting the Create hook consideration.As mentioned in the PR objectives, there's uncertainty about whether the
Createmethod could utilize static precompiles. Consider adding a comment to document this consideration for future reference.// If the precompile instance is created, we have to update the EVM with // only the recipient precompile and add it's address to the access list. +// Note: If the Create method is found to utilize static precompiles in the future, +// the access list population logic may need to be adjusted accordingly.CHANGELOG.md (1)
48-48: LGTM with minor formatting suggestionThe changelog entry properly documents a state machine breaking change. Consider breaking the line into two for better readability:
-- (evm) [#3000](https://github.com/evmos/evmos/pull/3000) Move population of access list with precompile addresses into EVM hook. ++ (evm) [#3000](https://github.com/evmos/evmos/pull/3000) Move population of access list with precompile ++ addresses into EVM hook.🧰 Tools
🪛 Markdownlint (0.35.0)
48-48: Expected: 120; Actual: 129
Line length(MD013, line-length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)x/evm/keeper/precompiles.go(1 hunks)x/evm/keeper/state_transition.go(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.35.0)
CHANGELOG.md
48-48: Expected: 120; Actual: 129
Line length
(MD013, line-length)
🔇 Additional comments (4)
x/evm/keeper/precompiles.go (2)
60-66: LGTM! Implementation correctly addresses access list population.
The changes properly fix the access list population by:
- Only adding precompiles when they are actual call recipients
- Supporting both static and dynamic precompiles
- Avoiding unnecessary inclusion of all static precompiles
This aligns perfectly with the PR objectives.
64-64: Verify removal of other access list population code.
Let's verify that this is now the only place where precompile addresses are added to the access list, ensuring we've properly centralized this logic.
x/evm/keeper/state_transition.go (2)
311-314: Clarify handling of precompiles in Create operations.
As mentioned in the PR objectives, there's uncertainty about whether the Create method could utilize static precompiles. This needs clarification to ensure we're not breaking any edge cases.
Let's check if Create operations could interact with precompiles:
311-314: LGTM! Verify hook implementation for precompile access list.
The change correctly removes static precompiles from initial access list preparation, allowing dynamic population through the EVM hook during calls. This aligns with the PR objectives and fixes the issue of unnecessary precompile inclusion.
Let's verify the hook implementation that handles precompile access list population:
✅ Verification successful
Let me verify the hook implementation by searching for the precompile hook that adds addresses to the access list.
Let me check the full implementation of the precompile hook to verify how it handles the access list.
LGTM! The precompile access list handling is correctly implemented.
The changes to remove static precompiles from initial access list preparation are safe because:
- The precompile hook correctly adds the recipient precompile address to the access list when it's actually used in a call (
evm.StateDB.AddAddressToAccessList(recipient)) - The hook is properly registered and called during EVM execution
- This approach is more efficient as it only adds precompiles to the access list when they are actually called, rather than including all static precompiles upfront
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the precompile hook implementation
# Expected: Find the hook implementation that adds precompiles to access list
# Search for the precompile hook implementation
ast-grep --pattern 'GetPrecompilesCallHook($$$)'
# Search for access list modifications within the hook
rg -A 5 'PrepareAccessList|AccessList'
Length of output: 104279
Script:
#!/bin/bash
# Search for precompile hook implementation that adds addresses to access list
ast-grep --pattern 'func $_($_, $_, $_, $_, $_, $_) vm.PrecompileCallHook {
$$$
AddAddressToAccessList($$$)
$$$
}'
# Search for precompile hook registration
rg -A 5 'GetPrecompilesCallHook|RegisterPrecompileHooks'
Length of output: 1284
Script:
#!/bin/bash
# Get the full implementation of GetPrecompilesCallHook and surrounding context
rg -B 5 -A 15 'func \(k \*Keeper\) GetPrecompilesCallHook'
# Also check for any other access list modifications in precompiles.go
rg -A 3 'AddAddressToAccessList' x/evm/keeper/precompiles.go
Length of output: 1568
Description
Currently, the access list is populated with all static precompiles. This logic does not take into consideration the new EVM type, which uses hooks to populate the precompiles field just in time if a precompile is a receiver of a
Call.For this reason:
This PR moves the logic to add precompiled addresses in the access list into the Call hook.
Considerations
If in the future, the
CreateEVM's method can use static precompiles, we should populate the access list with the target one. I'm not sure this is a possibility, but it is something we should keep in mind. If we don't want to think about it in the future, I could add again the logic to add to the access list all static preocmpiles.Summary by CodeRabbit
New Features
CHANGELOG.mdwith new entries detailing significant changes.FeeMarketWrapperfor managing EVM coins with varying decimal precision.slashingprecompile and enhanced governance queries.Bug Fixes
Improvements