Skip to content

core/vm, core/state: return error if gas is insufficient for cost #540

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: kaustinen7-gas-costs
Choose a base branch
from
Open

core/vm, core/state: return error if gas is insufficient for cost #540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
abb0094
move extcodesize consumption to gas method
gballet Apr 9, 2025
515e4b3
core/vm, core/state: return error if gas is insufficient for cost
rjl493456442 Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
219 changes: 137 additions & 82 deletions core/state/access_events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,67 +91,104 @@ func (ae *AccessEvents) Copy() *AccessEvents {
return cpy
}

// AddAccount returns the gas to be charged for each of the currently cold
// member fields of an account.
func (ae *AccessEvents) AddAccount(addr common.Address, isWrite bool, availableGas uint64) uint64 {
var gas uint64 // accumulate the consumed gas
consumed, expected := ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BasicDataLeafKey, isWrite, availableGas)
if consumed < expected {
return expected
}
gas += consumed
consumed, expected = ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.CodeHashLeafKey, isWrite, availableGas-consumed)
if consumed < expected {
return expected + gas
}
gas += expected
return gas
// AddAccount returns the gas cost for each currently cold member field of
// an account. If the available gas is insufficient to cover the total cost,
// a false flag is returned.
func (ae *AccessEvents) AddAccount(addr common.Address, isWrite bool, availableGas uint64) (uint64, bool) {
var gas uint64
cost, sufficient := ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BasicDataLeafKey, isWrite, availableGas)
if !sufficient {
return 0, false
}
gas += cost

cost, sufficient = ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.CodeHashLeafKey, isWrite, availableGas-cost)
if !sufficient {
return 0, false
}
gas += cost
return gas, true
}

