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

Incorrect gas cost passed to tracer for EIP-2929 calls with cold account accesses #22649

Closed
adietrichs opened this issue Apr 11, 2021 · 5 comments · Fixed by #22702
Closed

Incorrect gas cost passed to tracer for EIP-2929 calls with cold account accesses #22649

adietrichs opened this issue Apr 11, 2021 · 5 comments · Fixed by #22702
Labels

Comments

@adietrichs
Copy link
Member

System information

Commit hash : anything past 6487c00 that introduced EIP-2929 support

Expected behaviour

With EIP-2929 enabled, the dynamic cost of all call types includes an optional COLD_ACCOUNT_ACCESS_COST. This cost, if charged, should also be part of the total opcode gas cost passed to the tracer and exposed e.g. via debug_traceTransaction.

Actual behaviour

While properly charged, the COLD_ACCOUNT_ACCESS_COST is never part of the gas cost reported to the tracer.

Explanation

6487c00 added the new core/vm/operations_acl.go file for EIP-2929 gas logic. Dynamic gas cost calculation for all call types is wrapped in operations_acl.go#L176:

func makeCallVariantGasCallEIP2929(oldCalculator gasFunc) gasFunc {
	return func(evm *EVM, contract *Contract, stack *Stack, mem *Memory, memorySize uint64) (uint64, error) {
		addr := common.Address(stack.Back(1).Bytes20())
		if !evm.StateDB.AddressInAccessList(addr) {
			evm.StateDB.AddAddressToAccessList(addr)
			if !contract.UseGas(ColdAccountAccessCostEIP2929 - WarmStorageReadCostEIP2929) {
				return 0, ErrOutOfGas
			}
		}
		return oldCalculator(evm, contract, stack, mem, memorySize)
	}
}

The issue is that the ColdAccountAccessCostEIP2929 is charged directly, instead of being passed to the outside together with the rest of the dynamic gas, to be charged there and passed to the tracer. While this is necessary to properly calculate the available call gas in the inner gasFunc, it causes the incorrect gas reporting to the tracer at interpreter.go#L278.

@adietrichs
Copy link
Member Author

adietrichs commented Apr 11, 2021

I will see if I can create a PR later today with a proposed fix that keeps the existing makeCallVariantGasCallEIP2929 wrapper structure.

@ligi
Copy link
Member

ligi commented Apr 15, 2021

Thanks for the great detailed issue! We would very much like to see the PR you make later today.

@adietrichs
Copy link
Member Author

later today

Turns out that was indeed optimistic. I created #22680 now though, which is the best minimal fix I could come up with.

@holiman
Copy link
Contributor

holiman commented Apr 20, 2021

I wanted to push some changes to this PR, but you seem to have disabled maintainer-push on your fork. I might open a new PR and cherry-pick your commit, if that's ok?

@adietrichs
Copy link
Member Author

Ah, wasn't aware we had disabled maintainer-push. New PR is fine with me, will close the other one then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants