Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suppress ComparisonOutOfRange errorprone warning in AbstractGasLimitSpecification #6727

Merged
merged 7 commits into from
Mar 18, 2024
Merged

Suppress ComparisonOutOfRange errorprone warning in AbstractGasLimitSpecification #6727

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cca7490
Refactor AbstractGasLimitSpecification
usmansaleem Mar 13, 2024
df223fd
Merge remote-tracking branch 'upstream/main' into cq_gaslimitspec
usmansaleem Mar 18, 2024
e8bfa27
test: Add unit test to cover max gas limit
usmansaleem Mar 18, 2024
6d029fc
Add suppression for ComparisonOutOfRange
usmansaleem Mar 18, 2024
d98a6a1
test: Use Long.MAX_VALUE
usmansaleem Mar 18, 2024
b480e29
Merge remote-tracking branch 'upstream/main' into cq_gaslimitspec
usmansaleem Mar 18, 2024
02d00a5
Merge remote-tracking branch 'upstream/main' into cq_gaslimitspec
usmansaleem Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion ...re/src/main/java/org/hyperledger/besu/ethereum/mainnet/AbstractGasLimitSpecification.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,14 @@ public static long deltaBound(final long currentGasLimit) {
return Long.divideUnsigned(currentGasLimit, GAS_LIMIT_BOUND_DIVISOR);
}

/**
* Verify that the target gas limit is within the allowed bounds.
*
* @param targetGasLimit the target gas limit to validate
* @return true if within bounds
*/
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you point to which tests this is covered by please?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ethereum/core/src/test/java/org/hyperledger/besu/ethereum/mainnet/TargetingGasLimitCalculatorTest.java

Copy link
Contributor

@siladu siladu Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree it seems safer to suppress this warning

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@siladu siladu Mar 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I'll add a unit test instead to cover the max value.

}
}
Loading