add tests for more sufficient gas consumption testing in callWithExactGas library #13217
Conversation
contracts/package.json
Outdated
| @@ -16,9 +16,9 @@ | |||
| "compile": "hardhat compile", | |||
| "coverage": "hardhat coverage", | |||
| "prepublishOnly": "pnpm compile && ./scripts/prepublish_generate_abi_folder", | |||
| "publish-beta": "pnpm publish --tag beta", | |||
| "publish- beta": "pnpm publish --tag beta", | |||
There was a problem hiding this comment.
I would have to assume this is unintentional? it will likely break github release workflow
There was a problem hiding this comment.
yea that's definitely unintentional. Idk how that got changed but i'll revert it
|
|
||
| function throwOutOfGasError() external pure { | ||
| while (true) { | ||
| //Intentionally consume all gas and throw an OOG error. |
| @@ -216,6 +220,41 @@ contract CallWithExactGas__callWithExactGasSafeReturnData is CallWithExactGasSet | |||
| assertGt(gasUsed, 500); | |||
| } | |||
|
|
|||
| function test_CallWithExactGasSafeReturnData_ConsumeAllGas_Success() external { | |||
| uint16 maxRetBytes = 0; | |||
| uint256 miscGasOverhead = 128; | |||
There was a problem hiding this comment.
pls document how the overhead is calculated as 128; also it should be a constant right?
| uint256 internal constant DEFAULT_GAS_LIMIT = 20_000; | ||
| uint16 internal constant DEFAULT_GAS_FOR_CALL_EXACT_CHECK = 5000; | ||
| uint256 internal constant EXTCODESIZE_GAS_COST = 2600; | ||
|
|
||
| // Calculated as the difference between gasUsed as returned from function and gas passed in to the function | ||
| uint256 internal constant CALL_WITH_EXACT_GAS_SAFE_RETURN_DATA_GAS_OVERHEAD = 128; |
There was a problem hiding this comment.
can you explain why the difference between gasUsed as returned from function and gas passed in to the function turns out to be 128, btw is this overhead constant regardless of receiver logic?
There was a problem hiding this comment.
I'm not able to say that with complete certainty due to optimizations in the compiler that may insert additional unexpected bytecode. The only way to get that level of absolute certainty would be to write a minimal huff-contract to ensure that every opcode is expected. 128 gas was determined as overhead because that was the difference in value returned from SafeCallWithExactGas. That function was called with 20k gas, but the gasUsed returned was 20,128. Since its impossible for the contract to use more than the gas it was assigned, the only logical explanation was that there's additional gas overhead in the call that isn't being properly accounted for.
If you want more certainty than I can attempt that level of granular bytecode construction with huff but it seems kinda like overkill.
There was a problem hiding this comment.
I see, looking at the code, it's likely constant, can you:
- pseudo-verify it is constant regardless of gas limit/payload/receiver by trying out a few different inputs
- succinctly explain in comments the heuristics used to arrive at 18, similar to context above
There was a problem hiding this comment.
I'll do my best. Without inspecting the bytecode manually or writing it in Huff it's very difficult to know exactly how much it takes cause of the compiler optimizations. I will write out the comments and see what I can do.
It looks like the actual amount is somewhere between 122 and 140 gas depending on the gas limit and where the loop ends.
| } { | ||
| // If 30 gas is remaining, then exit the loop by returning | ||
| if lt(gas(), 30) { | ||
| pop(add(0, 0)) // Add two numbers but don't push result onto the stack. Safely consume any residual gas from an odd number remaining. |
There was a problem hiding this comment.
good job on going granular, please explain how the values are calculated, the important thing is the why, e.g:
- why terminate at 30, but not like 40, or 20
- why consume residual gas via
pop(add(0, 0))as opposed more loop iterations
There was a problem hiding this comment.
That value was calculated via manual binary search to determine the least amount of gas possible which can be returned. 30 was the least amount of gas that could remain before an OOG error would be thrown on this call.
Based on what I was able to deduce, one more iteration of the loop would cost more gas than remains, but you need to consume the remaining gas. I.E you leave 30 gas because 1 more loop would cost >30, but you still need to consume the gas. Add is one of the cheapest non-state-changing opcode you can perform, but yul requires you to pop it off the stack of assign it to a variable, and using pop ensures we're not charged the extraneous cost for storing in memory/stack. I will add all of these to comments.
There was a problem hiding this comment.
this may not be reliable right? for example if you need 20 gas to return, with current setup it might reach gas() with 28 gas, and it is able to return, however if someone changes the gas limit passed in, and if it happens to be 22 gas remaining when reaching gas(), then the test might fail.
There was a problem hiding this comment.
maybe consider a range of input gases, eg. 20k, 20k+1, 20k+2,... 20k+30
and locate the minimum gas cutoff number that make all of them pass
| ); | ||
|
|
||
| assertFalse(success, "Error: External Call Succeeded where it should not"); | ||
| assertEq(retData.length, 0, "retData should be zero for OOG error"); |
There was a problem hiding this comment.
Since maxRetBytes is zero, this doesn't actually test anything. If anything was returned it would have been cut off
There was a problem hiding this comment.
The gas consumer contract which is invoked explicitly does not return anything when called so it doesn't make sense to include a max return bytes for this contract.
if lt(gas(), 60) {
return(0x0, 0x0) // Return with no return data
}
…ntractkit/chainlink into chore/1280-callwithExactgasTests
|
The helper library
CallWithExactGas.soldid not have any tests that ensured that all of the gas provided is properly consumed in the event that an incorrect amount of gas is passed in. CCIP makes extensive use of this library and so this tests attempts to ensure that all gas is properly utilized when contracts are called with the helper functions.Added two tests to
callWithExactGas.t.soltest_callWithExactGasAndConsumeAllwhich creates a mock contract and ensures that the amount of gas consumed is the same as the amount passed in.test_callWithExactGasAndConsumeMoreThanProvidedwhich attempts to consume more gas than is provided in the event that gasLimit > (63/64) of available gas passed to the child call in conjunction with EIP-150