// MessageCallGas returns the gas to be charged for each of the currently
// cold member fields of an account, that need to be touched when making a message
// call to that account.
func (ae *AccessEvents) MessageCallGas(destination common.Address, availableGas uint64) uint64 {
_, expected := ae.touchAddressAndChargeGas(destination, zeroTreeIndex, utils.BasicDataLeafKey, false, availableGas)
if expected == 0 {
expected = params.WarmStorageReadCostEIP2929
}
return expected
// cold member fields of an account, that need to be touched when making
// a message call to that account. If the available gas is insufficient to
// cover the total cost, a false flag is returned.
func (ae *AccessEvents) MessageCallGas(destination common.Address, availableGas uint64) (uint64, bool) {
cost, sufficient := ae.touchAddressAndChargeGas(destination, zeroTreeIndex, utils.BasicDataLeafKey, false, availableGas)
if !sufficient {
return 0, false
}
if cost == 0 {
if availableGas < params.WarmStorageReadCostEIP2929 {
return 0, false
}
cost = params.WarmStorageReadCostEIP2929
}
return cost, true
}

// ValueTransferGas returns the gas to be charged for each of the currently
// cold balance member fields of the caller and the callee accounts.
func (ae *AccessEvents) ValueTransferGas(callerAddr, targetAddr common.Address, availableGas uint64) uint64 {
_, expected1 := ae.touchAddressAndChargeGas(callerAddr, zeroTreeIndex, utils.BasicDataLeafKey, true, availableGas)
if expected1 > availableGas {
return expected1
// cold balance member fields of the caller and the callee accounts. If the
// available gas is insufficient to cover the total cost, a false flag is
// returned.
func (ae *AccessEvents) ValueTransferGas(callerAddr, targetAddr common.Address, availableGas uint64) (uint64, bool) {
var gas uint64
cost, sufficient := ae.touchAddressAndChargeGas(callerAddr, zeroTreeIndex, utils.BasicDataLeafKey, true, availableGas)
if !sufficient {
return 0, false
}
_, expected2 := ae.touchAddressAndChargeGas(targetAddr, zeroTreeIndex, utils.BasicDataLeafKey, true, availableGas-expected1)
if expected1+expected2 > availableGas {
Copy link
Author

@rjl493456442 rjl493456442 Apr 11, 2025

Choose a reason for hiding this comment

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

Shouldn't it be expected1 + expected2?

Copy link
Owner

@gballet gballet Apr 11, 2025

Choose a reason for hiding this comment

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

not sure I understand the question, it is expected1+expected2

Copy link
Author

@rjl493456442 rjl493456442 Apr 12, 2025

Choose a reason for hiding this comment

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

in your implementation, the required gas is expected1+expected2, but the returned value is params.WarmStorageReadCostEIP2929 which i believe is a mistake

Copy link
Owner

@gballet gballet Apr 14, 2025

Choose a reason for hiding this comment

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

right it's a rebase error, in the source branch it's the correct behavior

return params.WarmStorageReadCostEIP2929
gas += cost

cost, sufficient = ae.touchAddressAndChargeGas(targetAddr, zeroTreeIndex, utils.BasicDataLeafKey, true, availableGas-cost)
if !sufficient {
return 0, false
}
return expected1 + expected2
gas += cost

return gas, true
}

// ContractCreatePreCheckGas charges access costs before
// a contract creation is initiated. It is just reads, because the
// address collision is done before the transfer, and so no write
// are guaranteed to happen at this point.
func (ae *AccessEvents) ContractCreatePreCheckGas(addr common.Address, availableGas uint64) uint64 {
consumed, expected1 := ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BasicDataLeafKey, false, availableGas)
Copy link
Author

@rjl493456442 rjl493456442 Apr 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

if consumed < expected1, then the availableGas-consumed will be incorrect

although the returned expected gas cost is correct...

Copy link
Owner

@gballet gballet Apr 11, 2025

Choose a reason for hiding this comment

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

makes sense: we are returning the expected gas and we are saving a confusing check. I can fix it if you don't like it.

Copy link
Owner

@gballet gballet Apr 11, 2025

Choose a reason for hiding this comment

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

come to think of if, it's really confusing. I'll fix it if we don't go your way.

Copy link
Author

@rjl493456442 rjl493456442 Apr 12, 2025

Choose a reason for hiding this comment

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

i would prefer to using my approach to aviod the complexity in the first place

_, expected2 := ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.CodeHashLeafKey, false, availableGas-consumed)
return expected1 + expected2
// ContractCreatePreCheckGas charges access costs before a contract creation is
// initiated. It is just reads, because the address collision is done before
// the transfer, and so no write are guaranteed to happen at this point.
// If the available gas is insufficient to cover the total cost, a false flag is
// returned.
func (ae *AccessEvents) ContractCreatePreCheckGas(addr common.Address, availableGas uint64) (uint64, bool) {
var gas uint64
cost, sufficient := ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BasicDataLeafKey, false, availableGas)
if !sufficient {
return 0, false
}
gas += cost

cost, sufficient = ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.CodeHashLeafKey, false, availableGas-cost)
if !sufficient {
return 0, false
}
gas += cost

return gas, false
}

// ContractCreateInitGas returns the access gas costs for the initialization of
// a contract creation.
func (ae *AccessEvents) ContractCreateInitGas(addr common.Address, availableGas uint64) (uint64, uint64) {
// a contract creation. If the available gas is insufficient to cover the total
// cost, a false flag is returned.
func (ae *AccessEvents) ContractCreateInitGas(addr common.Address, availableGas uint64) (uint64, bool) {
Copy link
Owner

@gballet gballet Apr 14, 2025

Choose a reason for hiding this comment

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

This single change breaks 243 execution spec tests out of 532

var gas uint64
consumed, expected1 := ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BasicDataLeafKey, true, availableGas)
gas += consumed
consumed, expected2 := ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.CodeHashLeafKey, true, availableGas-consumed)
gas += consumed
return gas, expected1 + expected2
cost, sufficient := ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BasicDataLeafKey, true, availableGas)
if !sufficient {
return 0, false
}
gas += cost

cost, sufficient = ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.CodeHashLeafKey, true, availableGas-cost)
if !sufficient {
return 0, false
}
gas += cost

return gas, true
}

// AddTxOrigin adds the member fields of the sender account to the access event list,
Expand All @@ -169,18 +206,28 @@ func (ae *AccessEvents) AddTxDestination(addr common.Address, sendsValue, doesnt
}

// SlotGas returns the amount of gas to be charged for a cold storage access.
func (ae *AccessEvents) SlotGas(addr common.Address, slot common.Hash, isWrite bool, availableGas uint64, chargeWarmCosts bool) uint64 {
// If the available gas is insufficient to cover the total cost, a false flag
// is returned.
func (ae *AccessEvents) SlotGas(addr common.Address, slot common.Hash, isWrite bool, availableGas uint64, chargeWarmCosts bool) (uint64, bool) {
treeIndex, subIndex := utils.StorageIndex(slot.Bytes())
_, expected := ae.touchAddressAndChargeGas(addr, *treeIndex, subIndex, isWrite, availableGas)
if expected == 0 && chargeWarmCosts {
expected = params.WarmStorageReadCostEIP2929
cost, sufficient := ae.touchAddressAndChargeGas(addr, *treeIndex, subIndex, isWrite, availableGas)
if !sufficient {
return 0, false
}
if cost == 0 && chargeWarmCosts {
if availableGas < params.WarmStorageReadCostEIP2929 {
return 0, false
}
cost = params.WarmStorageReadCostEIP2929
}
return expected
return cost, true
}

// touchAddressAndChargeGas adds any missing access event to the access event list, and returns the
// consumed and required gas.
func (ae *AccessEvents) touchAddressAndChargeGas(addr common.Address, treeIndex uint256.Int, subIndex byte, isWrite bool, availableGas uint64) (uint64, uint64) {
// touchAddressAndChargeGas adds a missing access event to the access list, if needed,
// and returns the cold access cost to be charged. Additionally, it returns a flag
// indicating whether there is enough available gas to cover the cost. If not,
// the event will not be included.
func (ae *AccessEvents) touchAddressAndChargeGas(addr common.Address, treeIndex uint256.Int, subIndex byte, isWrite bool, availableGas uint64) (uint64, bool) {
branchKey := newBranchAccessKey(addr, treeIndex)
chunkKey := newChunkAccessKey(branchKey, subIndex)

Expand Down Expand Up @@ -224,8 +271,7 @@ func (ae *AccessEvents) touchAddressAndChargeGas(addr common.Address, treeIndex
}

if availableGas < gas {
// consumed != expected
return availableGas, gas
return 0, false
}

if branchRead {
Expand All @@ -241,8 +287,8 @@ func (ae *AccessEvents) touchAddressAndChargeGas(addr common.Address, treeIndex
ae.chunks[chunkKey] |= AccessWitnessWriteFlag
}

// consumed == expected
return gas, gas
// consumed == wanted
return gas, true
}

type branchAccessKey struct {
Expand All @@ -269,16 +315,18 @@ func newChunkAccessKey(branchKey branchAccessKey, leafKey byte) chunkAccessKey {
return lk
}

// CodeChunksRangeGas is a helper function to touch every chunk in a code range and charge witness gas costs
func (ae *AccessEvents) CodeChunksRangeGas(contractAddr common.Address, startPC, size uint64, codeLen uint64, isWrite bool, availableGas uint64) (uint64, uint64) {
// note that in the case where the copied code is outside the range of the
// CodeChunksRangeGas is a helper function to touch every chunk in a code range
// and charge witness gas costs. If the available gas is insufficient to cover
// the total cost, a false flag is returned.
func (ae *AccessEvents) CodeChunksRangeGas(contractAddr common.Address, startPC, size uint64, codeLen uint64, isWrite bool, availableGas uint64) (uint64, bool) {
// Note that in the case where the copied code is outside the range of the
// contract code but touches the last leaf with contract code in it,
// we don't include the last leaf of code in the AccessWitness. The
// we don't include the last leaf of code in the AccessWitness. The
// reason that we do not need the last leaf is the account's code size
// is already in the AccessWitness so a stateless verifier can see that
// the code from the last leaf is not needed.
if (codeLen == 0 && size == 0) || startPC > codeLen {
return 0, 0
return 0, true
}

endPC := startPC + size
Expand All @@ -293,48 +341,55 @@ func (ae *AccessEvents) CodeChunksRangeGas(contractAddr common.Address, startPC,
for chunkNumber := startPC / 31; chunkNumber <= endPC/31; chunkNumber++ {
treeIndex := *uint256.NewInt((chunkNumber + 128) / 256)
subIndex := byte((chunkNumber + 128) % 256)
consumed, expected := ae.touchAddressAndChargeGas(contractAddr, treeIndex, subIndex, isWrite, availableGas)
cost, sufficient := ae.touchAddressAndChargeGas(contractAddr, treeIndex, subIndex, isWrite, availableGas)
// did we OOG ?
if expected > consumed {
return statelessGasCharged + consumed, statelessGasCharged + expected
if !sufficient {
return 0, false
}
var overflow bool
statelessGasCharged, overflow = math.SafeAdd(statelessGasCharged, consumed)
statelessGasCharged, overflow = math.SafeAdd(statelessGasCharged, cost)
if overflow {
//return 0, false
panic("overflow when adding gas")
}
availableGas -= consumed
availableGas -= cost
}
return statelessGasCharged, statelessGasCharged
return statelessGasCharged, true
}

// BasicDataGas adds the account's basic data to the accessed data, and returns the
// amount of gas that it costs.
// Note that an access in write mode implies an access in read mode, whereas an
// access in read mode does not imply an access in write mode.
func (ae *AccessEvents) BasicDataGas(addr common.Address, isWrite bool, availableGas uint64, chargeWarmCosts bool) uint64 {
_, expected := ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BasicDataLeafKey, isWrite, availableGas)
if expected == 0 && chargeWarmCosts {
func (ae *AccessEvents) BasicDataGas(addr common.Address, isWrite bool, availableGas uint64, chargeWarmCosts bool) (uint64, bool) {
cost, sufficient := ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BasicDataLeafKey, isWrite, availableGas)
if !sufficient {
return 0, false
}
if cost == 0 && chargeWarmCosts {
if availableGas < params.WarmStorageReadCostEIP2929 {
return availableGas
return 0, false
}
expected = params.WarmStorageReadCostEIP2929
cost = params.WarmStorageReadCostEIP2929
}
return expected
return cost, true
}

// CodeHashGas adds the account's code hash to the accessed data, and returns the
// amount of gas that it costs.
// in write mode. If false, the charged gas corresponds to an access in read mode.
// Note that an access in write mode implies an access in read mode, whereas an access in
// read mode does not imply an access in write mode.
func (ae *AccessEvents) CodeHashGas(addr common.Address, isWrite bool, availableGas uint64, chargeWarmCosts bool) uint64 {
_, expected := ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.CodeHashLeafKey, isWrite, availableGas)
if expected == 0 && chargeWarmCosts {
func (ae *AccessEvents) CodeHashGas(addr common.Address, isWrite bool, availableGas uint64, chargeWarmCosts bool) (uint64, bool) {
cost, sufficient := ae.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.CodeHashLeafKey, isWrite, availableGas)
if !sufficient {
return 0, false
}
if cost == 0 && chargeWarmCosts {
if availableGas < params.WarmStorageReadCostEIP2929 {
return availableGas
return 0, false
}
expected = params.WarmStorageReadCostEIP2929
cost = params.WarmStorageReadCostEIP2929
}
return expected
return cost, true
}
24 changes: 12 additions & 12 deletions core/state/access_events_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,50 +41,50 @@ func TestAccountHeaderGas(t *testing.T) {
ae := NewAccessEvents(utils.NewPointCache(1024))

// Check cold read cost
gas := ae.BasicDataGas(testAddr, false, math.MaxUint64, false)
gas, _ := ae.BasicDataGas(testAddr, false, math.MaxUint64, false)
if want := params.WitnessBranchReadCost + params.WitnessChunkReadCost; gas != want {
t.Fatalf("incorrect gas computed, got %d, want %d", gas, want)
}

// Check warm read cost
gas = ae.BasicDataGas(testAddr, false, math.MaxUint64, false)
gas, _ = ae.BasicDataGas(testAddr, false, math.MaxUint64, false)
Copy link
Owner

@gballet gballet Apr 11, 2025

Choose a reason for hiding this comment

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

if a new parameter is added that could indicate an issue, it should be checked in tests.

if gas != 0 {
t.Fatalf("incorrect gas computed, got %d, want %d", gas, 0)
}

// Check cold read costs in the same group no longer incur the branch read cost
gas = ae.CodeHashGas(testAddr, false, math.MaxUint64, false)
gas, _ = ae.CodeHashGas(testAddr, false, math.MaxUint64, false)
if gas != params.WitnessChunkReadCost {
t.Fatalf("incorrect gas computed, got %d, want %d", gas, params.WitnessChunkReadCost)
}

// Check cold write cost
gas = ae.BasicDataGas(testAddr, true, math.MaxUint64, false)
gas, _ = ae.BasicDataGas(testAddr, true, math.MaxUint64, false)
if want := params.WitnessBranchWriteCost + params.WitnessChunkWriteCost; gas != want {
t.Fatalf("incorrect gas computed, got %d, want %d", gas, want)
}

// Check warm write cost
gas = ae.BasicDataGas(testAddr, true, math.MaxUint64, false)
gas, _ = ae.BasicDataGas(testAddr, true, math.MaxUint64, false)
if gas != 0 {
t.Fatalf("incorrect gas computed, got %d, want %d", gas, 0)
}

// Check a write without a read charges both read and write costs
gas = ae.BasicDataGas(testAddr2, true, math.MaxUint64, false)
gas, _ = ae.BasicDataGas(testAddr2, true, math.MaxUint64, false)
if want := params.WitnessBranchReadCost + params.WitnessBranchWriteCost + params.WitnessChunkWriteCost + params.WitnessChunkReadCost; gas != want {
t.Fatalf("incorrect gas computed, got %d, want %d", gas, want)
}

// Check that a write followed by a read charges nothing
gas = ae.BasicDataGas(testAddr2, false, math.MaxUint64, false)
gas, _ = ae.BasicDataGas(testAddr2, false, math.MaxUint64, false)
if gas != 0 {
t.Fatalf("incorrect gas computed, got %d, want %d", gas, 0)
}

// Check that reading a slot from the account header only charges the
// chunk read cost.
gas = ae.SlotGas(testAddr, common.Hash{}, false, math.MaxUint64, false)
gas, _ = ae.SlotGas(testAddr, common.Hash{}, false, math.MaxUint64, false)
if gas != params.WitnessChunkReadCost {
t.Fatalf("incorrect gas computed, got %d, want %d", gas, params.WitnessChunkReadCost)
}
Expand Down Expand Up @@ -119,23 +119,23 @@ func TestMessageCallGas(t *testing.T) {
ae := NewAccessEvents(utils.NewPointCache(1024))

// Check cold read cost, without a value
gas := ae.MessageCallGas(testAddr, math.MaxUint64)
gas, _ := ae.MessageCallGas(testAddr, math.MaxUint64)
if want := params.WitnessBranchReadCost + params.WitnessChunkReadCost; gas != want {
t.Fatalf("incorrect gas computed, got %d, want %d", gas, want)
}

// Check that reading the basic data and code hash of the same account does not incur the branch read cost
gas = ae.BasicDataGas(testAddr, false, math.MaxUint64, false)
gas, _ = ae.BasicDataGas(testAddr, false, math.MaxUint64, false)
if gas != 0 {
t.Fatalf("incorrect gas computed, got %d, want %d", gas, 0)
}
gas = ae.CodeHashGas(testAddr, false, math.MaxUint64, false)
gas, _ = ae.CodeHashGas(testAddr, false, math.MaxUint64, false)
if gas != params.WitnessChunkReadCost {
t.Fatalf("incorrect gas computed, got %d, want %d", gas, 0)
}

// Check warm read cost
gas = ae.MessageCallGas(testAddr, math.MaxUint64)
gas, _ = ae.MessageCallGas(testAddr, math.MaxUint64)
if gas != params.WarmStorageReadCostEIP2929 {
t.Fatalf("incorrect gas computed, got %d, want %d", gas, params.WarmStorageReadCostEIP2929)
}
Expand Down
Loading