search

keep-starknet-strange / garaga

State-of-the-art Elliptic Curve operations and SNARKS verification for Cairo & Starknet 🐺.
https://garaga.gitbook.io/
MIT License
202 stars 51 forks source link

bug: Error compiling when groth16 verifier contract is a member of a Scarb workspace #198

Closed danielntmd closed 1 month ago

danielntmd commented 2 months ago

Bug Report

Garaga version:

v0.13.2.3 scarb version: 2.8.0

Current behavior: When building the verifier contract as a member of a Scarb workspace, the error error: Invalid order of type declarations for serialization. occurs. The function multi_pairing_check_bn254_3P_2F_with_extra_miller_loop_result inside of the verifier contract seems to be the culprit. Commenting out this function compiles the package properly.

Expected behavior:

The package should build properly.

Steps to reproduce:

  1. Use Garaga gen with the bn254 verification key found in /hydra/garaga/starknet/groth16_contract_generator/examples (specifically use "v0.13.2.3" of Garaga")
  2. Create a Scarb workspaceand add the verifier package as a workspacemember
  3. Scarb build will result in error

Related code:

This repo contains the bug. Code: https://github.com/danielntmd/garaga_scarb_workplace_bug

Other information:

Disclaimer: This issue might be revolved around a lack of fundamental understanding of how Scarb handles its workspace. Any feedback is greatly appreciated.

feltroidprime commented 1 month ago

Looking at it. Thank you for reporting.

feltroidprime commented 1 month ago

This is weird because outside of a workspace it seems to work. I tried to make more functions and structs public but it seems not to be the issue.

Is this related ? https://github.com/software-mansion/scarb/pull/1593

danielntmd commented 1 month ago

Not sure.. I don't think it has to do with features since the error doesn't explicitly incorporate them, but perhaps the error is adjacent?

maciektr commented 1 month ago

Copying my response from telegram below:

If you build your package as a workspace member, only profiles and cairo section defined in workspace root manifest are taken into account. This means, that you need to put the sierra-replace-ids = false there, for it to take effect. That is the difference between workspace/no workspace build that makes it fail for you.

I cannot speak as of why it fails with debug ids, if you think this should work, please report it as a bug in the compiler repository.

The PR you've linked should not be related at all, it only changes some validation logic in Scarb itself.

feltroidprime commented 1 month ago

Hey, so the issue comes from function names (or maybe recursive types from circuit) in Garaga being too large.

As generated by the garaga generator, adding in Scarb.toml

[cairo]
sierra-replace-ids = false

Fixes this.

But as @maciektr mention, one need to add this at the top of the workspace's scarb.toml I tested on the provided repo and it now compiles without issues.

feltroidprime commented 1 month ago

I'm investigating if I can put it at the garaga lib level so contracts that import it don't have to add it themselves.

feltroidprime commented 1 month ago

I'm investigating if I can put it at the garaga lib level so contracts that import it don't have to add it themselves.

Apparently no.

So the solution is to add 

[cairo]
sierra-replace-ids = false

as suggested above in the top-level .toml of the workspace.