Fill fixes continued #519
Merged
Fill fixes continued #519
Conversation
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
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
jsign
commented
Oct 24, 2024
| @@ -4,7 +4,8 @@ on: | |||
| push: | |||
| branches: [master, kaustinen-with-shapella] | |||
| pull_request: | |||
| branches: [master, kaustinen-with-shapella] | |||
| branches: | |||
There was a problem hiding this comment.
Remove this diff when the PR is about to be merged.
| @@ -4,7 +4,8 @@ on: | |||
| push: | |||
| branches: [master] | |||
| pull_request: | |||
| branches: [master, kaustinen-with-shapella] | |||
| branches: | |||
| ringIndex := number % params.Eip2935BlockHashHistorySize | ||
| var pnum common.Hash | ||
| binary.BigEndian.PutUint64(pnum[24:], ringIndex) | ||
| statelessGas := witness.TouchSlotAndChargeGas(params.HistoryStorageAddress[:], pnum, false, math.MaxUint64, false) |
There was a problem hiding this comment.
This was incorrect -- we don't have infinite amount of gas. This method is only called from opBlockHash. If we don't pass the right limit, even if you ran OOG it would always add it to the witness (when it shouldn't).
| @@ -111,7 +111,12 @@ func gasSelfdestructEIP4762(evm *EVM, contract *Contract, stack *Stack, mem *Mem | |||
| beneficiaryAddr := common.Address(stack.peek().Bytes20()) | |||
| contractAddr := contract.Address() | |||
|
|
|||
| statelessGas := evm.Accesses.TouchBasicData(contractAddr[:], false, contract.Gas, false) | |||
There was a problem hiding this comment.
TL;DR: everything in SELFDESTRUCT was ignoring if we had enough gas.
Comment on lines
-161
to
+175
| wgas := evm.Accesses.TouchBasicData(addr[:], false, contract.Gas, true) | ||
| wgas := evm.Accesses.TouchBasicData(addr[:], false, contract.Gas-gas, true) |
There was a problem hiding this comment.
We where using more gas than truly available.
gballet
reviewed
Oct 24, 2024
| if wanted > contract.Gas { | ||
| return wanted, nil | ||
| } | ||
| statelessGas := wanted |
There was a problem hiding this comment.
we could change TouchBasicData to return both the wanted and the actually charged
gballet
reviewed
Oct 24, 2024
| if wanted > contract.Gas-statelessGas { | ||
| return statelessGas + wanted, nil | ||
| } | ||
| statelessGas += wanted |
There was a problem hiding this comment.
right, but it would only save one line and not be used for anything afterwards. No point in making the PR footprint larger for that.
gballet
reviewed
Oct 24, 2024
gballet
reviewed
Oct 24, 2024
remove diff used for test
gballet
reviewed
Oct 24, 2024
gballet
added a commit
that referenced
this pull request
Oct 24, 2024
* state: access witness with partial cost charging Signed-off-by: Ignacio Hagopian <[email protected]> * fix most problems Signed-off-by: Ignacio Hagopian <[email protected]> * rebase fixes Signed-off-by: Ignacio Hagopian <[email protected]> * fixes Signed-off-by: Ignacio Hagopian <[email protected]> * fixes Signed-off-by: Ignacio Hagopian <[email protected]> * . Signed-off-by: Ignacio Hagopian <[email protected]> * remove 4762 gas call wrappers Signed-off-by: Ignacio Hagopian <[email protected]> * more progress Signed-off-by: Ignacio Hagopian <[email protected]> * call gas Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * simplify warm costs Signed-off-by: Ignacio Hagopian <[email protected]> * cleanup Signed-off-by: Ignacio Hagopian <[email protected]> * commit statedb Signed-off-by: Ignacio Hagopian <[email protected]> * pass upper bound for gas consumption Signed-off-by: Guillaume Ballet <[email protected]> * fix missing gas sum Signed-off-by: Ignacio Hagopian <[email protected]> * fix: warm storage cost for basic data and code hash * fix: return wanted in TouchBasicData gas functions * fix Signed-off-by: Ignacio Hagopian <[email protected]> * fix: add history contract to TestProcessorVerkle * ci: new workflows for testing (#504) * ci: new workflows for testing Signed-off-by: Ignacio Hagopian <[email protected]> * nit Signed-off-by: Ignacio Hagopian <[email protected]> * ci: fix stable consumption Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * more fixes Signed-off-by: Ignacio Hagopian <[email protected]> * more fixes Signed-off-by: Ignacio Hagopian <[email protected]> * update tag Signed-off-by: Ignacio Hagopian <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]> * import fixes from base branch until 51dca93 Signed-off-by: Guillaume Ballet <[email protected]> * fix: create init order redux (#510) Signed-off-by: Guillaume Ballet <[email protected]> * import most changes from jsign-witness-fix Co-authored-by: Ignacio Hagopian <[email protected]> Signed-off-by: Guillaume Ballet <[email protected]> * rebase nits Signed-off-by: Ignacio Hagopian <[email protected]> * fix: move 1/64 before gas call * fix: value transfer gas charge before 1/64th + warm costs for precomp + system contracts Signed-off-by: Guillaume Ballet <[email protected]> * Filling fixes (#516) * fix makeCallVariantGasEIP4762 Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * ci: target v0.0.6 Signed-off-by: Ignacio Hagopian <[email protected]> * improve fix Signed-off-by: Ignacio Hagopian <[email protected]> * add witness gas to "old" gas upon error Signed-off-by: Guillaume Ballet <[email protected]> * workflow: remove target branch from list of branches --------- Signed-off-by: Ignacio Hagopian <[email protected]> Signed-off-by: Guillaume Ballet <[email protected]> Co-authored-by: Guillaume Ballet <[email protected]> * nit Signed-off-by: Ignacio Hagopian <[email protected]> * fix compilation error Signed-off-by: Ignacio Hagopian <[email protected]> * Fill fixes continued (#519) * fix selfdestruct Signed-off-by: Ignacio Hagopian <[email protected]> * extcodecopy witness fix Signed-off-by: Ignacio Hagopian <[email protected]> * blockhash fix Signed-off-by: Ignacio Hagopian <[email protected]> * run ci Signed-off-by: Ignacio Hagopian <[email protected]> * Apply suggestions from code review remove diff used for test * same thing in other file --------- Signed-off-by: Ignacio Hagopian <[email protected]> Co-authored-by: Guillaume Ballet <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]> Signed-off-by: Guillaume Ballet <[email protected]> Co-authored-by: Ignacio Hagopian <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR fixes other filling failures.