docs(yp): Palla/yp review contract deployment #4249
Conversation
…ned executed code
iAmMichaelConnor
changed the title
Benchmark resultsMetrics with a significant change:
Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Values are compared against data from master at commit L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 16 txs.
Circuits statsStats on running time and I/O sizes collected for every circuit run across all benchmarks.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contracts are deployed in the tx.
Transaction processing duration by data writes.
|
| public_keys_hash = pedersen([nullifier_pubkey, tagging_pubkey, incoming_view_pubkey, outgoing_view_pubkey], GENERATOR__PUBLIC_KEYS) | ||
| ``` | ||
|
|
||
| <!-- TODO(@iamMichaelConnor): Can we just hash the `x` coordinate for each pubkey and be done with it, for the sake of the hashing? Or should we also include the `y` coordinate? Or at least the sign bit? If we include the sign bit, does it make sense to "compress" all the sign bits in a single field, or is it better to expand it? --> |
There was a problem hiding this comment.
Using just the x coordinate would make me nervous, given a (now-patched) Aztec connect bug which tried to use just an x-coordinate. It might be safe, but it would need thought, so for now we should prioritise security over efficiency.
I suppose the signed bits could be compressed into a single field. We'd need to compare the efficiency of:
- Hashing 8 fields (4 points), vs
- Hashing 5 fields (4 x-coords, and 1 sign-field (Seinfeld?!); bit-decomposing the sign-field; possibly range-checking the sign-field to be <= 4; computing the value of
y; and computing the correctly signedy.
There was a problem hiding this comment.
Can't we just restrict x coordinates to all have the same "sign", similar to how it's done in Ethereum-land during ecrecover?
There was a problem hiding this comment.
Quite possibly. We'll need to find a way to convey such optimisation proposals to the crypto research team, in such a way that they have enough background to comment.
There was a problem hiding this comment.
Discussion in Slack here
| <!-- prettier-ignore --> | ||
| | Field | Type | Description | | ||
| |----------|----------|----------| | ||
| | `function_selector` | `u32` | Selector of the function. Calculated as the hash of the method name and parameters. The specification of this is left to the compiler and not enforced by the protocol. | | ||
| | `function_selector` | `u32` | Selector of the function. Calculated as the hash of the method name and parameters. The specification of this is not enforced by the protocol. | |
There was a problem hiding this comment.
Might a salt still be useful?
If someone uses an off-the-shelf private function, observers of the broadcast function leaves would be able to learn (through a dictionary/rainbow attack) the private functions which have been deployed.
Or would the function leaves not be broadcast if someone wants to keep their private functions private?
There was a problem hiding this comment.
You just made me realize there's no need to broadcast private function selector and vkhash in the ContractClassRegisterer. As long as we broadcast the merkle root, we're good.
The point here is that any user who wants to send a private tx that goes through a private function in a contract will need the contract artifact (which includes the bytecode) anyway, so broadcasting the function selector and vkhash doesn't add anything. The only important thing to broadcast during class registration is public bytecode.
There was a problem hiding this comment.
Ah, good point!
|
|
||
| ## Public Deployment | ||
| Private functions in the contract should require a merkle membership proof for the Initialization Nullifier, to prevent them from being called before the constructor is invoked. Nevertheless, a contract may choose to allow some functions to be called before initialization, such as in the case of [Diversified and Stealth account contracts](../addresses-and-keys/diversified-and-stealth.md). Public functions, on the other hand, should not need this check, since the Public Kernel Circuit guarantees that the contract is Publicly Deployed, and a contract cannot be Publicly Deployed without being Initialized. Note that every Contract Class needs to have at least one constructor if an instance of it is intended to be Publicly Deployed. |
There was a problem hiding this comment.
Note that every Contract Class needs to have at least one constructor if an instance of it is intended to be Publicly Deployed.
Is this true? Is it not possible for the ContractInstanceDeployer to emit an Initialisation Nullifier where the underlying initialization_hash is 0?
There was a problem hiding this comment.
No, because the initialization hash is emitted from the contract itself, not from the Deployer.
Still, we can remove the restriction that "a contract needs to be initialized in order to be publicly deployed" as suggested in "Allowing Uninitialized Publicly Deployed Instances", which simplifies things (for us).
| What should `contract_args_hash` be if there is no constructor? `0`? | ||
| Q: why "contract_args_hash" and not "constructor_args_hash"? | ||
| --> | ||
| Note the absence of a deployer field. This is because the deployer address is optionally mixed into the `salt` as part of the deployment logic, implemented by the `ContractInstanceDeployer` as described below. |
There was a problem hiding this comment.
Is this as an optimisation to reduce hashing?
Readers might get confused that there's no "deployer address", but then most conventions described below describe a deployer address being mixed with the salt. It might be clearer to keep both separate concepts (deployer and salt)?
Or maybe I'm just not appreciating why this has been done
There was a problem hiding this comment.
You're right. It was meant as an optimization, since deployer was not enforced by the protocol at all, but rather by the deployer contract, and it could be mixed-in with the salt. But it ends up being more confusing, so better just keep it as a separate field. I'll add it back.
There was a problem hiding this comment.
Added the deployer again to the struct, which gets hashed along with the init_args and salt when computing the address:
salted_initialization_hash = pedersen([salt, initialization_hash, deployer as Field, portal_contract_address as Field], GENERATOR__SALTED_INITIALIZATION_HASH)
partial_address = pedersen([contract_class_id, salted_initialization_hash], GENERATOR__CONTRACT_PARTIAL_ADDRESS_V1)
address = pedersen([public_keys_hash, partial_address], GENERATOR__CONTRACT_ADDRESS_V1)
The
deployeraddress of a contract instance is used to restrict who can initialize the contract (ie call its constructor) and who can publicly deploy it. Note that neither of these checks are enforced by the protocol: the initialization is checked by the constructor itself, and the deployment by theContractInstanceDeployer. Furthermore, a contract class may choose to not enforce this restriction by removing the check from the constructor.The
deployeraddress can be set to zero to signal that anyone can initialize or publicly deploy an instance.
|
|
||
| However, this opens the door for a contract to be simultaneously Publicly Deployed and Uninitialized, which is a state that does not seem to map to a valid use case. Furthermore, it requires public functions to check the Initialization Nullifier on every call, which in the current approach is not needed as the presence of the Deployment Nullifier checked by the Public Kernel is enough of a guarantee that the contract was initialized. | ||
|
|
||
| <!-- To @iAmMichaelConnor: I am tempted to actually implement this approach in spite of its drawbacks. It makes responsibilities so much more clearly defined: Deployer deploys, Initializer initializes, and there is no link between the two. It also allows removing constructors when they are not needed. And the added cost of checking the initialization nullifier from public functions doesn't seem too terrible. --> |
There was a problem hiding this comment.
I might be a bit confused. Is the purpose of "deploy" solely to enforce the bytecode to be broadcast?
If so, I suppose the sequencer would only need to check for a deployment nullifier if it doesn't exist in the tree. If the deployment nullifier does exist, the sequencer doesn't need to prove that fact, because they clearly have the bytecode and they can prove this bytecode is correct via the initialisation nullifier.
If the merkle membership/non-membership check for the deployment nullifier is done in the kernel circuit, then this observation doesn't help much: the kernel circuit is a fixed circuit with no branching, so the hashing will always happen. But if there's a way for the hashing to be done within the AVM, then the sequencer can skip the merkle membership proof unless they want to claim gas fees for a non-deployed contract.
There was a problem hiding this comment.
Is the purpose of "deploy" solely to enforce the bytecode to be broadcast?
Yep!
If the deployment nullifier does exist, the sequencer doesn't need to prove that fact, because they clearly have the bytecode and they can prove this bytecode is correct via the initialisation nullifier.
Not sure I agree. Even if the sequencer can prove that they know the bytecode, it's important they prove that everyone knows the bytecode of the public function being executed. Otherwise, we open the door to public functions that can only be run by a subset of nodes that are privy to that bytecode, which doesn't feel right.
There was a problem hiding this comment.
If the merkle membership/non-membership check for the deployment nullifier is done in the kernel circuit, then this observation doesn't help much: the kernel circuit is a fixed circuit with no branching, so the hashing will always happen.
Good point! Given this, I'm even more tempted to go down this route and completely split initialization and deployment.
But if there's a way for the hashing to be done within the AVM, then the sequencer can skip the merkle membership proof unless they want to claim gas fees for a non-deployed contract.
Not sure. Feels doable, but it also feels like we're pushing into the AVM a responsibility that is very clearly the kernel's.
|
@spalladino Thanks for making these changes. I went through and wrote some comments, and asked some questions :) |
|
Some unsolicited feedback as I was learning about this: Mat have missed it, but the section should probably cover account contracts and how they would work. I haven't fully digested our forum discussion on this but it felt like that had some things that aren't mentioned in the YP 🤷 Btw Mike told me that "You can use a contract before deploying if it doesn't have a constructor" - this isn't immediately obvious after reading the YP. Whether it deserves to be in the YP is a different matter. |
Addressed in a new commit!
Hmm disagree, since account contracts are deployed same as any other contract. Still, I'm not sure what's the best place for account contracts to be discussed in the yellow paper (assuming they have to be included).
Can you elaborate on which ones?
That's covered in the "Statuses" section of the instances doc, but added a few more sentiences making sure it's clear. |
spalladino
merged commit 69b0933
into
mc/yp-review-contract-deployment
Please read [contributing guidelines](CONTRIBUTING.md) and remove this line. --------- Co-authored-by: Santiago Palladino <[email protected]>
Please read contributing guidelines and remove this line.