Suppress ComparisonOutOfRange errorprone warning in AbstractGasLimitSpecification #6727
Conversation
Simplify isValidTargetGasLimit bound condition Signed-off-by: Usman Saleem <usman@usmans.info>
| public static boolean isValidTargetGasLimit(final long targetGasLimit) { | ||
| return DEFAULT_MIN_GAS_LIMIT <= targetGasLimit && DEFAULT_MAX_GAS_LIMIT >= targetGasLimit; | ||
| // only check the lower bound as the upper bound is Long.MAX_VALUE | ||
| return targetGasLimit >= DEFAULT_MIN_GAS_LIMIT; |
There was a problem hiding this comment.
Can you point to which tests this is covered by please?
There was a problem hiding this comment.
ethereum/core/src/test/java/org/hyperledger/besu/ethereum/mainnet/TargetingGasLimitCalculatorTest.java
There was a problem hiding this comment.
Thanks. I find this errorprone check odd - wouldn't this code break if we ever changed the DEFAULT_MAX to be something other than Long.MAX_VALUE?
I don't think the test coverage is good enough to pick up on that either.
Better to suppress the warning in this case IMO.
There was a problem hiding this comment.
agree it seems safer to suppress this warning
There was a problem hiding this comment.
To counter-argue, if we jump from long to BigInteger for example, then the targetGasLimit would also change from long to BigInteger. What is the value DEFAULT_GAS_MAX_LIMIT as per the spec? Here it seems to be bounded by the variable type we are using i.e. Long.MAX_VALUE.
I'll close this PR and will add the suppression when we bring in the updated errorprone library dependency.
There was a problem hiding this comment.
Can't find a max value in the spec, was looking here: https://ethereum.github.io/execution-specs/src/ethereum/shanghai/fork.py.html#ethereum.shanghai.fork.check_gas_limit:0
It's UInt so unspecified arbitrary size https://ethereum.github.io/execution-specs/src/ethereum/base_types.py.html
if we ever changed the DEFAULT_MAX to be something other than Long.MAX_VALUE
I was more thinking if a future spec introduced a specific max value to enforce something in the protocol.
There was a problem hiding this comment.
Agreed. I'll add a unit test instead to cover the max value.
Signed-off-by: Usman Saleem <usman@usmans.info>
Signed-off-by: Usman Saleem <usman@usmans.info>
Signed-off-by: Usman Saleem <usman@usmans.info>
Signed-off-by: Usman Saleem <usman@usmans.info>
Signed-off-by: Usman Saleem <usman@usmans.info>
usmansaleem
changed the title
Signed-off-by: Usman Saleem <usman@usmans.info>
…Specification (hyperledger#6727) * Add suppression for ComparisonOutOfRange * test: Add unit test to cover max gas limit --------- Signed-off-by: Usman Saleem <usman@usmans.info> Signed-off-by: amsmota <antonio.mota@citi.com>
…Specification (hyperledger#6727) * Add suppression for ComparisonOutOfRange * test: Add unit test to cover max gas limit --------- Signed-off-by: Usman Saleem <usman@usmans.info> Signed-off-by: amsmota <antonio.mota@citi.com>
…Specification (hyperledger#6727) * Add suppression for ComparisonOutOfRange * test: Add unit test to cover max gas limit --------- Signed-off-by: Usman Saleem <usman@usmans.info>
PR description
Suppress ComparisonOutOfRange error-prone warning in AbstractGasLimitSpecification. This was reported during errorprone plugin update. Add a unit test to cover the max limit as well.
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Most advanced CI tests are deferred until PR approval, but you could:
./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests