Align gas cap calculation for transaction simulation to Geth approach #7703
Conversation
fab-10
changed the title
fab-10
force-pushed
the
align-gas-cap-simulation-to-geth
branch
from
0278348
to
6a302c8
Compare
| final long simulationGasCap; | ||
|
|
||
| // when not set gas limit is -1 | ||
| if (callParams.getGasLimit() >= 0) { |
There was a problem hiding this comment.
Do you think it's big and focused enough to factor it into a discrete method?
fab-10
force-pushed
the
align-gas-cap-simulation-to-geth
branch
from
6a302c8
to
6235c1f
Compare
| @@ -291,6 +283,38 @@ public Optional<TransactionSimulatorResult> processWithWorldUpdater( | |||
| return Optional.of(new TransactionSimulatorResult(transaction, result)); | |||
| } | |||
|
|
|||
| private long calculateSimulationGasCap( | |||
| final CallParameter callParams, final BlockHeader blockHeaderToProcess) { | |||
There was a problem hiding this comment.
Sorry to come back to you on this, but rather than passing whole CallParameter and BlockHeader, we can pass gas limits only, it would make it even more focused
There was a problem hiding this comment.
no worries, it makes sense again.
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
fab-10
force-pushed
the
align-gas-cap-simulation-to-geth
branch
from
6235c1f
to
3e5ed76
Compare
|
|
||
| // when not set gas limit is -1 | ||
| if (callParams.getGasLimit() >= 0) { | ||
| if (rpcGasCap > 0 && callParams.getGasLimit() > rpcGasCap) { |
There was a problem hiding this comment.
I wonder if it is more clear to use rpcGasCap > 0 as the first test.
Basically, we start by asking if the user have a configured rpcGasCap, and depending on this user choice, we can check other options.
There was a problem hiding this comment.
I understand it could be subjective, I prefer that user provided gas limit drives the choice.
PR description
A recent PR (#6978) made the optional
rpc-gas-capconfig to always override the gas cap used for tx simulation, when specified, but while this could be good foreth_call, it does not work well with calls likeeth_estimateGaswhen the gas cap could be based on what use provided or the sender balance, and it also diverging from what Geth does.This PR align the gas cap calculation used during tx simulation, to what Geth does, with only a little difference, the algorithm is the following:
rpc-gas-cap, in which case we return the latterrpc-gas-capif configured, otherwise use the block gas limit^.^ in this case Geth use
MaxUint64 / 2Fixed Issue(s)
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