implement FILL_COSTS on top of devnet7 #524
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,8 @@ | |
| package state | ||
|
|
||
| import ( | ||
| "maps" | ||
|
|
||
| "github.com/ethereum/go-ethereum/common" | ||
| "github.com/ethereum/go-ethereum/common/math" | ||
| "github.com/ethereum/go-ethereum/params" | ||
|
|
@@ -42,14 +44,16 @@ var zeroTreeIndex uint256.Int | |
| type AccessWitness struct { | ||
| branches map[branchAccessKey]mode | ||
| chunks map[chunkAccessKey]mode | ||
| fills map[chunkAccessKey]struct{} | ||
|
|
||
| pointCache *utils.PointCache | ||
| } | ||
|
|
||
| func NewAccessWitness(pointCache *utils.PointCache, fills map[chunkAccessKey]struct{}) *AccessWitness { | ||
| return &AccessWitness{ | ||
| branches: make(map[branchAccessKey]mode), | ||
| chunks: make(map[chunkAccessKey]mode), | ||
| fills: fills, | ||
|
Comment on lines
+52
to
+56
There was a problem hiding this comment. First reaction looking at this, is why you'd like to initialize a witness with a pre-filled map. My intuition this is related with This might add up to the previous comment I did why I think (Note: not saying we should do this in this PR or similar -- mostly talking about long term strategies) There was a problem hiding this comment. the difference is that writes are per-tx while the fills are per-block. Not sure the stateedb can always provide us with this info, I'll have to think about it when rewriting. |
||
| pointCache: pointCache, | ||
| } | ||
| } | ||
|
|
@@ -64,6 +68,9 @@ func (aw *AccessWitness) Merge(other *AccessWitness) { | |
| for k, chunk := range other.chunks { | ||
| aw.chunks[k] |= chunk | ||
| } | ||
| for k := range other.fills { | ||
| aw.fills[k] = struct{}{} | ||
| } | ||
| } | ||
|
|
||
| // Key returns, predictably, the list of keys that were touched during the | ||
|
|
@@ -81,18 +88,18 @@ func (aw *AccessWitness) Keys() [][]byte { | |
|
|
||
| func (aw *AccessWitness) Copy() *AccessWitness { | ||
| naw := &AccessWitness{ | ||
| branches: maps.Clone(aw.branches), | ||
| chunks: maps.Clone(aw.chunks), | ||
| fills: maps.Clone(aw.fills), | ||
| pointCache: aw.pointCache, | ||
| } | ||
| return naw | ||
| } | ||
|
|
||
| func (aw *AccessWitness) TouchFullAccount(addr []byte, isWrite, isFill bool, availableGas uint64) uint64 { | ||
|
There was a problem hiding this comment. As mentioned before: assuming the caller always is write about knowing the tree doesn't have the value, then I guess this is fine. There was a problem hiding this comment. Note this also shows as fundamental flaw in the API design: someone calling I think I had said this before too and suggested maybe a flag-ish style API? Like There was a problem hiding this comment. Maybe we could add: or similar. There was a problem hiding this comment. you can't charge fill costs in a state read, if write is false. |
||
| var gas uint64 | ||
| for i := utils.BasicDataLeafKey; i <= utils.CodeHashLeafKey; i++ { | ||
| consumed, wanted := aw.touchAddressAndChargeGas(addr, zeroTreeIndex, byte(i), isWrite, isFill, availableGas) | ||
| if consumed < wanted { | ||
| return wanted + gas | ||
| } | ||
|
|
@@ -103,19 +110,23 @@ func (aw *AccessWitness) TouchFullAccount(addr []byte, isWrite bool, availableGa | |
| } | ||
|
|
||
| func (aw *AccessWitness) TouchAndChargeMessageCall(addr []byte, availableGas uint64) uint64 { | ||
| _, wanted := aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BasicDataLeafKey, false, false, availableGas) | ||
| if wanted == 0 { | ||
| wanted = params.WarmStorageReadCostEIP2929 | ||
| } | ||
| return wanted | ||
| } | ||
|
|
||
| func (aw *AccessWitness) TouchAndChargeValueTransfer(callerAddr, targetAddr []byte, isFill bool, availableGas uint64) uint64 { | ||
| _, wanted1 := aw.touchAddressAndChargeGas(callerAddr, zeroTreeIndex, utils.BasicDataLeafKey, true, false, availableGas) | ||
| if wanted1 > availableGas { | ||
| return wanted1 | ||
| } | ||
| // The isFill here is a bit strange: one would have to pay for the fill costs | ||
| // and potentially run out of gas before the account is created. This is due | ||
| // to the charging for a potential account creation being split in two locations: | ||
| // the basic data is written in the gas function, and the code hash in opCall. | ||
| _, wanted2 := aw.touchAddressAndChargeGas(targetAddr, zeroTreeIndex, utils.BasicDataLeafKey, true, isFill, availableGas-wanted1) | ||
|
Comment on lines
+125
to
+129
There was a problem hiding this comment. @jsign this is the comment describing the situation we just discussed. I think it's terrible. Let me know if you have a better way to describe this, otherwise I'll take another shot at it tomorrow. There was a problem hiding this comment. Yeah, I see. I don't think is that bad. If I look at this method in a vaccum, what is doing seems logical compared to how the API is proposed. i.e: the |
||
| if wanted1+wanted2 == 0 { | ||
| return params.WarmStorageReadCostEIP2929 | ||
| } | ||
|
|
@@ -127,45 +138,45 @@ func (aw *AccessWitness) TouchAndChargeValueTransfer(callerAddr, targetAddr []by | |
| // address collision is done before the transfer, and so no write | ||
| // are guaranteed to happen at this point. | ||
| func (aw *AccessWitness) TouchAndChargeContractCreateCheck(addr []byte, availableGas uint64) uint64 { | ||
| gas1, wanted1 := aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BasicDataLeafKey, false, false, availableGas) | ||
| _, wanted2 := aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.CodeHashLeafKey, false, false, availableGas-gas1) | ||
| return wanted1 + wanted2 | ||
| } | ||
|
|
||
| // TouchAndChargeContractCreateInit charges access costs to initiate | ||
| // a contract creation. | ||
| func (aw *AccessWitness) TouchAndChargeContractCreateInit(addr []byte, availableGas uint64, isFill bool) (uint64, uint64) { | ||
| gas1, wanted1 := aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BasicDataLeafKey, true, isFill, availableGas) | ||
| gas2, wanted2 := aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.CodeHashLeafKey, true, isFill, availableGas-gas1) | ||
| return gas1 + gas2, wanted1 + wanted2 | ||
| } | ||
|
|
||
| func (aw *AccessWitness) TouchTxOriginAndComputeGas(originAddr []byte) { | ||
| for i := utils.BasicDataLeafKey; i <= utils.CodeHashLeafKey; i++ { | ||
| aw.touchAddressAndChargeGas(originAddr, zeroTreeIndex, byte(i), i == utils.BasicDataLeafKey, false, math.MaxUint64) | ||
| } | ||
| } | ||
|
|
||
| func (aw *AccessWitness) TouchTxTarget(targetAddr []byte, sendsValue, doesntExist bool) { | ||
| aw.touchAddressAndChargeGas(targetAddr, zeroTreeIndex, utils.BasicDataLeafKey, sendsValue, doesntExist, math.MaxUint64) | ||
| // Note that we do a write-event in CodeHash without distinguishing if the tx target account | ||
| // exists or not. Pre-7702, there's no situation in which an existing codeHash can be mutated, thus | ||
| // doing a write-event shouldn't cause an observable difference in gas usage. | ||
| // TODO(7702): re-check this in the spec and implementation to be sure is a correct solution after | ||
| // EIP-7702 is implemented. | ||
| aw.touchAddressAndChargeGas(targetAddr, zeroTreeIndex, utils.CodeHashLeafKey, doesntExist, doesntExist, math.MaxUint64) | ||
| } | ||
|
|
||
| func (aw *AccessWitness) TouchSlotAndChargeGas(addr []byte, slot common.Hash, isWrite, isFill bool, availableGas uint64, warmCostCharging bool) uint64 { | ||
| treeIndex, subIndex := utils.GetTreeKeyStorageSlotTreeIndexes(slot.Bytes()) | ||
| _, wanted := aw.touchAddressAndChargeGas(addr, *treeIndex, subIndex, isWrite, isFill, availableGas) | ||
| if wanted == 0 && warmCostCharging { | ||
| wanted = params.WarmStorageReadCostEIP2929 | ||
| } | ||
| return wanted | ||
| } | ||
|
|
||
| func (aw *AccessWitness) touchAddressAndChargeGas(addr []byte, treeIndex uint256.Int, subIndex byte, isWrite, isFill bool, availableGas uint64) (uint64, uint64) { | ||
| branchKey := newBranchAccessKey(addr, treeIndex) | ||
| chunkKey := newChunkAccessKey(branchKey, subIndex) | ||
|
|
||
|
|
@@ -189,6 +200,11 @@ func (aw *AccessWitness) touchAddressAndChargeGas(addr []byte, treeIndex uint256 | |
| if (chunkValue & AccessWitnessWriteFlag) == 0 { | ||
| chunkWrite = true | ||
| } | ||
|
|
||
| _, ok := aw.fills[chunkKey] | ||
| if isFill && !ok { | ||
| chunkFill = true | ||
| } | ||
| } | ||
|
|
||
| var gas uint64 | ||
|
|
@@ -225,6 +241,9 @@ func (aw *AccessWitness) touchAddressAndChargeGas(addr []byte, treeIndex uint256 | |
| if chunkWrite { | ||
| aw.chunks[chunkKey] |= AccessWitnessWriteFlag | ||
| } | ||
| if chunkFill { | ||
| aw.fills[chunkKey] = struct{}{} | ||
| } | ||
|
|
||
| // consumed == wanted | ||
| return gas, gas | ||
|
|
@@ -255,7 +274,7 @@ func newChunkAccessKey(branchKey branchAccessKey, leafKey byte) chunkAccessKey { | |
| } | ||
|
|
||
| // touchCodeChunksRangeOnReadAndChargeGas is a helper function to touch every chunk in a code range and charge witness gas costs | ||
| func (aw *AccessWitness) TouchCodeChunksRangeAndChargeGas(contractAddr []byte, startPC, size uint64, codeLen uint64, isWrite, isFill bool, availableGas uint64) (uint64, uint64) { | ||
| // 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 | ||
|
|
@@ -278,7 +297,7 @@ func (aw *AccessWitness) TouchCodeChunksRangeAndChargeGas(contractAddr []byte, s | |
| for chunkNumber := startPC / 31; chunkNumber <= endPC/31; chunkNumber++ { | ||
| treeIndex := *uint256.NewInt((chunkNumber + 128) / 256) | ||
| subIndex := byte((chunkNumber + 128) % 256) | ||
| consumed, wanted := aw.touchAddressAndChargeGas(contractAddr, treeIndex, subIndex, isWrite, isFill, availableGas) | ||
| // did we OOG ? | ||
| if wanted > consumed { | ||
| return statelessGasCharged + consumed, statelessGasCharged + wanted | ||
|
|
@@ -294,8 +313,8 @@ func (aw *AccessWitness) TouchCodeChunksRangeAndChargeGas(contractAddr []byte, s | |
| return statelessGasCharged, statelessGasCharged | ||
| } | ||
|
|
||
| func (aw *AccessWitness) TouchBasicData(addr []byte, isWrite, isFill bool, availableGas uint64, warmCostCharging bool) uint64 { | ||
|
There was a problem hiding this comment. Feel some tension here between Not saying this is a huge problem... just feels a bit weird. There was a problem hiding this comment. agreed, not a problem but we're starting to accumulate booleans and that creates for non-sensical situations that should never happen ™️ but might be triggered at some point |
||
| _, wanted := aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BasicDataLeafKey, isWrite, isFill, availableGas) | ||
| if wanted == 0 && warmCostCharging { | ||
| if availableGas < params.WarmStorageReadCostEIP2929 { | ||
| return availableGas | ||
|
|
@@ -306,7 +325,7 @@ func (aw *AccessWitness) TouchBasicData(addr []byte, isWrite bool, availableGas | |
| } | ||
|
|
||
| func (aw *AccessWitness) TouchCodeHash(addr []byte, isWrite bool, availableGas uint64, chargeWarmCosts bool) uint64 { | ||
| _, wanted := aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.CodeHashLeafKey, isWrite, false, availableGas) | ||
| if wanted == 0 && chargeWarmCosts { | ||
| if availableGas < params.WarmStorageReadCostEIP2929 { | ||
| return availableGas | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code line already shows something evident in the proposed
isFill boolAPI.Every caller should decide if the witness addition should charge a fill cost, which seems odd unless every caller knows if the underlying tree has a "hole" in the tree at that place.
For example, for this line, it doesn't seem obvious why a withdrawal would need to charge fill cost unconditionally since that seems to depend on runtime conditions. In this case isn't that relevant since we aren't charging that to anyone -- but just feels odd.
The much nicer alternative is not including this
isFill boolin the APIs, and letAccessWitnessfigure out if the write should charge fill cost, probably assisted bystatedb.I think I mentioned this a while ago when we discussed about FILL_COST, so shouldn't be a big surprise this thought came again now -- resharing just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that is right, we would save some typing for sure. This is meant for measurements so I won't do it before devcon, but you are correct that we should do it this way.