-
Notifications
You must be signed in to change notification settings - Fork 2
feat: use multi-output predicates #49
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Andrew Twyman <artwyman@gmail.com>
…fying the proof from the blobs
Joint work with @ax0 . Integrate multi-outputs crafting into the app_cli, update craftlib predicates & statements building. Co-authored-by: Ahmad <root@ahmadafuni.com>
…t connected to pod logic)
ed255
left a comment
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.
Overall looks good to me! I've left a few comments
|
|
||
| pub subset_of: CustomPredicateRef, | ||
| pub subset_of_recursive: CustomPredicateRef, | ||
| pub batch_def: CustomPredicateRef, |
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 think we're not using most of these fields. We could just remove them.
| // These properties are committed on-chain | ||
| pub inputs: HashSet<Hash>, | ||
| pub key: RawValue, | ||
| // TODO: Maybe replace this with a Value -> Value map? |
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.
Or even usize -> Value and we only use numbers to index an item in a batch?
Update: I saw that we're using strings for index in craftlib. Value for index sounds good if it gives us more flexibility / ergonomics.
commitlib/src/lib.rs
Outdated
|
|
||
| // Build ItemInBatch(item, batch) | ||
| Ok(st_custom!(self.ctx, | ||
| ItemInBatch() = ( |
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 added a rustfmt rule so that this macro doesn't get formatted. This means it must be formatted manually.
Notice that this line is not indented.
If you have any suggestion to improve this I'd be happy to listen!
|
|
||
| // Build ItemDef(item, ingredients, inputs, key, work) | ||
| Ok(st_custom!(self.ctx, | ||
| ItemDef() = ( |
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.
See previous comment. Notice that this line is indented twice.
common/src/payload.rs
Outdated
| write_elems(&mut buffer, &self.item.0); | ||
| write_elems(&mut buffer, &self.created_items_root.0); | ||
| assert!(self.nullifiers.len() <= 255); | ||
| // TODO: What are the constraints on these lengths apart from the type casts below? |
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.
there are no other constraints really.
For convenience we use a fixed size encoding, we think that a max value of 255 ought to be enough so we encode the length as a u8.
But if that's too small we can just use a u16.
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.
On the decoding side we may choose to have an upper limit on the length to avoid abuse, because the ETH cost of committing many items in a batch is the same as committing one single item, but the synchronizer takes more load.
synchronizer/src/main.rs
Outdated
| } | ||
| Err(e) => { | ||
| info!("Invalid do_blob: {:?}", e); | ||
| error!("Ignoring blob due: Invalid do_blob: {:?}", e); |
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 understand the need to highlight this so that it stands out; but I'd like to point out that receiving an invalid do_blob is not an error from the synchronizer point of view.
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.
Makes sense, initially when there was an error due an invalid blob (note: blob supposed to be a DO blob, but with incorrect formatting (eg. wrongly compressed plonky2 proof)) this was not being noticed but in a lower layer it was just panicking (and stopping the synchronizer completely), which is how noticed an error while adding new features (although it was not clear to detect, needed debugging to find out the cause); so from that I set it to 'warn' and returned the error (with ? instead of the old .unwrap()), but it was easy to miss along all the other logs; so, coming from a panic (.unwrap()), the error log seemed the natural step (also since it was a blob labeled as DO type with an error of formatting).
But it's true that there could be cases where somebody sends an invalid blob labeled as DO type without being an actual error, which should not be reported as an error by the synchronizer; before this PR that case would result in the synchronizer panicking, now it will log the error message.
Maybe we can change it to warn? (for sure not going back to panicking)
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 wasn't aware this was panicking before! Thanks for fixing that.
A warn sounds good to me.
craftlib/src/item.rs
Outdated
| let mut s1 = empty_set.clone(); | ||
| s1.insert(&wood).unwrap(); | ||
| let mut inputs = s1.clone(); | ||
| println!("1"); |
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.
these prints should be removed
craftlib/src/item.rs
Outdated
| let batch_hash = batch_def.batch_hash(self.params)?; | ||
| let dust_hash = hash_values(&[batch_hash.into(), DUST_BLUEPRINT.into()]); | ||
| let gem_hash = hash_values(&[batch_hash.into(), GEM_BLUEPRINT.into()]); | ||
| dbg!(&batch_def.ingredients.keys); |
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.
to be removed
app_cli/src/lib.rs
Outdated
| }) | ||
| .collect::<Result<Vec<_>>>()?; | ||
|
|
||
| // TODO instead of 'i', use 'index' (which is the blueprint (which takes |
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.
this TODO sounds good, I suggest applying this already.
…ce PoW of Stone from 3 to 2 (since now proving in general already involves more recursive steps)
c9e0c6e to
a2b32a7
Compare
18726ab to
6a2ddf4
Compare
shell.nix
Outdated
| wayland.dev | ||
| wayland-protocols | ||
| sway | ||
| emacs |
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.
😆
ed255
left a comment
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.
LGTM!
| ItemDef(item, ingredients, inputs, key, work, private: batch, index, keys) = AND( | ||
| BatchDef(batch, ingredients, inputs, keys, work) | ||
| ItemInBatch(item, batch, index, keys) | ||
| DictContains(keys, index, key) |
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.
Why is the DictContains needed if ItemInBatch already includes it?
No description provided.