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.

Sign up for GitHub

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

Jump to bottom

Feat/multi dict prover #553

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Feat/multi dict prover #553

wants to merge 13 commits into from

Conversation

Tabaie
Copy link
Contributor

@Tabaie Tabaie commented Jan 14, 2025 •
edited
Loading

This PR enables the prover to support multiple compression dictionaries at once.
Note that blob submission would still operate with a single dictionary and is thus not affected.

BREAKING CHANGE:
As shown in prover/config/config-sepolia-full.toml, the name of the property dict_path under [blob_decompression] has changed to dict_paths and its value is an array of strings, instead of a single string.

@cla-assistant CLA Assistant
Copy link

cla-assistant bot commented Jan 14, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@cla-assistant CLA Assistant
Copy link

cla-assistant bot commented Jan 14, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Tabaie Tabaie added Prover Tag to use for all work impacting the prover Data compressor go Pull requests that update Go code labels Jan 14, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.53%. Comparing base (db31170) to head (caa49c6).

Additional details and impacted files 
@@             Coverage Diff              @@
##               main     #553      +/-   ##
============================================
- Coverage     68.75%   68.53%   -0.22%     
+ Complexity     1166     1125      -41     
============================================
  Files           326      320       -6     
  Lines         13039    12720     -319     
  Branches       1312     1268      -44     
============================================
- Hits           8965     8718     -247     
+ Misses         3526     3472      -54     
+ Partials        548      530      -18
Flag Coverage Δ *Carryforward flag
hardhat 98.64% <ø> (-0.11%) ⬇️
kotlin 66.08% <ø> (-0.30%) ⬇️ Carriedforward from 3aac628

*This pull request uses carry forward flags. Click here to find out more.

see 32 files with indirect coverage changes

@Tabaie Tabaie had a problem deploying to docker-build-and-e2e January 14, 2025 23:26 — with GitHub Actions Error
@Tabaie Tabaie temporarily deployed to docker-build-and-e2e January 16, 2025 06:33 — with GitHub Actions Inactive
@Tabaie Tabaie had a problem deploying to docker-build-and-e2e January 16, 2025 06:47 — with GitHub Actions Error
@Tabaie Tabaie temporarily deployed to docker-build-and-e2e January 16, 2025 06:50 — with GitHub Actions Inactive
@Tabaie Tabaie temporarily deployed to docker-build-and-e2e January 16, 2025 14:31 — with GitHub Actions Inactive
gbotrel
gbotrel previously approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@gbotrel gbotrel left a comment

Choose a reason for hiding this comment

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

lgtm

prover/lib/compressor/blob/v1/blob_maker.go
@@ -302,31 +302,30 @@ func (bm *BlobMaker) Equals(other *BlobMaker) bool {
}

// DecompressBlob decompresses a blob and returns the header and the blocks as they were compressed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could use an updated comment on what this function do in a bit more details;

i.e. DecompressBlob will parse the header, get a dict checksum, attempt to find the corresponding dict from the store, etc... landed here when trying to follow through the usage of this method with many returns from somewhere else it was not clear :)

Copy link
Contributor Author

@Tabaie Tabaie Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Would it be better to bundle all the non-error return values into a response struct?

prover/cmd/prover/cmd/setup.go
@@ -32,7 +33,8 @@ import (
type SetupArgs struct {
Force bool
Circuits string
DictPath string
DictPath string // to be deprecated; only used for compiling v0 blob decompression circuit
Copy link
Contributor

Choose a reason for hiding this comment

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

call it DictPathV0Deprecated ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is, it is technically not deprecated since it still has a legitimate use-case that the new alternative doesn't cover (v0 circuits).

@Tabaie Tabaie had a problem deploying to docker-build-and-e2e January 16, 2025 20:59 — with GitHub Actions Error
@Tabaie Tabaie had a problem deploying to docker-build-and-e2e January 16, 2025 21:21 — with GitHub Actions Error
@Tabaie Tabaie had a problem deploying to docker-build-and-e2e January 16, 2025 21:25 — with GitHub Actions Error
@Tabaie Tabaie force-pushed the feat/multi-dict-prover branch from 5581dfb to 711e2e1 Compare January 16, 2025 21:25
@Tabaie Tabaie had a problem deploying to docker-build-and-e2e January 16, 2025 21:26 — with GitHub Actions Error
Tabaie
Tabaie commented Jan 16, 2025
View reviewed changes
prover/lib/compressor/dict/25-01-15-INCOMPLETE.bin Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is 64KB - 60 B long and cannot be used as is. After three 20-byte addresses are appended to it, we can rename it to 25-01-15.bin

@Tabaie Tabaie temporarily deployed to docker-build-and-e2e January 16, 2025 21:50 — with GitHub Actions Inactive
@Tabaie Tabaie requested a review from gbotrel January 17, 2025 17:23
@Tabaie Tabaie temporarily deployed to docker-build-and-e2e January 17, 2025 17:27 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbotrel gbotrel gbotrel approved these changes

@AlexandreBelling AlexandreBelling Awaiting requested review from AlexandreBelling

@julien-marchand julien-marchand Awaiting requested review from julien-marchand

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
Data compressor go Pull requests that update Go code Prover Tag to use for all work impacting the prover
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Tabaie @codecov-commenter @gbotrel