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: upload ceremony file to storage #3

Merged
merged 3 commits into from
Jan 22, 2024
Merged

Conversation

veaceslavdoina
Copy link
Contributor

This PR add a step to upload ceremony file to storage in two names

  • proof_main.zkey
  • 55c969da98f17fc878e5aee610a702e9cb1a1e5b3b0f2ebb26aef9690a96eafa (sha256 from the first file)

After upload, file can be downloaded using the following URL's

curl https://ceremony.codex.storage/proof_main.zkey -o proof_main.zkey

curl https://ceremony.codex.storage/55c969da98f17fc878e5aee610a702e9cb1a1e5b3b0f2ebb26aef9690a96eafa -o proof_main.zkey.hash

Closes https://github.com/codex-storage/infra-codex/issues/123

Copy link
Member

@AuHau AuHau left a comment

Choose a reason for hiding this comment

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

This workflow might be run for different environments, for testing purposes, etc., so having it simply moved to the address https://ceremony.codex.storage/proof_main.zkey does not really make sense to me. What is the goal of having it under this address? Is this URL then meant to be used for distributing the zkey to the Codex storage nodes?

@AuHau
Copy link
Member

AuHau commented Jan 16, 2024

Also what about cleanup of those files? As I said, it might be used for some testing etc... The Github Artifacts has 5 days expiry, so they will be automatically cleaned up, but if we move it to S3 then is there some planned retention or cleanup?

@bkomuves
Copy link
Collaborator

also this is as of now is completely unsafe, so better call it something which clearly indicates that it is temporary and purely for testing purposes?

(ceremony is not a good word either. let's call it "proving key", that's what is. It is generated by a circuit-specific trusted setup ceremony, which is of course completely not to be trusted because it's faked by the CI :)

@veaceslavdoina
Copy link
Contributor Author

veaceslavdoina commented Jan 16, 2024

This workflow might be run for different environments, for testing purposes, etc., so having it simply moved to the address https://ceremony.codex.storage/proof_main.zkey does not really make sense to me. What is the goal of having it under this address? Is this URL then meant to be used for distributing the zkey to the Codex storage nodes?

Good question and for initial tests, have a static name maybe helpful. If you think that not at all - let's use just a sha256.

Also what about cleanup of those files? As I said, it might be used for some testing etc... The Github Artifacts has 5 days expiry, so they will be automatically cleaned up, but if we move it to S3 then is there some planned retention or cleanup?

We have 10GB Free tier and I will add rotation via bash once it will be clear how many files we would like to store. We can't use just Expiration because we need at least one file always to be accessible.

My initial plan was to store no more than 10 x 5GB of data. But can we store just 2 last files?

Do you ave any proposal about that?

@veaceslavdoina
Copy link
Contributor Author

also this is as of now is completely unsafe, so better call it something which clearly indicates that it is temporary and purely for testing purposes?

(ceremony is not a good word either. let's call it "proving key", that's what is. It is generated by a circuit-specific trusted setup ceremony, which is of course completely not to be trusted because it's faked by the CI :)

Good point, and your proposal proving-key is about file name or URL https://proving-key.codex.storage ?
Or we can use a subpath for now

https://ceremony.codex.storage/sha256
https://ceremony.codex.storage/proving-key/sha256

@bkomuves
Copy link
Collaborator

I meant mainly the file name. The URL is not bad actually, though maybe can figure out something better later?

It could be for example circuit.codex.storage?

@veaceslavdoina
Copy link
Contributor Author

veaceslavdoina commented Jan 16, 2024

It could be for example circuit.codex.storage?

Sure, If you think that it is better representation - let's switch to it.

@veaceslavdoina
Copy link
Contributor Author

veaceslavdoina commented Jan 17, 2024

Did an updated based on the comments

  • Move all variables to the common place
  • Unify variables case
  • Upload file only using hash as name - @AuHau
  • Added an optional prefix to the storage path to distinguish files with a current default proving-key - @bkomuves
    • path used for consistency, to have file names as a hash, let's discuss if we need to change that
    • set it to empty value (s3_bucket_path: '') to use root path
    • we can't pass it as an input as we are limited with 10 inputs for now, but can use variables to not edit workflow each time
  • Changed download base URL to circuit.codex.storage - @bkomuves
  • Added a step Show download URL
    Screenshot 2024-01-17 at 18 50 43
    https://circuit.codex.storage/proving-key/69db5c28d100ee153f16aeb90d85c7327c4f60ccea9487550a17fbe7bad5018d
    

@AuHau
Copy link
Member

AuHau commented Jan 17, 2024

So, I have been thinking about this a bit more. I have not really been part of the discussion regarding how to distribute the Zkey file and other things, so I am wondering what is the goal of this?

The key thing we need to remember is that we need to keep synchronized several things. Otherwise, the proving won't work correctly. The things I keep in mind are the zkey file, but also the solidity verifier, which are tightly coupled. So we need to make sure that if you "upgrade zkey" file on Codex node, the node needs to run along blockchain network that has deployed the correct verifier. We are currently working on integrating the verifier into our contracts and we plan to have different Verifiers for different environments (local, dist-testing, testnet, ...) and we will always deploy the appropriate version of the Verifier per environment.

Sooo that brings me back to the "why we need this patch" question. My original idea about how this workflow will be used is that, if somebody decides that new version of circuit assets needs to be generated, then he will then download the generated artifacts and disseminate the assets (zkey, verifier etc.) to the appropriate places manually. If we want to automatize the "disseminating" part, then we should do it IMHO for all the required parts.

@veaceslavdoina
Copy link
Contributor Author

All story started during the last Client meeting and a way to ship circuit file for proofs verification. We discussed several ways like, ship with Docker image, use Codex Nodes (will be done later), CDN. Some details are here - https://github.com/codex-storage/infra-codex/issues/121.

download the generated artifacts and disseminate the assets (zkey, verifier etc.) to the appropriate places manually.

That is the goal for now - we need to make required files available for download in an easy and fast way.
If we miss something, let's discus that and include in the upload step.

If manual way is a preferred one, let's follow that approach. We at least have a storage with CDN as a temporary workaround for distribution.

Copy link
Member

@AuHau AuHau left a comment

Choose a reason for hiding this comment

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

Well we can merge it if you want, but I just wanted to point out, that these things needs to be properly synced otherwise the proving system won't work ;-)

@veaceslavdoina
Copy link
Contributor Author

@tbekas now is working on the mechanism to get that file during Codex startup and would need to have source of that file accessible via HTTP endpoint.

I did some manual run from my fork and file is accessible, so he is not blocked.

@tbekas, do we need that now and in general?

@AuHau
Copy link
Member

AuHau commented Jan 17, 2024

@tbekas not sure what is the plan, but maybe you could extend the auto-detecting mechanism of which smart contract's address Codex uses based on ChainID, to have there also the related Zkey download URL mapped there.

I am talking about this place: https://github.com/codex-storage/nim-codex/blob/master/codex/contracts/deployment.nim

@bkomuves
Copy link
Collaborator

Indeed the verifier contract and the zkey is tightly coupled, they must be in sync. For example the generated verifier contract has some constants (actually taken from .zkey file) hard-coded. In theory it would be possible to refactor the verifier contract to take these from external sources, but it's probably not worth doing it.

I guess the issue particularly with the .zkey file is that it's big and it needs to be distributed to all nodes and more generally to anybody who wants to sanity check before trusting the system.

@tbekas
Copy link

tbekas commented Jan 18, 2024

@bkomuves @AuHau would it be possible to read from the blockchain which circuit file (hash) should be downloaded based on the verifier that's used in the contract in the blockchain that we're connected to?

@bkomuves
Copy link
Collaborator

@tbekas: In theory, yes. The verifier contract contains some hard-coded constants which are unique for the given .zkey file (I wouldn't call it "circuit file"). If you can extract those somehow, they can be matched with a list of possible .zkey files.

Not surprisingly, you cannot compute the hash of the file from these, so for that we would have to maintain a table with the mapping.

@AuHau
Copy link
Member

AuHau commented Jan 22, 2024

@bkomuves @AuHau would it be possible to read from the blockchain which circuit file (hash) should be downloaded based on the verifier that's used in the contract in the blockchain that we're connected to?

That is a pretty good idea! I like it. It will need more work, but yes, we could have a constant in the Verifier contract that would contain the hash of the Zkey, which then can be exposed to the nim-codex and can be queried to find out what Zkey file to retrieve. I guess now it will be downloaded over HTTP from some storage (S3 I guess?), but in future we will then be able to shift to downloading from Codex network itself, which is pretty neat.

This pretty much simplifies the "synchronization" problem as it will boil down only to contract's dependency.

@bkomuves
Copy link
Collaborator

@AuHau in theory there is no need for a hash, the coordinates of the points alpha, beta, gamma, delta already identify the .zkey file (these are randomly generated when creating the zkey and contain about 4x the amount of bits of information as a sha256 hash).

Though I just realized that in the CI we always use the same entropy which probably ruins this... hmm.

@bkomuves
Copy link
Collaborator

On a second though, including the zkey hash solves the "need to maintain a mapping" thing nicely, so actually that seems to make sense.

@AuHau
Copy link
Member

AuHau commented Jan 22, 2024

Well, yes, it identifies the zkey file, but not in a way in which you could retrieve it. The plan for the future is to upload the zkey file directly to Codex and have the nodes download it when needed. For that, we would have to have some lookup table that would translate the points to hash that we could use to retrieve the file, as you mentioned earlier. Sooo having the hash directly in contract simplifies this process quite significantly IMHO.

Edit: Damn, your thinking is faster then my writing 🤪

@AuHau
Copy link
Member

AuHau commented Jan 22, 2024

I will merge this as I will need it for the follow up work as discussed here: #3 (comment)

@AuHau AuHau changed the title Upload ceremony file to storage ci: upload ceremony file to storage Jan 22, 2024
@AuHau AuHau merged commit 01b924d into master Jan 22, 2024
@veaceslavdoina veaceslavdoina deleted the feat/upload-ceremony-to-storage branch January 22, 2024 14:51
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.

None yet

4 participants