-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Validated EVM Contracts #2348
Validated EVM Contracts #2348
Conversation
Signed-off-by: Danno Ferrin <[email protected]>
Signed-off-by: Danno Ferrin <[email protected]>
* remove 1985 references, I don't see it applying * enumerate invalid opcodes * embedded links to othe EIPs * style on backticking numbers and opcodes Signed-off-by: Danno Ferrin <[email protected]>
Signed-off-by: Danno Ferrin <[email protected]>
Signed-off-by: Danno Ferrin <[email protected]>
Signed-off-by: Danno Ferrin <[email protected]>
Signed-off-by: Danno Ferrin <[email protected]>
EIPS/eip-2348.md
Outdated
--- | ||
|
||
<!-- | ||
HTML checker won't let me reference unpublished EIPs, namely 2327, 1707, 1712 |
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.
You can still put these into a references section or into the rationale.
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.
They are referenced alongside their ideas in the specification body. I would have preferred to also have them in the header.
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.
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.
1707 is abandoned in the PR text and getting wei to publish his EIPs in this repo is problematic at the moment, so I have links to the PRs.
There is also the issue that they both need some minor revisions. 1707 needs to have the reference to WASM removed as that is out of scope of the "minimum required changes", and 1712 does not have accomidation for future evolution and only references PUSHn
in the multibyte list, as well as not accounting for BEGINDATA
. So I could not reference those EIPs as the text currently stands. And getting Wei to update the texts at the moment is blocked for reasons orthogonal to the content of those EIPs.
EIPS/eip-2348.md
Outdated
<!-- | ||
HTML checker won't let me reference unpublished EIPs, namely 2327, 1707, 1712 | ||
requires: 1702, 2327 | ||
replaces: 615 (in part), 1707 (unpublished, abandoned), 1712 (unpublished) |
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.
Btw strictly speaking the replaces
field can only reference Final (aka. deployed) EIPs.
Lets merge #2327 first so that can at least be properly referenced. |
EIPS/eip-2348.md
Outdated
|
||
For purposes of EVM Program Counter calculations the first byte after the version header is location | ||
`0`. The contract header is not part of the accessible contract data. There is no mechanism within | ||
the EVM to retrieve the header values. |
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.
There is no mechanism within the EVM to retrieve the header values.
CODECOPY
?
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.
Good catch, but how does this reconcile with the start location? Do we keep PC=0 at zero and specify the EVM should skip the first 4 bytes of such a contract? Or do we let it not match?
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.
added three options to deal with this in the EIP text. Only one will make it out of draft.
Signed-off-by: Danno Ferrin <[email protected]>
Signed-off-by: Danno Ferrin <[email protected]>
|
||
### Static Jump Validations | ||
|
||
For every jump operation preceded by a `PUSHn` instruction the value of the data pushed on to the |
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 about jump operations not preceded by a PUSHn
instruction?
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 about jump operations not preceded by a
PUSHn
instruction?
In that case we have to use data flow analysis to determine if the argument to JUMP
is a constant specified by a PUSHn
. I assume validation is a one time thing (before deploying the contract?) so building a data flow graph does not seem to be too expensive.
BTW, compilers only generate PUSH2
in this case because ... there is a size limit to the smart 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.
I would say we should not use this relatively weak heuristics as a part of new validation rules. It is better to implement subroutines (which eliminates the most common source of dynamic jumps - which is return from the subroutine), and then the actual static jumps, and disable the dynamic jumps all together. Then we can remove JUMPDEST
.
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.
Using subroutines is way better for validating the contract, but it is not infeasible to validate a contract without static jumps. Symbolic execution is still able to figure out which jump is dynamic and hence report it.
In one attack we've seen , the attacker expanded memory to a megabyte, then did Anyway, once this failed (which it did pretty cheaply), the caller could modify one byte in memory (cheap), and do it again. Since mem was already expanded, this is a pretty cheap operation. Now, we can cheat a bit, and defer some parts of the jumpdest analysis -- we need only store a bitmap, one eight a size of the code, where we remember if a portion is a With this PR, for every such attack-create, we need to do more work up-front. The entire validation needs to be done, because if it is failing, then we need to immediately abort execution. So that means that for that one megabyte, where previously we could just iterate over bytes (and flip some bits at pushn segments [1] ), we need to
So I think this brings on a more than 2x amplification to that attack described previously. [1] To expand a bit: it is computationally expensive to flip every bit in a jumpdest bitmap for a one-megabyte contract. Therefore, it's better to flip bits only at pushn segments: whenever we have a |
Also, for reference: the jumpdest analysis in geth is this tiny piece: https://github.com/ethereum/go-ethereum/blob/master/core/vm/analysis.go#L38 . It would be interesting to see the corresponding implementation that would be required for this proposal. |
Here's my conceptual code: shemnon/besu@1c7f3e6#diff-24b154f8350072f858088bcc018d1888R41 Was this attack contract done prior to Spurious Dragon? Contract sizes were cut down to 24K then, so the contract length checking would mark the contract as invalid on mainnet fairly quickly. That's check is done in a different class than what I linked but is present in besu. It's still an amplification attack but the size is significantly limited on mainnet and up to date testnets. For this logic it's one pass, storing into two bitsets, so 8k extra memory for mainnet and only one contract walk. It is followed up with a couple of bit checks but that should all be in cache for modern processors. As far as the cost for flipping jumpdest bits remember we are also checking opcode validity at each point, so there would be more processing at every operation already. For jump operations not preceded by a PUSHn no validation can reasonably performed, especially since the stack value can be set by a called parameter. This would allow the VM to defer jumpdest validation until just prior to the first encounter of a dynamic jump operation. Or a client could note that there was a dynamic jump operation and store the jumpdest map somewhere useful. Future versions of the gas table could charge more for dynamic jumps to incentivize static jump usage. |
… `0xefevm` as header bytesSigned-off-by: Danno Ferrin <[email protected]> Signed-off-by: Danno Ferrin <[email protected]>
contract is treated exactly as through it had been deployed to an account with version `0`. For | ||
these contracts none of the other subsections in this EIP apply. | ||
|
||
When deploying a contract if a contract starts with `0xef` and has a length 4 or later the first |
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.
later => larger, greater
|
||
## Motivation | ||
|
||
There are two major motivations: first the need to make the EVM easier to evolve, and the second is |
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.
Two major and one minor?
rules described in the Version Header, `BEGINDATA`, Invalid Opcode Validation, and Static Jump | ||
Validations sections. | ||
|
||
These EIP sections applies to contracts stored or in the process of being stored in in accounts with |
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 in accounts => in accounts
There has been no activity on this pull request for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review. |
This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment. |
A set of contract markers and validation rules relating to those markers is proposed. These
validation rules enable forwards compatible evolution of EVM contracts and provide some assurances
to Ethereum clients allowing them to disable some runtime verification steps by moving these
validations to the deployment phase.