Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core/vm: clean up EVM environmental structure #31061

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

rjl493456442
Copy link
Member

This PR does a few things including:

  • Remove ContractRef interface
  • Remove vm.AccountRef which implements ContractRef interface
  • Maintain the jumpDests struct in EVM for sharing between call frames
  • Simplify the delegateCall context initialization

@rjl493456442
Copy link
Member Author

rjl493456442 commented Jan 22, 2025

I will setup a full sync to ensure nothing is broken. As the changes touch very core part.

https://my.papertrailapp.com/systems/13274397401/events

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

In general LGTM, a great idea and makes stuff a lot cleaner, there was just two things that I wanted to talk about before merging (not really related to this PR, just the code in general)

c.Code = code
c.CodeHash = hash
c.CodeAddr = addr
Copy link
Member

Choose a reason for hiding this comment

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

This was never used, right? weird...

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know... At least it's not used now

@rjl493456442 rjl493456442 force-pushed the evm-remove-accountref branch from 27d3025 to 5893f07 Compare January 22, 2025 11:09
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

LGTM

For everyone else reviewing this, we slightly change the behavior with this.
Previously a contract created by CREATE2 would cache the jumpdest analysis for the initcode, while a contracted created by CREATE would not. Now both contracts will NOT cache the jumpdest analysis for initcode. Since it is very unlikely for two initcodes to be exactly the same, and in the case of an attack its very easy to circumvent this, its better not to bloat the memory with unusable jumpdest analyses

@rjl493456442 rjl493456442 force-pushed the evm-remove-accountref branch from 4564ca0 to e71bbae Compare February 7, 2025 04:03
@rjl493456442 rjl493456442 added this to the 1.15.1 milestone Feb 7, 2025
@gballet gballet self-assigned this Feb 7, 2025

// CallContext provides a basic interface for the EVM calling conventions. The EVM
// depends on this context being implemented for doing subcalls and initialising new EVM contracts.
type CallContext interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Searching for vm.CallContext across github yields only https://github.com/Fantom-foundation/go-ethereum-substate/blob/ab1d0339012b5f67dae1c8bcff7ff1429feeff36/core/vm/evm.go#L133 which looks like an old version of the source. Probably safe to remove.

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

Just one question otherwise I think it's a very nice PR

paddedCodeCopy, copyOffset, nonPaddedCopyLength := getDataAndAdjustedBounds(code, uint64CodeOffset, length.Uint64())
if !contract.IsSystemCall {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like an alternative to #31114. Checking the conversation there it seems unresolved. Should we leave it out of this PR or do you think this is the correct fix for the issue?

c.jumpdests = parent.jumpdests
} else {
c.jumpdests = make(map[common.Hash]bitvec)
func NewContract(caller common.Address, address common.Address, value *uint256.Int, gas uint64, jumpDests map[common.Hash]bitvec) *Contract {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding code and codeHash here? It seems in all cases we are doing:

c := NewContract()
c.SetCallCode

@fjl fjl modified the milestones: 1.15.1, 1.15.2, 1.15.3 Feb 13, 2025
@rjl493456442 rjl493456442 merged commit 32c6aa8 into ethereum:master Feb 18, 2025
4 checks passed
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.

5 participants