fix: ignore the gas price for noop context so that gas can be zero#25919
fix: ignore the gas price for noop context so that gas can be zero#25919joshmarinacci wants to merge 7 commits into
Conversation
add unit test Signed-off-by: Josh Marinacci <joshua@marinacci.org>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #25919 +/- ##
============================================
- Coverage 70.34% 70.34% -0.01%
+ Complexity 11841 11838 -3
============================================
Files 2627 2627
Lines 110309 110309
Branches 12109 12109
============================================
- Hits 77602 77597 -5
- Misses 28693 28694 +1
- Partials 4014 4018 +4
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
tinker-michaelj
left a comment
tinker-michaelj
left a comment
There was a problem hiding this comment.
LGTM, tyvm @joshmarinacci !
|
|
||
| public boolean isNoopGasContext() { | ||
| return staticCall || gasPrice == 0; | ||
| return staticCall; |
There was a problem hiding this comment.
This method now means static/query context rather than no gas accounting context. Can we either rename it or add a short javadoc?
There was a problem hiding this comment.
good point. I'll rename it to isStaticCall().
Neeharika-Sompalli
left a comment
Neeharika-Sompalli
left a comment
There was a problem hiding this comment.
LGTM! Thanks @joshmarinacci
| @@ -38,7 +38,7 @@ public BlockValues blockValuesOf(final long gasLimit) { | |||
| } | |||
|
|
|||
| public boolean isNoopGasContext() { | |||
There was a problem hiding this comment.
Codex review on this PR suggest to validate these points:
The change from staticCall || gasPrice == 0 to just staticCall means zero-tinybar gas price transactions now go through the normal charging path with zero amounts.
- Ethereum transaction signer nonce: before, zero gas price returned before sender.incrementNonce(); now it increments. This affects later WRONG_NONCE checks, aliased account nonce, and newSenderNonce/signer nonce in records.
- Intrinsic gas validation: zero-price transactions now still require gasLimit >= intrinsicGas; before that check was skipped.
- Zero-amount gas events: collectGasFee(..., 0, withNonceIncrement) and refunds can now be recorded/replayed. Amounts net to zero, but the nonce increment flag matters, especially rollback handling.
- Value balance checks: normal charging paths validate sender balance against value even when gas cost is zero, so failure status/timing may change for underfunded value transfers.
There was a problem hiding this comment.
Thank you @Neeharika-Sompalli for this input.
- Ethereum transaction signer nonce: before, zero gas price returned before sender.incrementNonce(); now it increments. This affects later WRONG_NONCE checks, aliased account nonce, and newSenderNonce/signer nonce in records.
- sounds like exactly the goal (as I understand)
- Intrinsic gas validation: zero-price transactions now still require gasLimit >= intrinsicGas; before that check was skipped.
- sound reasonable
- Zero-amount gas events: collectGasFee(..., 0, withNonceIncrement) and refunds can now be recorded/replayed. Amounts net to zero, but the nonce increment flag matters, especially rollback handling.
- not sure on this one. Seems also ok if we use 0 gasPrice transactions and expect the nonce to increment.
- Value balance checks: normal charging paths validate sender balance against value even when gas cost is zero, so failure status/timing may change for underfunded value transfers.
- sound reasonable
| } | ||
|
|
||
| @Test | ||
| void zeroPriceGasDoesNoChargingWorkButDoesReturnIntrinsicGas() { |
There was a problem hiding this comment.
Are we sure we want to just remove this test? Maybe adjust it to match the new rules and add a comment explaining what is checked?
There was a problem hiding this comment.
That's a good point. I'll add it back and modify it.
|
|
add back the unit test Signed-off-by: Josh Marinacci <joshua@marinacci.org>
swirlds-automation
dismissed stale reviews from tinker-michaelj and Neeharika-Sompalli
3 file(s) changed in commit f688a84
✅ All tests passed ✅🏷️ Commit: f688a84 Learn more about TestLens at testlens.app. |
6 participants
Description:
The Single Day Performance Test (SDPT) is failing because it sets the price of the GAS extra to 1. Under Simple Fees all values are in tiny cents, so a fee of 1 will get rounded to 0 when it is converted to tiny bars at a rate of 12 to 1. This triggers isNoopGasContext() to return true even though the price of gas is really more than zero.
This PR fixes the SDPT failure on a gas price of 1 tinycent by changing
HederaEvmContext.isNoopGasContext()to not care about the price of gas, only if it is a static call.Related issue(s):
Fixes #
Notes for reviewer:
Checklist