Use different transaction simulators for different behavior of rpc gas cap #7691
Conversation
fab-10
force-pushed
the
fix-rpc-gas-cap
branch
from
5381c51
to
0959c98
Compare
fab-10
changed the title
rpc-gas-cap option should not incoditionally override smaller …
fab-10
force-pushed
the
fix-rpc-gas-cap
branch
from
0959c98
to
14d3193
Compare
fab-10
changed the title
rpc-gas-cap option should not incoditionally override smaller …rpc-gas-cap option should not incoditionally override smaller tx gas limit
fab-10
changed the title
rpc-gas-cap option should not incoditionally override smaller tx gas limitrpc-gas-cap option should not inconditionally override smaller tx gas limit
…x gas limit Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
fab-10
force-pushed
the
fix-rpc-gas-cap
branch
from
14d3193
to
513ce1f
Compare
| if (rpcGasCap > 0) { | ||
| gasLimit = rpcGasCap; | ||
| simulationGasLimit = Math.min(txGasLimit, rpcGasCap); |
There was a problem hiding this comment.
There is a real need to make RPC gas limit above block size limit, especially on eth_call. I wonder if we cannot make eth_estimateGas using a different variante of the transaction simulator
There was a problem hiding this comment.
using different transaction simulators is doable, let me try to implement that way
There was a problem hiding this comment.
@ahamlat I have implemented your suggestion and now eth_call behaves differently of other call in regards of how rpc-gas-cap is applied
fab-10
changed the title
rpc-gas-cap option should not inconditionally override smaller tx gas limit
fab-10
force-pushed
the
fix-rpc-gas-cap
branch
from
a7d3eac
to
a521353
Compare
…s cap Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
fab-10
force-pushed
the
fix-rpc-gas-cap
branch
from
a521353
to
d00c324
Compare
| rpcGasCap); | ||
| return simulationGasLimit; | ||
| } | ||
| return txGasLimit; |
There was a problem hiding this comment.
I'm trying to understand the logic here. If rpcGasCap == 0 we return txGasLimit and do not consider txGasCap? If so, can we move the final long txGasCap = txGasLimit >= 0 ? txGasLimit : blockGasLimit to be inside the if?
Also, could you please add a summary to the PR description.
|
|
||
| public TransactionSimulator build() { | ||
| final SimulationGasLimitCalculator simulationGasLimitCalculator; | ||
| if (rpcGasCap > 0 && rpcGasCapMode.equals(RpcGasCapMode.UNBOUNDED)) { |
There was a problem hiding this comment.
a long + enum encodes same amount of information as Long or Optional null or Optional.empty() may mean no bound. Introduction of RpcGasCapMode is verbose, but maybe it makes it more obvious
There was a problem hiding this comment.
However possibility to set it to (0, BOUNDED) makes it errorprone. Optional or a nullable Long is less errorprone from this perspective
| blockchainQueries.getBlockchain(), | ||
| blockchainQueries.getWorldStateArchive(), | ||
| protocolSchedule) | ||
| .rpcGasCap(apiConfiguration.getGasCap(), BOUNDED) |
There was a problem hiding this comment.
Why do we want to use BOUNDED for eth_estimateGas? Isn't linea_estimateGas supposed to behave the same as eth_estimateGas?
| public interface SimulationGasLimitCalculator { | ||
| long calculate(final long txGasLimit, final long blockGasLimit); | ||
|
|
||
| static long bounded(final long txGasLimit, final long blockGasLimit, final long rpcGasCap) { |
There was a problem hiding this comment.
I think if we replace 0 values with Long.MAX_VALUE the logic would be as simple as taking a min over 3 values. Would it be simpler to read?
|
Thanks all for reviewing, but after studying a bit more and looking at what Geth does, I am closing this and changing approach in this new PR #7703 |
…tx gas limit
PR description
Fixed 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