sablier-labs / v2-core

⏳ Core smart contracts of the Sablier V2 token distribution protocol
https://sablier.com
Other
288 stars 38 forks source link

Reverting test when lowering gas limit #956

Closed DaniPopes closed 2 days ago

DaniPopes commented 2 days ago

Hey, we're trying to lower the default gas limit in foundry to a more reasonable amount in https://github.com/foundry-rs/foundry/pull/8274

Only one test is failing out of all our integration/external tests and it's test_RevertWhen_LoopCalculationOverflowsBlockGasLimit

I'm not sure how to fix this because I don't know the internals, it can be reproduced currently like so: forge t --gas-limit $((2**30-1)) --mt OverflowsBlock

smol-ninja commented 2 days ago

Hi @DaniPopes. Thank you for raising this and updating us on the recent change in Foundry. I've reviewed it and have a question for you.

  1. What happens if a call consumes over 1 billion gas? In our scenario, the loop isn't infinite but iterates a significantly high number of times. Previously, it would revert, but now with the recent change, it exits with an EvmError: MemoryOOG error instead of reverting.

However, I believe we can now remove this test since we inherently verify in the code that the loop won't exceed the block gas limit. I have fixed it in https://github.com/sablier-labs/v2-core/pull/957.

cc @andreivladbrg comments, if any.

DaniPopes commented 2 days ago

I believe the gas limit is reached while instantiating the 250_000 length array, so it fails before doing the sub call LockupDynamic.SegmentWithDelta[] memory segments = new LockupDynamic.SegmentWithDelta[](250_000);

Increasing it to 2B makes the test pass again

smol-ninja commented 2 days ago

Yes. Thats right. So I have lowered it to 25,000 from 250,000.

smol-ninja commented 2 days ago

Temporary fix in main branch: https://github.com/sablier-labs/v2-core/commit/73b2d8043cd8aac48f92ccd49ef77dc3498629ed

smol-ninja commented 1 day ago

@DaniPopes can I set the default gas limit in foundry.toml? The 1B limit isn't sufficient for a few additional functions.

For instance, in this example, we use a loop to calculate the upper bound of a value for different chains. We loop over multiple chains with multiple values until we hit the block gas limit. This fails with the 1B gas limit. I had to use --gas-limit $((5**30-1)), which is 5B gas, to run it successfully

It would be great to set the default gas limit through the foundry config file. Each project can have different requirements, so hard-coding the limit isn't ideal, in my opinion.

DaniPopes commented 1 day ago

It's not hardcoded, it's a default. The config is gas_limit

smol-ninja commented 1 day ago

Ah my bad. Thank you so much for quick reply.