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

bug: gas computation / unexpected cycle in computation issue with recursive type definition #5524

Closed
remybar opened this issue May 9, 2024 · 6 comments · Fixed by #5539
Closed
Labels
bug Something isn't working

Comments

@remybar
Copy link
Contributor

remybar commented May 9, 2024

Bug Report

Cairo version:

At least on 2.5.4 and 2.6.3

Current behavior:

For the Dojo project, we use the following types to define the layout of our data that will be stored in a dedicated smart contract:

#[derive(Copy, Drop, Serde, Debug, PartialEq)]
struct FieldLayout {
    selector: felt252,
    layout: Layout
}

#[derive(Copy, Drop, Serde, Debug, PartialEq)]
enum Layout {
    Fixed: Span<u8>,
    Struct: Span<FieldLayout>,
    Tuple: Span<FieldLayout>,
    Array: Span<FieldLayout>,
    ByteArray,
    Enum: Span<FieldLayout>,
}

Then, we have 3 functions to read/write/delete an object based on its layout.
This code leads to:

  • Failed calculating gas usage, it is likely a call for gas::withdraw_gas is missing. error when we try to run our test suite.
  • Compiling Sierra class to CASM with compiler version 2.5.4... Error: found an unexpected cycle during cost computation when we try to declare the compiled contract class on Katana.

I built the following tiny project outside of Dojo to reproduce the issue : https://github.com/remybar/scarb-recursion

In this project, there is only these 2 types and the entity() function which reads an object thanks to the _read_layout() function.

This project has exactly the same issues than our Dojo project.
If I remove the Struct: Span<FieldLayout> variant from Layout, I can run scarb test and declare the contract class on Katana.
Also, if the _read_layout() function does not have any loop inside, everything works well.

So, I guess the issue is due to the fact that 2 variants from Layout refers, directly on indirectly through an intermediate type, to itself (the Layout type). And if we loop on this type, the gas computation failed.

Expected behavior:

At a first glance, this Cairo code seems legit to me and should work fine.

Steps to reproduce:

Just run scarb test and/or declare the contract class on Katana.

Related code:

https://github.com/remybar/scarb-recursion

@remybar remybar added the bug Something isn't working label May 9, 2024
@remybar
Copy link
Contributor Author

remybar commented May 9, 2024

Tell me if you need more explanation, context or whatever !

@remybar remybar changed the title bug: bug: gas computation / unexpected cycle in computation issue with recursive type definition May 9, 2024
@orizi
Copy link
Collaborator

orizi commented May 9, 2024

Thank you for the report!
managed to minimally recreate an example now as well.
For your specific usecase you can even reorder your function definitions - and your code would fully compile.
will be taking care of it soon.

@remybar
Copy link
Contributor Author

remybar commented May 10, 2024

Awesome ! Thanks @orizi for your quick fix !

What is the good way to reorder function definitions to fix the issue ? Because I could use this trick to have my feature branch working on Dojo while waiting for your fix to be merged :)

@orizi
Copy link
Collaborator

orizi commented May 10, 2024

It seems that if you define the functions with the loops first it works better. (Before the function that calls them)

@remybar
Copy link
Contributor Author

remybar commented May 10, 2024

It seems that if you define the functions with the loops first it works better. (Before the function that calls them)

Ah yes, it looks like this trick works fine with Cairo 2.5.4 but not with Cairo 2.6.3.
I think that's why the bug randomly disappeared on my Dojo feature branch based on Cairo 2.5.4 but reappeared when rebasing with a branch with Cairo 2.6.3 😅

Thanks for the feedback 👍

@orizi
Copy link
Collaborator

orizi commented May 10, 2024

probably has something to do with the changes in inlining that changed the scheduling of the algorithm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants