-
Notifications
You must be signed in to change notification settings - Fork 112
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
Refactor offchain state #1834
base: main
Are you sure you want to change the base?
Refactor offchain state #1834
Conversation
// We also can't rely on different instances of the contract having different offchain state instances, because this specific instance of | ||
// offchain state is referenced by the class definition of the smart contract. | ||
// By using a closure and exporting the offchain state and the contract together, we can always refer to the correct instance of the offchain state | ||
offchainState = offchainStateInstance; |
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 line is the source of a lot of frustration. Here are some alternatives attempted:
offchainState = offchainState.init()
- Re-initialized the state every time the prover is invoked. The smart contract is not capable of staying up to date with the contract state.
OffchainState#memoizedInstances
- Attempted to store previously initialized instances in a map in global state to get around the re-init problem, but ran into issues trying to read variables in provable code. E.g. if we use the contract address as a key, the prover blows up.
offchainState: typeof offchainStateInstance
- The contract can't compile because provable methods rely on offchainState being defined at compile time
That leaves the implementation as implemented here. The instance offchainStateInstance
is tied to the class definition of the smart contract. That means 2 or more smart contract instances will all have the same offchain state instance. Even if we reset the instance state after initialization, the prover will re-run this line and set the offchain state back to the orifinally-defined value.
I found the best/only way to manage this is to turn the whole smart contract class into a closure so that the definition of each smart contract can be paired with its offchain state instance. This doesn't feel amazing, but it does unblock the use case.
ExampleContract: ExampleContractB, | ||
} = ExampleContractWithState(); | ||
|
||
const Local = await Mina.LocalBlockchain({ proofsEnabled: true }); |
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.
testLocal
accepts a single contract instance only, so I re-implemented the behavior here for the specific case I needed.
|
||
console.time('compile contract'); | ||
await ExampleContractA.compile(); | ||
await ExampleContractB.compile(); |
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.
Contracts have the same _verificationKey.hash
, but _provers
need to be recompiled, or else all the contracts will point to the same instance of offchain state again.
Love it |
Given that these modifications affect both the public API and developer usage patterns, we should evaluate options for maintaining backwards compatibility. If backwards compatibility isn't feasible, we may have to consider targeting V2 for these changes |
@Trivo25 this is in the experimental namespace, so do we need to be worried about compatibility? I do think an accompanying PR in the docs is required to match this change before release. |
That's a good point, then probably not |
@45930 sounds like that could be solved with |
Ok, I think I have a working branch locally that works as expected with the memoization happening "behind the scenes". I don't love that this is hidden from the developer, but I like it better than forcing them to use a closure. WDYT?
OR
|
@mitschabaude , bf0e09e is pretty clean! I think this API is a lot less error-prone and requires less deep knowledge to use. But it does add more secret global state, which tends to cause its own issues down the line. |
init(): void { | ||
super.init(); | ||
this.offchainState.setContractInstance(this); | ||
} |
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.
in general, this will be the wrong place for connecting the offchain state. init()
is only called on initial deployment. can't we move this into offchainState.init()
?
instance.setContractClass( | ||
contractInstance.constructor as OffchainStateContractClass<Config> | ||
); | ||
memoizedInstances.set(key, instance); | ||
} |
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.
any reason not to do instance.setContractInstance(contractInstance)
here?
Summary
The experimental offchain state API currently uses a singleton class to manage storage on behalf of a smart contract. This blocks the use case where many instances of the same smart contract need to be instantiated at the same time, because they will all use the same offchain state under the hood, leading to data mismatches.
This PR splits the offchain state API into
OffchainState
andOffchainStateInstance
such that theOffchainState
can be compiled into a circuit definition, and theOffchainStateInstance
stores the internal data like which contract instance it is associated with and the merkle trees of data.Note
I called it out in line, but please note that there is still an ongoing DevX problem where the smart contract class definition has to reference a specific offchain state instance in order to compile. That leaves us in the same problem, where unless handled by the developer, multiple contracts will point to the same storage. There is now a viable workaround, but it doesn't work as nicely as we'd like. I'd appreciate any input to overcome this issue.
Closes: #1832