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.

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

GasEstimator issue triggered by changing U128 serialization #11324

Closed
benesjan opened this issue Jan 17, 2025 · 0 comments · Fixed by #12213
Closed

GasEstimator issue triggered by changing U128 serialization #11324

benesjan opened this issue Jan 17, 2025 · 0 comments · Fixed by #12213
Assignees
Labels
T-bug Type: Bug. Something is broken.

Comments

@benesjan
Copy link
Contributor

benesjan commented Jan 17, 2025

In this PR I changed U128 serialization such that it serializes to 2 fields and not 1. This then makes it possible to use as contract function arg as the resulting serialization format matches intrinsic serialization of Noir (U128 struct contains 2 limbs and that's what Noir assigns the values to when matching the witness with the function args).

But for whatever reason this has caused gas_estimation.test.ts to fail (specifically the second and third test case).

Palla helped me with debugging and he thinks this is an issue with the estimator. There is a workaround - if I set estimatedGasPadding to a larger value the tests passes.

The plan is that I will revisit this once I implement Packable trait because once that is done U128 will be once again stored as 1 field in storage. This will help us determine whether the issue is storage related as the storage costs should get to values before this PR was made.

Look for "TODO(#11324)" in the codebase to find where the workaround was placed.

UPDATE

Just run the test in my Packing PR and it has not resolved the issue. On the contrary I had to increase the estimated gas padding so Packing for whatever reason made the estimation even worse.

Note that with that PR note logs are not yet packed so makes sense to revisit this after that is done as the issue could be related to DA fees.

UPDATE 2

I've figured out that we have an issue with teardown fee estimation.
The estimated gas without padding is:

{"gasLimits":"Gas { daGas=5120 l2Gas=402269 }","teardownGasLimits":"Gas { daGas=1024 l2Gas=80738 }"}

and the actually consumed gas is:

publicGas: 'Gas { daGas=4096 l2Gas=336476 }', teardownGas: 'Gas { daGas=1024 l2Gas=80801 }'

See that the teardown l2Gas is higher than estimation.
In this PR I've reverted the changes done to pay_refund teardown func and I confirm that it passes without the workaround.

UPDATE 3

Palla had a good guess that the issue most likely arises from U128 comparison here resulting in different gas costs based on the max_fee (during estimation we feed in a much larger fee than during real execution).

I confirmed that when I convert the values to fields first and then do the comparison on fields then there is no gas estimation issue.

Alvaro confirmed in a discussion on slack that there indeed is a slow and fast track when comparing U128s.

@benesjan benesjan changed the title GasEstimator issue triggered by changing U128 serialization GasEstimator issue triggered by changing U128 serialization Jan 17, 2025
@benesjan benesjan self-assigned this Jan 17, 2025
@benesjan benesjan added the T-bug Type: Bug. Something is broken. label Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: Bug. Something is broken.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant