fix: Fail cleanly on negative gas limit

[kimbor]

Description:
When a contract transaction is submitted with a negative gas limit, we currently throw an exception. Instead we should return INSUFFICIENT_GAS.

Related issue(s):
Fixes #17470

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.00% (target: -1.00%)	✅ 55.56%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (09de95b)	98065	71462	72.87%
Head commit (fe4ca5d)	98083 (+18)	71478 (+16)	72.88% (+0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#17486)	18	10	55.56%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 68.97%. Comparing base (09de95b) to head (fe4ca5d).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...edera/node/app/workflows/ingest/IngestChecker.java	43.75%	8 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #17486   +/-   ##
=========================================
  Coverage     68.96%   68.97%           
- Complexity    22767    22775    +8     
=========================================
  Files          2619     2619           
  Lines         98282    98300   +18     
  Branches      10184    10187    +3     
=========================================
+ Hits          67783    67802   +19     
- Misses        26669    26671    +2     
+ Partials       3830     3827    -3     

Files with missing lines	Coverage Δ
...utils/throttles/GasLimitDeterministicThrottle.java	100.00% <100.00%> (ø)
...edera/node/app/workflows/ingest/IngestChecker.java	88.67% <43.75%> (-3.55%)	⬇️

... and 1 file with indirect coverage changes

[david-bakin-sl]

Questions you could answer if you wish, and a request for some testi for the change to GasLimitDeterministicThrottle.

[david-bakin-sl]

Is this reasonable for it to throw on negative? I traced back one call chain (the one through ThrottleAccumulator.checkAndEnforceThrottle) to QueryWorkflowIMpl.handleQuery which is the first one with a try/catch and it looks like that'll catch it as a Exception, emit an error, and than fail the transaction. Instead of reporting BUSY (which, admittedly, is odd for negative gas, probably should be caught and translated toINSUFFICIENT_GAS). There's another call chain too, I didn't follow it to the tops of it, but doesn't look like any close exception handler there either. If we're sticking with the signature of this method so that it returns boolean maybe it should just return false - it's invalid input so just throttle it.

Or maybe it just needs to throw a PrecheckException instead of IllegalArgumentException.

[david-bakin-sl]

Also, should have a test for negative in GasLimitDeterministicThrottleTest.

[david-bakin-sl]

Also, I wonder if we should disallow anything less than the intrinsic gas limit here. It's just going to get rejected at precheck anyway. Is there a reason to disallow those txs earlier? Some kind of fairness or DoS argument?

[kimbor]

For my first iteration of this PR, I had it throwing a PreCheckException here. I ended up changing that on the grounds that I didn't want to add a dependency on com.hedera.node.app.spi module. The throws IllegalArgumentException is just a sanity check, any execution path should fail long before it gets here.

[kimbor]

I will add a test for negative gasLimit in GasLimitDeterministicThrottleTest.

[david-bakin-sl]

Again, my question would be: Why <0 instead of <=0 or even <= 21000.

(Don't know the answer here or above, which is why I'm asking.)

In this case you'd have to signal the default case - i.e., not a contract-related operation throttled by gas - separately than using 0L.

[david-bakin-sl]

Needs a test in IngestCheckerTest.ThrottleTests?

Actually, I don't see a test for any of the conditions in assertThrottlingPreconditions. Also actually, assertThrottlingPreconditions seems misnamed? Until this code you just added it doesn't seem to be checking throttling preconditions at all, just some interesting system preconditions.

[kimbor]

Most of the preconditions are checked during pureChecks methods on each transaction type. That's where we check whether the gas limit is <=intrinsicGas. But, checking throttles comes earlier in the ingest workflow than running pureChecks. Before we check throttles, we do just the bare minimum checks necessary to ensure that the throttle checking code will succeed. That's what this method is about.

[kimbor]

And yes, I'll add that test too.

[david-bakin-sl]

This tests new code at IngestChecker.assertThrottlingPreconditions. Needs a separate test for new code at GasLimitDeterministicThrottle.allow. If there's a way to hapispec test throttles, don't know.

[kimbor]

The GasLimitDeterministicThrottle.allow txGasLimit<0 code path should never execute. I'll create a unit test for it but I don't think there's any way to create a hapi test.