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

CI: Update cycle-count-regression for latest Sphinx #164

Closed
wwared opened this issue Aug 16, 2024 · 1 comment
Closed

CI: Update cycle-count-regression for latest Sphinx #164

wwared opened this issue Aug 16, 2024 · 1 comment
Assignees

Comments

@wwared
Copy link
Contributor

wwared commented Aug 16, 2024

After we merge #161, the new version of Sphinx has slightly different output, which causes the regression CI check to fail, see https://github.com/argumentcomputer/zk-light-clients/actions/runs/10411627928/job/28868538903 for an example.

I'm not sure if the total number of cycles is still being printed by the execution-only mode by default anymore, specifically in that CI run it printed for the execute portion:

2024-08-16T16:20:10.576643Z DEBUG execute: loading memory image
2024-08-16T16:20:10.580254Z  INFO execute: clk = 0 pc = 0x201e8c    
2024-08-16T16:20:10.580339Z DEBUG execute: ┌╴read_inputs    
2024-08-16T16:20:10.590888Z  INFO execute: └╴155,189 cycles    
2024-08-16T16:20:10.641776Z DEBUG execute: ┌╴verify_transaction_inclusion    
2024-08-16T16:20:10.643710Z  INFO execute: └╴28,167 cycles    
2024-08-16T16:20:10.643749Z DEBUG execute: ┌╴verify_signature    
2024-08-16T16:20:11.319562Z  INFO execute: └╴7,366,941 cycles    
2024-08-16T16:20:11.319940Z DEBUG execute: ┌╴verify_merkle_proof    
2024-08-16T16:20:11.323110Z  INFO execute: └╴47,397 cycles    
2024-08-16T16:20:11.371492Z  INFO execute: close time.busy=802ms time.idle=1.20µs
Execution took 7.410766367s
test inclusion::test::test_execute_inclusion ... ok

If needed, we can make a small patch to sphinx to re-add this information at the end of execute. This information is still being printed by the prove_core output, so we can also switch the regression check to do that instead as an alternative

@samuelburnham
Copy link
Member

Closing as this was fixed in #188

Successful runs:
https://github.com/argumentcomputer/zk-light-clients/actions/runs/10831133987/job/30052438529
https://github.com/argumentcomputer/zk-light-clients/actions/runs/10831133987/job/30052440405

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

No branches or pull requests

3 participants