accounts/abi/bind/backend: SimulatedBackend options to always honour vm.Config

[ARR4N]

backends.SimulatedBackend has an internal method callContract() that is common to each of CallContract(), PendingCallContract() and EstimateGas(). In cloning the EVM to run the calls, it fails to to copy any vm.Config held by the underlying *core.Blockchain. This makes it impossible to, for example, trace static calls in testing*.

This PR introduces variadic configuration Options to override the default functionality in a backwards-compatible manner by not changing the effective function signature for existing code. The alternative of a config struct with a New() *SimulatedBackend method makes for a more cumbersome API, hence the Option pattern.

The options CloneVMConfigOnCall(), CloneVMConfigOnPendingCall(), ClondeVMConfigOnEstimateGas(), and AlwaysCloneVMConfig() do as they say on the tin. Internally, they simply switch flags in a new struct in SimulatedBackend. The NoBaseFee field of vm.Config is always set to true, in keeping with historical behaviour.

*This isn't just a hypothetical situation: I discovered it after writing a trace-based coverage analysis tool for Solidity contracts, which is blocked by this PR.

[ARR4N]

@gballet and @MariusVanDerWijden can you please take a look when you get a chance? I've also sent over #25055 but if you've only got time for one review, please focus on this one.

[MariusVanDerWijden]

Hey @aschlosberg, thank you for the PR and sorry for taking so long to look at it. Generally the idea of having a way to modify the vm.Config within the Simulated Backend makes sense to me. Your approach seems very complicated for trying to fix such a small issue though. My idea would be to add a *vm.Config to the SimulatedBackend struct and add a SetVMConfig(*vm.Config method to it which can be used to set it.
Then on callContract you can check whether s.vmconf != nil? and use the override, otherwise use the default one.
What do you think?

[ARR4N]

Hey @MariusVanDerWijden, not a problem re the delay, this is low priority :) thanks for the feedback.

What would your suggested approach look like for SendTransaction()? Would it copy the *vm.Config to the underlying *core.Blockchain? I'm concerned that the transaction and call configs would either diverge or we'd require users to set the value twice.

Which bit is too complicated for you? The introduction of Options, or the granularity of those provided? Or something else? I think that only having a clone-on-call (regular or pending) without the granularity of the 3 paths would make sense—I personally would need to exclude gas estimation. If it's the Option pattern that you don't like, a method that achieves the same thing would work although I do think it feels more natural in a constructor:

sim := backends.NewSimulatedBackend(…)
cfg := sim.Blockchain().GetVMConfig()
… // modify cfg
sim.CloneVMConfigOnCall(true)

is pretty similar to:

```Go
sim := backends.NewSimulatedBackend(…, backends.CloneVMConfigOnCall())
cfg := sim.Blockchain().GetVMConfig()
… // modify cfg

but tx vs call configs gets messy:

sim := backends.NewSimulatedBackend(…)
txConfig := sim.Blockchain().GetVMConfig()
… // modify txConfig
sim.SetVMConfigForCalls(txConfig) // same config? different one? txConfig is a pointer so do we clone it here?

[ARR4N]

@MariusVanDerWijden I've opted to simplify the entire process by just exposing the flag struct that was previously unexported, reverting all of the Options. Please can you take another look?

[fjl]

Wouldn't it be simpler to just copy the config on every call? Why should that be optional?

[fjl]

Another idea: add a method SetVMConfig on SimulatedBackend, which would set the config to be used for Call/EstimateGas etc.

Edit: @MariusVanDerWijden already figured that out 17 days ago :)

[ARR4N]

Wouldn't it be simpler to just copy the config on every call? Why should that be optional?

That would be ideal, but it introduces a breaking change into an exported API. Unfortunately the historical behaviour might be relied on by someone so I don't think we should change it, which is why there's otherwise unnecessary complexity.

Another idea: add a method SetVMConfig on SimulatedBackend, which would set the config to be used for Call/EstimateGas etc.

Would this also be copied to the config held by sim.Blockchain().GetVMConfig()? Having a SetVMConfig() that only works for non-transaction paths doesn't feel right.

[holiman]

IMO we should just add a new constructor, where you pass in the config you want to use. That way, it also gets passed into the blockchain. It becomes 'weird' if the backend has one config, and the internal blockchain has another. It also gets weird if you try to SetConfig at arbitrary points in time, and things like the extra eips get changed.

The alternative of a config struct with a New() *SimulatedBackend method makes for a more cumbersome API, hence the Option pattern.

I still think that would be the simplest solution.

[ARR4N]

Thanks @holiman. Just to confirm that we weren't talking about different things, I meant a type backends.SimulatedBackendConfig (or more succinct name) that has a New(). Do you mean the same, or do you mean backends.NewSimulatedBackendWithConfig(*vm.Config) that is then propagated to the blockchain and always used? I'm not in favour of the former, and I think the latter can be improved upon:

We'd then have NewSimulatedBackend(), New…WithDatabase(), and New…WithVMConfig() and no way to specify both the database and the VM config. This is why I originally suggested variadic options to NewSimulatedBackend()—it's a common pattern (example from Google + the original recommendation from Go expert, Dave Cheney) for specifying an arbitrary number of configuration directives without changing the constructor signature. I'd go so far as recommending deprecation of New…WithDatabase() in a future PR, in favour of NewSimulatedBackend(alloc, gasLimit, backends.WithDatabase(db)) so there's only 1 constructor.

It becomes 'weird' if the backend has one config, and the internal blockchain has another.

I'm not suggesting it should, which is why I'd like to clarify in my first paragraph.

[holiman]

or do you mean backends.NewSimulatedBackendWithConfig(*vm.Config) that is then propagated to the blockchain and always used?

Yes, that's what I meant. Re the constructors, I'm not sure I have a preference. I mean, we could just clobber them as NewSimulatedBackendWithDatabaseAndConfig, or do the more elegant variadic options-route. I could probably accept either, not sure what other people prefer.

[ARR4N]

we could just clobber them as NewSimulatedBackendWithDatabaseAndConfig, or do the more elegant variadic options-route. I could probably accept either, not sure what other people prefer

@MariusVanDerWijden originally felt the options were too complicated, but that was only when we were considering adding vm.Config and I hadn't realised that we'd end up with the power set of constructors 🤣. One more value that needs to be configured and we're at 9 x New…WithFooAndBarAndBaz(). I also had too many options so would no longer give fine-grained control over when config cloning occurs.

@MariusVanDerWijden would you be ok with the following?

type Option …

func NewSimulatedBackend(core.GenesisAlloc, uint64, ...Option) *SimulatedBackend

// WithDatabase configures a SimulatedBackend to use the specified database.
func WithDatabase(ethdb.Database) Option

// WithVMConfig configures a SimulatedBackend to use the specified VM config, which is used for transactions and calls.
func WithVMConfig(vm.Config) Option

// NewSimulatedBackendWithDatabase creates a new binding backend based on the given database and uses a simulated blockchain for testing purposes. A simulated backend always uses chainID 1337.
// DEPRECATED: use NewSimulatedBackend(WithDatabase()).
func NewSimulatedBackendWithDatabase(ethdb.Database, core.GenesisAlloc, gasLimit) *SimulatedBackend

I can push the DB Option to a later PR if you'd like, but I'm sure we all want to see this whole thing wrapped up sooner.

[MariusVanDerWijden]

We discussed that we like the big long clunky constructor better than the variadic methods. So just adding NewSimulatedBackendWithDBAndConfig should do the trick. We can worry about smarter approaches when we add the next 9 constructors ;)
Also I still think that a SetVMConfig() method might be a good idea

[ARR4N]

We discussed that we like the big long clunky constructor better than the variadic methods. So just adding NewSimulatedBackendWithDBAndConfig should do the trick. We can worry about smarter approaches when we add the next 9 constructors ;) Also I still think that a SetVMConfig() method might be a good idea

Both done, with backwards compatibility maintained. Anyone who uses the original constructors and directly modifies the vm.Config won't have it cloned unless they swap constructor or use SetVMConfig().

[fjl]

We are reworking the simulated backend in #28202 so this doesn't really apply anymore.