feat!: prover input run on native arch #418
Conversation
antoniolocascio
force-pushed
the
alocascio-prover-input-run
branch
4 times, most recently
from
87e48b7 to
b10b28e
Compare
antoniolocascio
force-pushed
the
alocascio-prover-input-run
branch
from
e50f65d to
982db5c
Compare
antoniolocascio
force-pushed
the
alocascio-prover-input-run
branch
from
982db5c to
261e5bf
Compare
To be used in testing
AntonD3
left a comment
AntonD3
left a comment
There was a problem hiding this comment.
Overall this looks great, left a few minor comments and questions
| zksync_os_evm_errors = { git = "https://github.com/matter-labs/zksync-os-interface", branch = "alocascio-prover-input-gen", default-features = false } | ||
| zksync_os_interface = { git = "https://github.com/matter-labs/zksync-os-interface", branch = "alocascio-prover-input-gen", version = "0.0.10"} |
There was a problem hiding this comment.
Thread to remember to merge this and fix here
antoniolocascio
force-pushed
the
alocascio-prover-input-run
branch
5 times, most recently
from
9d6adc3 to
9327836
Compare
antoniolocascio
force-pushed
the
alocascio-prover-input-run
branch
from
9327836 to
feaf0c4
Compare
AntonD3
left a comment
AntonD3
left a comment
There was a problem hiding this comment.
Overall looks good, there are some things that are not very clear for me around arithmetic advice(asked questions in comments), and please consider that it requires non-trivial integration from the platform side - pubdata is now not available from forward run
| unsafe { | ||
| let num_digits = to_consume.next_multiple_of(8) / 8; | ||
| const BIGINT_DIGIT_USIZE_SIZE: usize = U256::BYTES / core::mem::size_of::<usize>(); | ||
| let num_digits = |
There was a problem hiding this comment.
it's confusing, I see that you haven't change it, but probably better to name this variable num_bigints if I undertand it correctly
There was a problem hiding this comment.
I agree, but I see the term "digit" being used for it in other methods/interface (even on the other modexp impl). If you don't mind, I would not change it here, as the PR is unrelated
| } | ||
| // We use different advice params depending on architecture | ||
| // Both are mostly the same, main difference is the width of pointers | ||
| #[cfg(target_arch = "riscv32")] |
There was a problem hiding this comment.
maybe better to have cfg based on pointer width, so it will work on non risc 32 bit architectures
There was a problem hiding this comment.
Hmm we have #[cfg(target_arch = "riscv32")] all over the place, we don't really support other 32 bit arch.
| let packed_lens = it.next().expect("packed lengths"); | ||
| let q_len = (packed_lens & 0xFFFF_FFFF) as usize; | ||
| let r_len = (packed_lens >> 32) as usize; | ||
| (it, q_len / 2, r_len / 2) |
There was a problem hiding this comment.
why do we have to divide by 2?
| pub b_len: u32, // Length of divisor in words | ||
| pub modulus_ptr: u32, // Pointer to modulus | ||
| pub modulus_len: u32, // Length of modulus in words | ||
| pub struct ModExpAdviceParamsGeneric<W> { |
There was a problem hiding this comment.
This type is used at the interface of the oracle (it's created in the ArithmeticQuery oracle processor, which runs on native), so I wanted to avoid it being architecture-dependent.
| let quotient = strip_leading_zeroes(&self.quotient); | ||
| let remainder = strip_leading_zeroes(&self.remainder); | ||
|
|
||
| // account for usize being u64 here |
There was a problem hiding this comment.
nit: should we assert that usize indeed is 64 bit?
There was a problem hiding this comment.
This crate is not part of the risc-v binary (and it should never be). We don't really support 32-bit arch for sequencing, so I would say it's not needed
Benchmark report
|
2 participants
What ❔
This PR changes the way we compute prover input. Before, we needed to run the slow risc-v simulator to do so.
Now, we can do on native architecture.
This run is also responsible for computing the pubdata.
Why ❔
Is this a breaking change?
Checklist