Feat: Private input integration #622
Conversation
naure
force-pushed
the
feat/private-input-2
branch
from
dba2a79 to
acccad7
Compare
_Follow-up to #620_ Simplify the `Platform` type by exposing the `Range` type. Co-authored-by: Aurélien Nicolas <[email protected]>
| let priv_io = memory_from_file(&args.private_input); | ||
| for (addr, value) in zip(platform.private_io.iter_addresses(), &priv_io) { | ||
| vm.init_memory(addr.into(), *value); | ||
| } |
There was a problem hiding this comment.
should we add a comment to say, if len(private_io) != len(private_input_file), then it is right-padded with 0s?
There was a problem hiding this comment.
Good point. It is not padded. Or precisely it is zero-padded only to the next power-of-two size.
The rest of memory does not exist - unsatisfiable if the program tries to use it.
(added a comment and assert)
There was a problem hiding this comment.
For technical reasons, if we are using rkyv, it would be better to pad from the left by default.
That's means the last byte that's specified should always appear in the same position in the VM.
The rest of memory does not exist - unsatisfiable if the program tries to use it.
That's good, and exactly what we want. Can you please make both the doc comment and the emulator reflect that behaviour?
Btw, would it be possible to make both kinds of padding be 'inaccessible' (from the point of view of the VM), instead of a mix of 0 and 'inaccessible'? That would be best!
naure
force-pushed
the
feat/private-input-2
branch
from
fcf2e57 to
4f15ceb
Compare
There was a problem hiding this comment.
For rkyv it would be best if we could pad on the left, ie the last byte specified by the user of the prover should always appear in the same address in the VM. Where the first byte appears should be a function of the length of the input specified.
As far as I can tell, we pad the input given with 0s to the next power of two, and then we pad the rest with ‘inaccessible bytes’. That’s good, but we need them from the left.
We can either hard code this to come from the left, or we can make it so that the ‘core’ functions always need the placement specified exactly, and then have a relatively thin shell of helper functionality on the outside that does the necessary calculations.
(The latter would be useful, if we want to explore other deserialisation schemes, because I suspect rkyv is a bit special in wanting left padding. But we can also just hardcode it now, because having extra mostly unused customisation options around is also a hassle in testing and maintenance.)
There was a problem hiding this comment.
Please see my comments.
Specifically I want to highlight two points:
- left padding is more useful that right padding for us.
- could we unify the padding to be 'inaccessible' values only, instead of a mix of 0 and 'inaccessible'?
If we do right padding by default, please leave a comment that it's because of rkyv. If you make it configurable, then we'll leave the comment on one of the outer layer helper functions.
| @@ -94,6 +101,17 @@ fn main() { | |||
| let elf_bytes = fs::read(&args.elf).expect("read elf file"); | |||
| let mut vm = VMState::new_from_elf(platform.clone(), &elf_bytes).unwrap(); | |||
|
|
|||
| tracing::info!("Loading private input file: {:?}", args.private_input); | |||
There was a problem hiding this comment.
On the discussion about private IO, someone suggested to use the term 'hint' throughout?
('Private input' works perfectly fine for me; but I remember some people having strong opinions on this.)
There was a problem hiding this comment.
Yeah, I favor hint over private input. Some places that use this terminology:
There was a problem hiding this comment.
Thanks for the ref. If the term "hint" is understood I’ll rename things to that.
There was a problem hiding this comment.
No worries, you can do that after you finish the sproll-evm test.
A follow-up PR after #622 In #622 integration test was failed in first place. A proposed fix in comment #622 (comment) is to skip the entire proof when table size should be 0. This fix is reasonable and fit the expectation, however, the integration test should NOT failed even BEFORE this fix. It should be garbage-in, garbage out and shouldn't break the proof. ### Root cause The root cause is when table size is 1 (we will padding next size which is 2) or 2, in tower sumcheck there will be empty rounds. In tower sumcheck verifier, there are bookkeeping variables to tracking each layer (evaluation, point), and bookkeeping variables will keep updated when goes through layer by layer verification. However when table size is 2, there is only one layer, so bookkeeping variables skip updated. However in this case, default value was wrong. The correct "default" value of bookkeeping variable should be the output layer evaluations. This PR correcting the default value design. Also add simple test to verify the logic. Co-authored-by: Wu Sung-Ming <[email protected]> Co-authored-by: naure <[email protected]>
Issue #614.
Pending a solution in #625