Eip 7709 implement gas costs #7983
Conversation
lu-pinto
force-pushed
the
eip-7709-implement-gas-costs
branch
5 times, most recently
from
f41b0e8 to
077723a
Compare
… contract Signed-off-by: Luis Pinto <[email protected]>
lu-pinto
force-pushed
the
eip-7709-implement-gas-costs
branch
from
8dbd810 to
b82953a
Compare
| @@ -81,6 +82,12 @@ public Hash apply(final MessageFrame frame, final Long blockNumber) { | |||
| return ZERO; | |||
| } | |||
|
|
|||
| final long cost = cost(frame, blockNumber); | |||
| if (frame.getRemainingGas() < cost) { | |||
| return Hash.EMPTY; | |||
There was a problem hiding this comment.
is it in the spec ? I see that in the first failure we return ZERO and in this one EMPTY
There was a problem hiding this comment.
not in the spec but this will not surface to consensus because it will be handled properly in BlockHashOperation. This was the only way I found to stop execution if out of gas. Could have an exception if you prefer.
There was a problem hiding this comment.
For precompiles execeptional halts (like this one, out of gas) are handled by returning null rather than Hash.EMPTY, which is as valid a response as any other hash.
There was a problem hiding this comment.
throwing an exception now
| final BlockHashLookup blockHashLookup = frame.getBlockHashLookup(); | ||
| final Hash blockHash = blockHashLookup.apply(frame, blockArg.toLong()); | ||
| final long lookupCost = remainingGas - frame.getRemainingGas(); | ||
| if (Hash.EMPTY.equals(blockHash)) { |
There was a problem hiding this comment.
It seems quite strange to use Hash.EMPTY as a way to determine if there is not enough gas.
There was a problem hiding this comment.
I would recommend null, it matches the way halts are detected in precompiles.
There was a problem hiding this comment.
Hmm I think I prefer the exception approach, with precompiles we never return the ExceptionalHaltReason back in result. This one in particular, is really related to not enough gas, so if in the future another type of problem is identified that needs to be relayed back in OperationResult.
There was a problem hiding this comment.
passing and checking for nulls is faster in a tight loop. There's a good amount of overhead in throwing and catching,.
There was a problem hiding this comment.
I haven't thought about the performance but indeed if the performances are better I agree with Danno
There was a problem hiding this comment.
Ok I can use null, the code is going to look a bit stranger because I'm also using the exception to return back the gas cost on which the check failed on - required to report the exact cost back to the tracers - and now I won't have a clean way to pass that info.
Re: performance concerns - this is a halt condition so the loop will be terminated right after the exception. There is the case of parent frame contexts which could still proceed executing but it's not a new pattern I'm introducing. UnderflowException and OverflowException already do the exact same thing.
… system contract forgot to add code from previous tip Signed-off-by: Luis Pinto <[email protected]>
lu-pinto
force-pushed
the
eip-7709-implement-gas-costs
branch
from
374afe2 to
74aa9fc
Compare
|
Nulls are faster than catch/throw. |
… system contract reimplement insufficient gas cost case with null Signed-off-by: Luis Pinto <[email protected]>
This reverts commit 5f1e6d1.
PR description
Implements gas costs for BLOCKHASH lookup opcode using the hash history system contract
TODO: Remove #7971 and #7778 commits. Grafting of first commits is required to support these changes.Fixed Issue(s)
fixes #7811
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests