Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

zkvm: non-copyable Program, Expression and Constraint #304

Merged
merged 9 commits into from
May 17, 2019

Conversation

oleganza
Copy link
Contributor

@oleganza oleganza commented May 10, 2019

This adds a non-copyable type Program which is separate from Data type. Instructions call and delegate expect Program type, instruction dup fails if used with a Program.

Also, the Expression and Constraint are made non-copyable and non-droppable. Variables are left to be copyable, so they can participate in constraints and then be used in the construction of Values.

Rationale is covered in #301 & #306.

Closes #301
Closes #306

@oleganza oleganza changed the title zkvm: program type spec zkvm: non-copyable program type May 12, 2019
@cathieyun
Copy link
Contributor

cathieyun commented May 14, 2019

Can you add a test showing that a contract trying to duplicate a program fails? (The tests checking that normal transactions succeed will now check that normal program outputting succeeds, but there is no failure case check here).

@oleganza oleganza changed the title zkvm: non-copyable program type zkvm: non-copyable Program, Expression and Constraint May 14, 2019
@oleganza
Copy link
Contributor Author

@cathieyun added tests for Program, Expression and Constraint. Expanded the scope of this PR to extend to Expressions and Constraints.

cathieyun
cathieyun previously approved these changes May 17, 2019
Copy link
Contributor

@cathieyun cathieyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

bobg
bobg previously approved these changes May 17, 2019
Copy link
Contributor

@bobg bobg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several minor suggestions, spec changes look 💯 overall. (Have not closely reviewed the code changes.)

zkvm/docs/zkvm-spec.md Outdated Show resolved Hide resolved


### Data type

A _data type_ is a variable-length byte array used to represent signatures, proofs and programs.
A _data type_ is a variable-length byte array used to represent [commitments](#pedersen-commitment), [scalars](#scalar), signatures and proofs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeating some feedback from an earlier round of review on this spec: "type" is being used in a confusing way here. You can talk about "the data type" or "an instance of type data" but not "a data type." Consider:

An object of the data type is a variable-length byte array...

(Same thing with "program type" below.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about renaming it into String in some other PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great. 👍

zkvm/docs/zkvm-spec.md Outdated Show resolved Hide resolved
@@ -1298,29 +1314,33 @@ Fails if the `prevoutput` is not a [data type](#data-type) with exact encoding o
_items... predicate_ **output:_k_** → ø

1. Pops [`predicate`](#predicate) from the stack.
2. Pops `k` items from the stack.
2. Pops `k` [portable items](#portable-types) from the stack.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the top k items includes a non-portable item, is that an error or is it skipped? I know you intend it to be an error, but I think it's a little ambiguous as written.

I think the original wording ("Pops k items from the stack") plus the new "fails if" text below is fine.



#### contract

_items... pred_ **contract:_k_** → _contract_

1. Pops [predicate](#predicate) `pred` from the stack.
2. Pops `k` items from the stack.
2. Pops `k` [portable items](#portable-types) from the stack.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment.

@@ -1362,45 +1382,47 @@ _contract(P) proof prog_ **call** → _results..._
7. Places the [payload](#contract-payload) on the stack (last item on top).
8. Set the `prog` as current.

Fails if the top two items are not [data](#data-type) or the third from top is not a [contract](#contract-type).
Fails if:
1. `prog` is not a [program type](#program-type),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about "a program type," "a data type," and "a contract type." I'd much prefer "a program," "a data item," and "a contract."

zkvm/docs/zkvm-spec.md Outdated Show resolved Hide resolved
Co-Authored-By: Bob Glickstein <bobg@users.noreply.github.com>
@oleganza oleganza dismissed stale reviews from bobg and cathieyun via c227f57 May 17, 2019 18:55
@oleganza oleganza merged commit 0f1433f into main May 17, 2019
@oleganza oleganza deleted the oleg/program-type branch July 11, 2019 09:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants