Skip to content
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

Support multiple parties (flat) #280

Merged
merged 14 commits into from
Jul 21, 2022
Merged

Support multiple parties (flat) #280

merged 14 commits into from
Jul 21, 2022

Conversation

mitschabaude
Copy link
Member

@mitschabaude mitschabaude commented Jul 14, 2022

  • Stacked on top of Expose constraint_system for running circuits in analyze mode #279
  • Closes Support other parties in a proof transaction #236
  • This doesn't deal with parties hierarchy yet, but with supporting multiple parties in a flat list
  • To support multiple parties, we had to consolidate the various "global states" we were having. Most of this PR is refactoring global state.
    • Previously, inside proofs we used a global state that wasn't compatible with the global state for normal transaction construction, and was also mixed with another snark-related global context.
    • Now, we have just one transaction-related global context and one snark-related one. They both are instances of a new Context type, which encapsulates the logic needed to enter and leave a global context, and throws errors when invariants are violated (which indicates that async functions relying on global context are run in parallel)

General notes on global state

Maintaining some kind of global state, which determines what function calls like Field.add() do under the hood, is very fundamental to how snarkyjs works. Thus, IMO we will never properly support -- for example -- running multiple async smart contract methods concurrently, where Field.add() should add a constraint / compute a witness in one constraint system inside one method, and to a different constraint system inside a different method.

The underlying problem is that with async execution, the notion of "I am in this function right now" gets lost. That's why React components, which call useState(), which returns something different depending on the component, can also not be async functions, for example.

The solution I'd advocate for is to accept this incompatibility with async execution and just make sure that the (async) prover is structured in such a way that the parts which rely on global state (probably, witness generation) are done synchronously before the async part is kicked off.

EDIT: Actually, probably it's better to allow async methods and instead consequently disallow running them concurrently

@mitschabaude mitschabaude changed the base branch from feature/constraint-system to main July 14, 2022 14:39
@mitschabaude mitschabaude changed the base branch from main to feature/constraint-system July 14, 2022 14:39
Copy link
Member

@bkase bkase left a comment

Choose a reason for hiding this comment

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

Just a bunch of questions

src/lib/mina.ts Outdated
Circuit.runAndCheck(f);
let err: any;
while (true) {
// this is such a ridiculous hack
Copy link
Member

Choose a reason for hiding this comment

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

lol what's happening here? Can you add an explanation in a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

explained here: #280 (comment)
Ok will add a better comment!

src/lib/party.ts Outdated
// to avoid that, provide the argument explicitly)
// if not specified, determine isSameAsFeePayer from the current transaction
// (gotcha: this doesn't constrain `isSameAsFeePayer`, to avoid making the circuit depend on something that can't be
// inferred at compile time. to constrain it, provide the argument explicitly)
Copy link
Member

Choose a reason for hiding this comment

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

what does constraining it look like? What are the implications to leaving it unconstrained?

Copy link
Member

Choose a reason for hiding this comment

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

Is there something that developers need to understand here to write safe contracts?

Copy link
Member Author

Choose a reason for hiding this comment

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

isSameAsFeePayer determines if we increment the nonce precondition by one, because the fee payer already increments the nonce once. For some reason I thought that we should do that inside the circuit. Now that I think about it more clearly, I see no reason at all to do the extra nonce incrementing in the circuit. There's no attack vector to modifying the nonce -- we set a nonce precondition anyway, so it has to be the current one to be accepted. I think this comment is just me being confused and erring on the paranoid side when writing code that will end up in smart contract circuits :D

So I'm inclined to delete those comments, move the nonce computation into a Circuit.witness block entirely and even remove the isSameAsFeePayer argument.

It gets more complicated though, because incrementing the nonce here is the wrong behavior now, to be addressed in #271. In the future, isSameAsFeePayer will determine whether to set useFullCommitment to true, or do a nonce increment. So there are four variables which would all get determined at runtime from the fee payer on the current transaction:

  • party.body.useFullCommitment (a Bool)
  • party.body.incrementNonce (a Bool)
  • party.body.preconditions.account.nonce.lower
  • party.body.preconditions.account.nonce.upper

Maybe the same reasoning applies in the future as well. We can let the prover set all four variables to whatever they want. If they want to be less restricitve and set both useFullCommitment and incrementNonce to false, the transaction will just fail (for lack of replay protection). If they are more restrictive, i.e. enable more replay protection than required, they just strictly reduce the malicious power of this transaction (it might fail because of a wrong precondition or a mismatch in the "full commitment"). So, again I see no attack vector.

Copy link
Member Author

Choose a reason for hiding this comment

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

TL;DR, I think we can avoid leaking these complications to developers and still have them write perfectly safe contracts

let err = new Error(
'Can not analyze methods inside Circuit.runAndCheck, because this creates a circuit nested in another circuit'
);
// EXCEPT if the code that calls this knows that it can first run `analyzeMethods` OUTSIDE runAndCheck and try again
Copy link
Member

Choose a reason for hiding this comment

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

when does this happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, the problem is for smart contracts which use neither compile nor digest, but just get run in a Mina.transaction block. This means that analyzeMethods is never called before that block. If analyzeMethods is called once, it caches its results so it never has to run the methods again.

Now, the Reducer.reduce function actually needs the analyzeMethods output, so it calls it under the hood. If it hasn't been run before, it will run inside Circuit.runAndCheck(f) inside createTransaction: https://github.com/o1-labs/snarkyjs/pull/280/files#diff-1f92503aa572cd2ea7bc843e4e77809f3aef81f49bb25995a80d032127482ffcR106
This fails because of the circuit nesting (AFAIU), so I thought createTransaction should do what this comment says -- try to re-run analyzeMethods first and then Circuit.runAndCheck(f) again.

It would be simpler to just call smartContract.analyzeMethods at the beginning of createTransaction for each smart contract. The problem is that createTransaction doesn't know which smart contracts are involved in the transaction, it's just executing some function the user gave it. That's also why we stick a .bootstrap function on the error here, so createTransaction knows what to call.

I wonder if we can come up with something better eventually :D If we get rid of the await isReady requirement, maybe we could just run the equivalent of analyzeMethods for one method at a time, right inside the method decorator.

Base automatically changed from feature/constraint-system to main July 18, 2022 22:07
@mitschabaude mitschabaude merged commit 7a635e0 into main Jul 21, 2022
@mitschabaude mitschabaude deleted the feature/many-parties branch July 21, 2022 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support other parties in a proof transaction
4 participants