-
Notifications
You must be signed in to change notification settings - Fork 8
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
QIP-0014 CREATE and CREATE2 specification #44
base: master
Are you sure you want to change the base?
Conversation
1. The CREATE opcode will generate an address using the following method: | ||
|
||
a. Split the bytecode_data into two parts: | ||
- `hashCode`: All but the last 4 bytes of bytecode_data |
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.
What if a user provides a valid contract but no salt?
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.
then it is not a valid contract, according to this spec
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.
It would be a valid contract and then result in a grinded address from the EVM.
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.
if they did not provide salt? the last 4 bytes of the data must always be interpreted as salt (otherwise ambiguity can lead to insecure edge cases). In that case, anyone that tries to interact with a contract, will ignore the last 4 bytes when they interpret the contract code.
Anyone that did not provide salt at last 4 bytes, will probably have a problem when the last 4 bytes are ignored (presumably initializer data)
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.
Correct, the last 4 bytes of data would also be interpreted as a salt. A contract is allowed to have appended extra data. If the contract bytecode itself has last 4 bytes that result in a valid address then it would not have extra 4 bytes appended.
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.
It's impossible to know without EVM execution whether the 4 bytes at the end are actual bytecode or appended extra data.
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 comments
README.md
Outdated
@@ -18,5 +18,6 @@ Those proposing changes should consider that ultimately consent may rest with th | |||
| [8](qip-0008.md) | Consensus (hard fork) | Dynamic Tree Expansion | wizeguyy | Standard | Draft | | |||
| [9](qip-0009.md) | Consensus (hard fork) | Interlinks | gameofpointers | standard | Draft | | |||
| [10](qip-0010.md) | Consensus (hard fork) | Network Object Identifiers | wizeguyy | standard | Draft | | |||
| [13](qip-0013.md) | Applications | Reusable Payment Codes | wizeguyy | standard | Draft | |
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 PR should be for QIP-14, not QIP 13. Please fix the ToC to link to qip-0014 and remove the qip-0013.md file
Or, more precisely: drop the qip-13 commit from this PR, and fix the ToC
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.
Will do
qip-0014.md
Outdated
@@ -0,0 +1,170 @@ | |||
``` | |||
QIP: 14 | |||
Layer: EVM |
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.
EVM isn't a valid layer. Should be one of <Consensus (soft fork) | Consensus (hard fork) | Peer Services | API/RPC | Applications>
from QIP-0001
I believe this counts as Consensus (hard fork)
|
||
## Abstract | ||
|
||
This QIP proposes modifications to the CREATE opcode in Quai Network to ensure shard-specific address generation, while maintaining CREATE2 functionality as is. The proposal aims to adapt Ethereum's address creation mechanism to Quai's sharded architecture. |
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 would prefer not to change the behavior of existing opcodes, and instead propose a new opcode (OP_CREATE3) which does what you want. Changing the behavior of CREATE or CREATE2 would break EVM compatibility
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 is a great point. I would query @jdowning100 or @mechanikalk on the downstream impact on contract deployment. Many contracts that use create (AMMS/NFTs/proxy contracts/etc.) are using solidity and expecting CREATE
to result in a valid address no matter what. So we would need to define whether it is more important for the result of CREATE
to match expectation or the behavior of CREATE
to match expectation.
We would likely need to add this new CREATE3
opcode in solidity as well if we choose the latter.
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.
The middle ground which I proposed to @mechanikalk is hashing all of the bytecode during the grinding since then the CREATE
computation keccak256(([sender_address, sender_nonce, bytecode])[12:]
fits the prior spec. If we include the grinding, this comes at the cost of the keccak on the entire length of the bytecode which is more expensive.
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.
Yeah, I understand this would require updating any contract which uses CREATE to use CREATE3 instead, but that is definitely the preferred way to do this. We can even deprecate CREATE and CREATE2 if we think that those methods of contract deployment are ill-advised.
Deprecating old opcodes and supplanting them with new opcodes is the way to extend an instruction set, like the EVM. Changing the behavior of existing opcodes is dangerous.
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 invite @mechanikalk @jdowning100 to weigh in
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 agree on @mechanikalk and @jdowning100 weighing in. We implemented the current CREATE logic during Iron Age in order to get AMMs / contracts deploying contracts to work in a minimal fashion. Relying on Chesterton's Fence principle we should further assess what caused us to implement it in CREATE versus making CREATE3.
2. If the resulting address is not valid for the current shard: | ||
The gas cost per iteration of bytecode modification and checking is the `KeccackGasCost` of the size of `concat(sender_address, sender_nonce, hashCodeHash, salt)`. | ||
|
||
a. Initialize a "grind nonce" to 0. |
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 init to zero? Should start with the nonce the user already provided.
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.
The salt is assumed to be part of the bytecode unless the user provided one is not valid. This QIP defines the first check of the salt potentially be part of the bytecode itself. The salt is then appended if the prior four bytes (bytecode or extra data) does not result in a valid address.
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.
The salt is assumed to be part of the bytecode
This is not a good idea. When I interpret contract data later, how am I supposed to know if the last 4 bytes are actual contract data, or if they are just salt for the address? This could potentially lead to issues, if a contract was interpreted one way at deployment, but then interpreted differently when it is executed.
The salt needs to be explicitly specified for every contract.
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.
Could you provide a scheme that perfectly separates the bytecode and salt? What if the bytecode itself results in a valid address?
`address = keccak256([sender_address, sender_nonce, keccak256(hashCode), salt])[12:]` | ||
|
||
e. If the address is still not valid, increment the grind nonce and repeat steps b, c, and d until a valid shard-specific address is found, until 1,000 tries has been exceeded, or the gas limit has been exceed. | ||
d. Exceeding 1,000 tries without finding a valid shard address will result in a error, thus failing the contract deployment. |
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 arbitrarily cap at 1000 iterations? This is the reason the gas limit exists. If a user is willing to pay the gas for 1001 iterations, then they should get all 1001 iterations.
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 agree with this approach. 1,000 tries was from the prior implementation.
7b95882
to
656135d
Compare
Creating a standard for how CREATE and CREATE2 are utilized in Quai to comport with shard address requirements.
PRs with associated implementation:
dominant-strategies/quais.js#271
dominant-strategies/go-quai#1980