Skip to content

Migrate to zokrates v0.6#13

Open
petscheit wants to merge 9 commits intoinformartin:masterfrom
petscheit:master
Open

Migrate to zokrates v0.6#13
petscheit wants to merge 9 commits intoinformartin:masterfrom
petscheit:master

Conversation

@petscheit
Copy link
Copy Markdown

This PR updates all necessary files to the newest zokrates syntax. I think one mayor downside with this approach could be the increased number of inputs in the solidity verifier, as we now have a u32[8] array representing 256 bits, instead of 2 field elements. Will look into that on Wednesday, to compare the gas costs per verification.

Issues:

  • Some blocks don't pass the target assertion. This behavior was consistent with the old version though (current master), and I suspect it has something to do with using the testnet. Maybe the hash rate in the testnet is more unstable than mainnet which results in unexpected diff. adjustments? Block #4031 is an example of this.

Questions:

I was a bit confused by the way the block numbers get selected when defining the starting block and batch size. When looking at preprocessing/zokrates_helper.py:13 I don't understand how that worked before, and correct headers where hashed.

The way I understand it, a batch should consist of a number of blocks, that are all in sequential order. That way, we can sequentially rehash all headers, to make sure they are correct. So when setting starting_block=1000 and batch_size=3, I would expect blocks 1000,1001,1002 to be part of the batch.

This was implemented differently though, and I honestly can't really make sense of it.

@petscheit
Copy link
Copy Markdown
Author

Benchmarking update:

I've done some quick benchmarking for the new version, and the verification cost with the new version is significantly then the old one, due to the increased number of verification inputs. The deployment costs also increased significantly.
image

On the upside, constraint count is down by about 3,5%. I will update the zokrates program to convert the results to fields as a last step, that should fix it for now, long term a u64 and u128 type in zokrates would be useful.

@petscheit
Copy link
Copy Markdown
Author

I've reverted the public inputs and outputs of the zokrates program to use fields instead of u32's, resulting in gas savings.
image

Internally, the zokrates program still uses u32's (needed for sha256), so there is a lot of packing/unpacking is done at the moment. Once ZoKrates solves the input/output inefficiencies, these can be removed without much work, to use u32's also for in and outputs. Also, there is one zokrates output less then 0.5.2 version, since the 'result' field has been replaced by an assert() which further reduces the verification costs.

@informartin
Copy link
Copy Markdown
Owner

Hi @petscheit , thank you for your PR, really appreciate your effort to introduce the newest Zokrates updates into the project and make zkRelay future-proof!

Issues:

* Some blocks don't pass the target assertion. This behavior was consistent with the old version though (current master), and I suspect it has something to do with using the testnet. Maybe the hash rate in the testnet is more unstable than mainnet which results in unexpected diff. adjustments? Block #4031 is an example of this.

Yes, that may very well be the case, @properfect12 is currently working on a test-framework that will allow us to improve the reliability of zkRelay It uses blocks from the mainnet and a dockerized local testnet. He is testing your PR using it already :)

Questions:

I was a bit confused by the way the block numbers get selected when defining the starting block and batch size. When looking at preprocessing/zokrates_helper.py:13 I don't understand how that worked before, and correct headers where hashed.

The way I understand it, a batch should consist of a number of blocks, that are all in sequential order. That way, we can sequentially rehash all headers, to make sure they are correct. So when setting starting_block=1000 and batch_size=3, I would expect blocks 1000,1001,1002 to be part of the batch.

This was implemented differently though, and I honestly can't really make sense of it.

You are right, the issue here is with the naming though. preprocessing/zokrates_helper.py:13 should not ask for a blockNo, but for a batchNo. @properfect12 has already changed this in a branch he is currently working on and the changes will be merged soon.

I've reverted the public inputs and outputs of the zokrates program to use fields instead of u32's, resulting in gas savings.

Your benchmarks look very promising! Good to see, that it can be that efficient with some optimizations. When measuring the RAM requirements, it seems to me that v0.6 demands much more space. While I could compile the program verifying 504 blocks with about 100GB RAM, the server runs out of memory when using v0.6 with a max capacity of 128GB. @JacobEberhardt mentioned Zokrates will include the external SHA256 module again, maybe that will solve the issue?

I would propose to leave the PR open for the moment and start the integration as soon as Zokrates copes with the observed memory issue, would you agree?

@petscheit
Copy link
Copy Markdown
Author

ZoKrates has fixed the memory issues on the develop branch, so I've updated this to be compatible. Didn't test with 504 inputs, but in other sha256 benchmarks the RAM requirements were reduced significantly. This also seems to be the sweet spot performance-wise, constraints and verification costs are the lowest measured so far. Updates of the benchmark:

image

This PR requires Zokrates/ZoKrates#736 to be merged, as there are minor changes in the pedersen hashing library

@petscheit
Copy link
Copy Markdown
Author

Here are some quick memory and compile time benchmark results for 20 blocks. Using 2.8GB in the compile step

file compile_opt_microsec memusg_compile_KiB constraints
validate.zok 256022499 2811292 1842892

@thecodingshrimp
Copy link
Copy Markdown
Collaborator

Hey @ALL,

yesterday I reached out to @petscheit via pm because of compiling issues, but since we should have a single source of truth, lets keep the discussion in this pr.

According to @petscheit, zokrates also needs the pr Zokrates/ZoKrates#731 to be merged to be compatible with the changes in this pr.

Still, after merging those changes, zokrates won't compile validate.zok and compute_merkle_proof.zok with the following error message:

Compiling validate.zok

Compilation failed:

compute_merkle_root.zok:5:15
        Function definition for function pedersenhash with signature (bool[512]) -> _ not found.

compute_merkle_root.zok:12:15
        Function definition for function pedersenhash with signature (bool[512]) -> _ not found.

compute_merkle_root.zok:17:15
        Function definition for function pedersenhash with signature (bool[512]) -> _ not found.

compute_merkle_root.zok:20:2
        Function definition for function pedersenhash with signature (bool[512]) -> bool[256] not found.

validate.zok:142:5
        Function definition for function compute_merkle_root with signature (bool[256][10]) -> field[2] not found.

The pedersenhash function expects an array of type u32[16], but gets a bool[512] array. Is there some sort of compiler flag to suppress the type conversion error or is there still something missing?

reproducing steps

clone Zokrates/ZoKrates:develop
merge Zokrates/ZoKrates#731 and Zokrates/ZoKrates#736
build zokrates and make it default

clone and setup petscheit/zkRelay:master or properfect12/zkRelay:tests_zok_0_6
exec zkRelay generate-files 10
exec zkRelay setup

@petscheit
Copy link
Copy Markdown
Author

Hi Leonard, thats on me. There was an import error which I fixed on Monday, but forgot to push. Sorry about that! Let me know if it works now

@thecodingshrimp
Copy link
Copy Markdown
Collaborator

finally got around to testing your changes. Looks good!

@informartin will merge this PR as soon as Zokrates/ZoKrates#736 is merged.

@thecodingshrimp
Copy link
Copy Markdown
Collaborator

I updated the test environment to work with zokrates 0.6 and created a PR at petscheit/zkRelay#1

@petscheit please review and merge it before we can merge this PR.

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.

3 participants