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

Upgrade to ark-* 0.5.0 #3047

Open
wants to merge 1 commit into
base: albatross
Choose a base branch
from
Open

Upgrade to ark-* 0.5.0 #3047

wants to merge 1 commit into from

Conversation

hrxi
Copy link
Contributor

@hrxi hrxi commented Nov 13, 2024

This allows us to drop our local patches. :)

This allows us to drop our local patches. :)
Copy link
Member

@paberr paberr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You removed ToBeBytesGadget, did we not use that?
Apart from that, LGTM

Regarding the current check failures:

  • The failing web-client looks a bit like the loop unrolling problem if I'm not mistaken, but it should be fixed in 0.5 through a conditional compilation flag.
  • The tests fail in the check whether we have enough signers. I already looked into it but couldn't find an obvious issue.

@paberr
Copy link
Member

paberr commented Nov 13, 2024

Also, while reviewing the failing tests, I stumbled across the following code from Bruno (check_signers function):

        // num_signers >= min_signers
        num_signers.is_cmp(&min_signers, Ordering::Greater, true)

It looks to me that there's a mismatch between the comment and the code.
We should ensure consistency between the circuit code and our regular consensus condition though. If the consensus accepts blocks with num_signers == min_signers but the ZKP does not, we can run into an issue where we can not create ZKPs anymore.

Copy link
Member

@paberr paberr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we need to check that the upgrade did not modify any constraints that invalidate our generated ZKP keys.

@hrxi
Copy link
Contributor Author

hrxi commented Nov 13, 2024

There's also this issue I created wrt. the test failure: arkworks-rs/r1cs-std#151. We can probably just copy that function to our codebase without the panic.

You removed ToBeBytesGadget, did we not use that?

This feature was added by 0.5.0 with the same name and same functionality, so I could just remove our ToBeBytesGadget.

  • The failing web-client looks a bit like the loop unrolling problem if I'm not mistaken, but it should be fixed in 0.5 through a conditional compilation flag.

Yup on both, but maybe another loop unrolling problem appeared since then. I'll look into it.

We should ensure consistency between the circuit code and our regular consensus condition though. If the consensus accepts blocks with num_signers == min_signers but the ZKP does not, we can run into an issue where we can not create ZKPs anymore.

Does that mean we need to adjust the consensus code because we can't adjust the ZKP code anymore?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants