Remove all preinit memory #1610
Conversation
All accesses to tapes are through ecalls, now that we are removing preinit memory.
This reverts commit 5f04cde.
|
Reverted SDK changes in e3c4ece - seems too big for this PR. I'll focus on runner changes for now, and leave SDK alone. |
Cached would incorrectly call self.evaluator.expr_tree, which would prevent it from checking if nested nodes are already in value_cache. We also prevent clippy from suggesting to use entry API for HashMap as the function body is impossible to implement using entry API.
We can't actually specify `git` dependencies twice. So only do it once.
supercedes #1552 Due to some limitations, the previous CLI PR used 2 plans instead of 1, which should be the correct approach. Additionally, instead of a _plan_, we use the system tape as the plan instead, because we have everything we need there. Few changes you might notice: - The first program is implied to be the entrypoint program. This is where we get the call tape hash. - We now only require `--system-tape <SYSTEM_TAPE>` to build a transaction bundle. (You can test this with the test script committed in this PR.) - We only dump the tape and its debug version now. Removed `ProofBundle` and the plan output completely. - Important to note that we also rely entirely on the ECALL tables - not the init tables for bundling. - You may see some redundant usages of `RuntimeArguments`... these are for preinit-memory, but since they are not of concern this PR, we will just blindly use them for now (even if there's no effect), and remove these usages in a followup PR. --------- Co-authored-by: Matthias Görgens <[email protected]>
fns. Now that we don't have preinit memory, we no longer need these structs.
Also delete all code related to initialization with preinit memory - this includes all helpers and other rkyv code.
| }; | ||
|
|
||
| // Populate partial buf from newly fetched bytes via `ECALL` | ||
| buf[populatable_from_internal_buf..].clone_from_slice(&self.internal_buf[old_len..]); |
There was a problem hiding this comment.
This seems very weird. Why don't you just point the ecall at the portion of the buffer you are now copying into?
There was a problem hiding this comment.
We pointed the ecall to the internal_buf, which allows for seekability later on, unless i'm missing your point here
There was a problem hiding this comment.
OK, that makes sense (if you believe that the internal buffer makes sense..)
matthiasgoergens
changed the title
| pub struct RandomAccessPreinitMemTape { | ||
| pub tape: Box<[u8]>, | ||
| pub struct RandomAccessEcallTape { | ||
| pub ecall_id: u32, |
There was a problem hiding this comment.
What is this good for? (And should it be an enum instead?)
There was a problem hiding this comment.
It's not an enum, because our VM requires it in u32. I tried refactoring this before, and it resulted in a lot of ugly .into() code.
Otherwise, we can also do the same for the rest of the ecall constants declared as u32s, which I would honestly prefer, since we have a few match statements in the code that match on ecalls and an enum would allow us to catch missing ecalls at compile-time. I shot myself in the foot with this by forgetting to include one of the ecalls in our few match statements before.
There was a problem hiding this comment.
As for what this is good for, we tie it to the tape involved so that we can call invoke its id for its corresponding ecall, so that whoever is writing the guest doesn't have to think about this.
| let raw_tapes = raw_tapes_from_system_tape(system_tape, self_prog_id.unwrap().into()); | ||
| let program = load_program(elf).unwrap(); | ||
| let state: State<F> = State::new(program.clone(), raw_tapes); |
There was a problem hiding this comment.
this looks much neater now! 👍
| "ecall", | ||
| in ("a0") SELF_PROG_ID_TAPE, | ||
| in ("a1") buf_ptr, | ||
| in ("a2") buf_len, |
There was a problem hiding this comment.
buf_len could be hardcoded here, isn't it?
… digest (#1683) We used the magic number `32`, or in other places as `COMMITMENT_SIZE`, in various places. This PR creates a public module `constants` in `sdk/common` and re-exports `DIGEST_BYTES` found in the `poseidon2hash` module as a pub const. Addresses #1610 (comment) as well --------- Co-authored-by: Kapil <[email protected]> Co-authored-by: codeblooded1729 <[email protected]>
A simple cleanup to make #1610 easier to review. --------- Co-authored-by: bing <[email protected]>
Remove (non-ELF) pre-init memory from both runner and circuits.
Overall, this cuts out all the pre-init tables (-5) but adds a new table for
self_prog_id(+1) (which we no longer have access to via pre-init memory), so we save 4 tables in total.A huge chunk of the diff is adding the template code (constraints, generation, etc.) that we add for a new
StorageDeviceStarkfor self prog id, and removing all the template code for the preinit starks.The key thing to review here is the logic to populate
SystemTapefrom ecalls, which is found insdk.