-
Notifications
You must be signed in to change notification settings - Fork 4
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
Zkllvm outdated info #48
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
zkllvm/use-cases/primer.mdx
Outdated
mkdir cmake && cd cmake | ||
``` | ||
|
||
Add the [**`CircuitCompile.cmake` module from the zkLLVM repository**](https://github.com/NilFoundation/zkLLVM/blob/master/cmake/CircuitCompile.cmake) to the `./new_circuit/cmake` directory: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SInce we require zkllvm
package to be installed, CircuitCompile.cmake
is a part of it, so the copying is not necessary, we can simply adjust module path to its fixed location:
list(APPEND CMAKE_MODULE_PATH "/usr/lib/zkllvm/share/zkllvm")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess find_package(zkllvm)
works here, isn't it? So user won't need to hardcode the path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it does not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's weird, how am I supposed to ship cmake modules then through apt packages...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@makxenov I see, this makes more sense! I've revised the flow and the CMake config to mention this
zkllvm/use-cases/primer.mdx
Outdated
find_package(crypto3 REQUIRED) | ||
``` | ||
|
||
:::tip[Boost] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would omit this tip. Boost is an internal dependency of crypto3
, so crypto3::all
target is responsible for providing it. We should not care about it here unless our circuit uses Boost directly. But in this case Boost does not deserve a special mention, because it's equal to any alternative library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, this makes sense. I have removed the tip
zkllvm/use-cases/primer.mdx
Outdated
circuit SOURCES ${SOURCES} | ||
LINK_LIBRARIES | ||
crypto3::all | ||
${Boost_LIBRARIES} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, please remove this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, removed
zkllvm/use-cases/primer.mdx
Outdated
${Boost_LIBRARIES} | ||
) | ||
|
||
set(binary_name circuit.ll) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting a variable but not using it after looks a little bit strange. Maybe replace it with a comment: "add_circuit() will generate IR with name circuit.ll"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right. I have amended this part
zkllvm/use-cases/primer.mdx
Outdated
cmake -G "Unix Makefiles" -B ./build -DCMAKE_BUILD_TYPE=Release . -Dcrypto3_DIR=/path/to/crypto3/installation | ||
``` | ||
|
||
This method is useful if a dependency is installed in a custom location rather than `/usr/local/` or any other location requiring root permissions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the permissions of a directory make no difference in specifying the path. I would say that this method could be useful in case if crypto3_DIR
is not fixed (for example, a user is writing a script that installs circuit dependencies and then calls cmake), and one doesn't want to hardcode path in a file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I have revised the explanation
|
||
# Write a circuit with hashes | ||
|
||
This 'how-to' series constructs a 'mock' zero knowledge bridge from scratch. The series starts with writing a simple circuit that uses hashes and ends with creating an algorithm for state-proof verification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm wrong, but usually zero-knowledge
contains hyphen when using as an adjective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are 100% right, I've added the hyphen
zkllvm/use-cases/primer.mdx
Outdated
Add the required library to the project configuration: | ||
|
||
```cmake | ||
set(crypto3_DIR "path/to/crypto3_headers") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's emphasize here that we need a /path/to/crypto3/installation
. Headers are not enough, cmake
must be able to find crypto3Config
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I changed the path description
[[circuit]] bool verify_protocol_state_proof ( | ||
typename sha2<256>::block_type last_confirmed_block_hash, | ||
std::array<block_data_type, 2> unconfirmed_blocks) { | ||
for (int i = 0; i < 2; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use if where it possible
bool res = is_same(unconfirmed_blocks[i].prev_block_hash, last_confirmed_block_hash);
typename sha2<256>::block_type evaluated_block_hash = hash<sha2<256>>(unconfirmed_blocks[i-1].prev_block_hash, unconfirmed_blocks[i-1].data);
return res & is_same(unconfirmed_blocks[i].prev_block_hash, evaluated_block_hash);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, I have rewritten the circuit function to eliminate the nested if
Found a typo, which we missed in previous review: here. Instead of |
|
||
Whenever a Rust circuit is compiled, `rustc` applies various optimizations to reduce its memory usage. | ||
|
||
Among these memory optimizations is [**reordering fields in structs and enums to avoid unnecessary 'paddings' in circuit IRs**](https://users.rust-lang.org/t/disable-struct-reordering-optimization-when-emitting-llvm-bitcode/74951). Consider the following example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is a good idea to link to some random forum post in docs. If we really want to leave a link here, maybe it's better to link to Rust docs:
https://doc.rust-lang.org/reference/type-layout.html#representations
Or at to Nomicon:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are 100% right. I failed to find a proper explanation of this specific problem but now the link is fixed
To avoid this problem, use the `#[repr(C)]` directive: | ||
|
||
```rust | ||
#[derive(Copy, Clone)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing #[repr(C)]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo, added it back in
|
||
Among these memory optimizations is [**reordering fields in structs and enums to avoid unnecessary 'paddings' in circuit IRs**](https://users.rust-lang.org/t/disable-struct-reordering-optimization-when-emitting-llvm-bitcode/74951). Consider the following example: | ||
|
||
```rust |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessary, but maybe for demonstration purposes we can construct smaller example? This one is a bit too big than needed to illustrate the issue and probably it will be a bit harder for users to get the point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I've simplified the example
true | ||
} | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code of the function is wrong. Here is the fixed version:
fn verify_protocol_state_proof(
last_confirmed_block_hash: Fq,
unconfirmed_blocks: [BlockDataType; 2],
) -> bool {
for i in 0..2 {
if i == 0 {
if !is_same(unconfirmed_blocks[i][0], last_confirmed_block_hash) {
return false;
}
} else {
let evaluated_block_hash =
hash(unconfirmed_blocks[i - 1][0], unconfirmed_blocks[i - 1][1]);
if !is_same(unconfirmed_blocks[i][0], evaluated_block_hash) {
return false;
}
}
}
true
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right, a mix-up on my part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in any case, the function is now rewritten from scratch
|
||
#[circuit] | ||
#[unroll_for_loops] | ||
fn verify_protocol_state_proof( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same. Fixed version above
|
||
:::info[Rust directives] | ||
|
||
The `#[derive(Copy, Clone)]` makes it so that circuit code does not unintentionally take ownership of variables that are supposed to be changed/used later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, what do you mean by that
This PR removes all outdated info from zkLLVM docs and replaces it with newly written how-tos
Changelog: