bug: "Internal error, cycle detected" with recursive struct in 2.3.0 but not in 2.3.0-rc0

[Quentash]

Bug Report

Cairo version:
2.3.0

Current behavior:

We need to be able to handle recursive structures in Kakarot for the RLP decoding.
The tests were running correctly in version 2.3.0-rc0 but are failing in 2.3.0 with this error :

thread 'main' panicked at /cargo/registry/src/index.crates.io-6f17d22bba15001f/salsa-0.16.1/src/lib.rs:490:48:
Internal error, cycle detected:

DatabaseKeyIndex { group_index: 2, query_index: 29, key_index: 759 }
DatabaseKeyIndex { group_index: 2, query_index: 29, key_index: 760 }

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Error: 'scarb metadata' exited with error

If I modify the struct and the tests so that they don't use struct recursion, the tests will run.

Expected behavior:

To be able to use struct recursion in 2.3.0 as well.

Steps to reproduce:

Ironically, I couldn't reproduce the bug in a MRE so I might only be able to link you the related PR : kkrt-labs/kakarot-ssj#457 (comment)

Once pulled, you should be able to run scarb test -f test_rlp.
Once using cairo 2.3.0-rc0 and assert that the tests run.
And once using cairo 2.3.0 and assert that the tests panic with the error mentionned above.
(Do not forget to modify the Scarb.toml as well)

Here is the related struct found in rlp.cairo:

#[derive(Drop, Copy, PartialEq)]
enum RLPItem {
    String: Span<u8>,
    List: Span<RLPItem>
}

Any tests calling the decode() function will trigger the error.

Other information:

I tried to reproduce the bug in a brand new scarb project but it is running smoothly. I will keep updated if I manage to reproduce the bug in a smaller setup. This is confusing.

[Eikix]

Hi, I tried running this locally.

Steps:

• Checkout dev: improve rlp.cairo to support recursive list kkrt-labs/kakarot-ssj#457
• Run scarb test
• All tests are green

Then:

• Replace all 2.3.0-rc0 by 2.3.0
• Run scarb test
• crate utils tests fail with:

❯ scarb test -p utils
     Running cairo-test utils
   Compiling test(utils_unittest) utils v0.1.0 (/Users/eliastazartes/code/kkrt-labs/kakarot-ssj/crates/utils/Scarb.toml)
thread 'main' panicked at /Users/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/salsa-0.16.1/src/lib.rs:490:48:
Internal error, cycle detected:

DatabaseKeyIndex { group_index: 2, query_index: 29, key_index: 754 }
DatabaseKeyIndex { group_index: 2, query_index: 29, key_index: 755 }

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Error: `scarb metadata` exited with error

[orizi]

Recursive structs are still supported - this is possibly not related.
When i attempted to do this on main it worked - so I can't really reproduce the issue.

[Eikix]

Recursive structs are still supported - this is possibly not related. When i attempted to do this on main it worked - so I can't really reproduce the issue.

Not sure I understand. I tried again on the branch linked by @Quentash to:

• Checkout the PR branch
• Run tests -> all green ✅

Then,

• Replace 2.3.0-rc1 and 2.3.0-rc0 mentions to 2.3.0
  

• Run tests again ->

     Running cairo-test utils
   Compiling test(utils_unittest) utils v0.1.0 (/Users/eliastazartes/code/kkrt-labs/kakarot-ssj/crates/utils/Scarb.toml)
thread 'main' panicked at /Users/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/salsa-0.16.1/src/lib.rs:490:48:
Internal error, cycle detected:

DatabaseKeyIndex { group_index: 2, query_index: 29, key_index: 754 }
DatabaseKeyIndex { group_index: 2, query_index: 29, key_index: 755 }

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Error: `scarb metadata` exited with error

[orizi]

Managed to run it now - and it seems to work.
have you changed both Cairo and Scarb versions?
Edit:
I see you have - but it just works on my machine.

[Eikix]

Managed to run it now - and it seems to work. have you changed both Cairo and Scarb versions? Edit: I see you have - but it just works on my machine.

Even scarb test -p utils?

[orizi]

Oh - it fails now - checking it out.
thanks!

[orizi]

Found the basic culprit basic information - it is about call cycle detection and auto addition of gas withdrawal.

[Eikix]

Found the basic culprit basic information - it is about call cycle detection and auto addition of gas withdrawal.

What does that mean:)?

[orizi]

We have some algorithm for finding SCCs for adding withdraw_gas statements, and it fails for some reason - debugging :)

[Eikix]

We have some algorithm for finding SCCs for adding withdraw_gas statements, and it fails for some reason - debugging :)

Thanks 🙏

[orizi]

Found and fixed.
How badly does this block you?
If we release 2.4.0-rc0 rather soon, will t be ok? or a 2.3.1 release is a must?
Thanks!

[Eikix]

Found and fixed.

How badly does this block you?

If we release 2.4.0-rc0 rather soon, will t be ok? or a 2.3.1 release is a must?

Thanks!

It's blocking us for decoding eth transactions, we could really use the earlier option if that's okay with you

[Quentash]

Thanks a lot, Ser @orizi !!

[enitrat]

If we release 2.4.0-rc0 rather soon, will t be ok? or a 2.3.1 release is a must?

@orizi We can use scarb nightlies that are main-based in the meantime, so there shouldn't be a need for a release

[orizi]

in any case, we are doing a 2.3.1 for some small fixes - this one is included - so this will be available hopefully later today.