Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
add
eth_simulateV1
#484base: main
Are you sure you want to change the base?
add
eth_simulateV1
#484Changes from 4 commits
31fcbe5
8b020ce
aaab3f8
8ab6539
61fb231
45313dd
d9411e0
06d27bd
b4ce098
0043e12
3095c08
c7ec336
c20052c
7aee5b3
8996847
e76cb57
dde3124
db721c4
aef37eb
85ad1a4
21c6dfa
e27cf9a
b597a7f
1ccfd37
f297b4f
e3332ea
e913466
8141886
2ee9fc2
42d1249
55f657c
0256a4b
de3e3f5
d7b4616
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
These default values will crash, since baseFeePerGas is always 7 or higher. These default values will be rejected by blockchain spec, since tx is not willing to pay base fee. Note that the default block
baseFeePerGas
is calculated according to spec, so this will be 7 or higher always.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.
Suggestion: change
maxFeePerGas
andgasPrice
(in case of tx type 0/1) tobaseFeePerGas
of the block and keepmaxPriorityFeePerGas
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.
yeah, this will crash on validation mode. On non-validation mode, we will allow you to make transactions that have zero basefee (similar to eth_call)
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.
Slight nit: for completeness,
withdrawals
should also be able to be overridden (to simulate withdrawals).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'd suggest using
0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE
as the sending contract, since that's become a de facto approach for representing ETH as a token address within contracts.Here's a simple sourcegraph search showing usage: https://sourcegraph.com/search?q=0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE&groupBy=repo
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 suspect if you were to do a similar search of
0x000...
you would get even more results. "A lot of people use X" is quite different from "Most people use X".If we ignore any past precedence, I feel like
0x000...
makes it a tad more explicit that this is special, and not some precompile or something. For parameters, 0xEee... makes a bit of sense because0
is the default value for a lot of stuff (e.g., null, missing, etc.) and thus often passed on accident, so using nonzero helps protect against a class of bugs. In the case of event sourcing, 0x000... isn't provided by user/developer so they cannot get it wrong.That being said, I don't have a strong argument against 0xEeee..., only that I think 0x000... is marginally better and I suspect more widely used.
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 a search for the zero address would yield more results, but only because the zero address is used for a much wider range of cases, whereas
0xEee...
seems to only be used to represent ETH. This is anecdotal and it's hard to know concretely, but in my experience0xEee...
is more common for representing ETH than the zero address.Personally, if I didn't know about the special ETH logs, seeing
0xEee...
would make the log's meaning more intuitive and apparent than seeing0x000...
.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.
Sorry, I meant if you could somehow limit the search of
0x0000...
to only places where it was being used as a representation of ETH I suspect it would be more/bigger. That being said, perhaps the searchability of0xEeee...
is a significant selling point.For what it is worth, this feature is disabled by default and you need to set a flag to turn it on. I'm not sure that is a meaningful argument though.
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 @mds1 that
0xEee..
seems more appropriate for Eth transfer logsThere 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 believe the spec has since been updated to use
0xEee....
.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 notes still mentions
0x0
here: https://github.com/ethereum/execution-apis/pull/484/files#diff-1e4727bbfb75c5cc85054abbf972a1cd472e9e512a9d8bbc8152210a0062182bR116Aside: It still feels "unclean" to me using a specific non-reserved address, but in researching this I was surprised to learn that EIP-1352 was never officially adopted.
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.
0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE
is not correct if you're using EIP-1191, the correct chainId encoded address would be0xeeeEEEeEEeeeEeEeEEEeEeeeeEEEEeEEeEeeeeeE
having it as
0x00...
would be agnostic to chainId encoding.Edit: this is a purely aesthetic choice, though I would find it helpful
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 and others have argued against EIP-1191 in the discussion for it: ethereum/EIPs#1121
My position is still largely the same. It is backward incompatible with EIP-55 checksumming, so I generally don't recommend apps checksum to 1191 until long after almost all tooling supports reading 1191 checksums.
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 am not entirely sure what this would remove/add. If I turn this on, in what situation would this be useful, and what would change if I would have kept it off? 🤔
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.
For instance, if validation is set to off, are tx gas limits not checked? (So I can include a tx with gas limit higher than the block gas limit?).
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.
Never mind, it is in
execute.yaml
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 bit ill defined (as discussed in todays call). Because Nethermind and Geth behave bit differently with
eth_call
and we have been trying to figure out the differences so we could document them. The intention that is that when validation is enabled, the clients would behave exactly the same, while not fixing the issue that with non-validation, the validation rules might differ a bit.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 I think this spec should explicitly state what should be not be validated if the validation is off. For instance, currently it only seems that you can override the sender (so you can create unsigned txs). But, it also seems that it is possible to set base fee to zero (so do not validate this as well). It should be clearly defined what should be validated and what not. (And this is non-trivial since there is a huge ruleset of what should be validated in transactions/blocks)
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.
For validation mode, I'm fine with base fee not being allowed to go below 7.
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 don't think it makes sense that much to validate base fee in validation mode if the user specifically sets the base fee to something. Otherwise we could remove all overrides for validation mode, as none of them can happen on mainnet.
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.
Please no! In fact I want to argue that we should remove
validation
flag and default to relax-mode. My argument is that this is easy to add in later since we have designed the API param to be an object. It will be a backwards-compatible change. I'd personally like to hear from some people to say they need this first.That said, we should still define exactly what is not validated in relax-mode. So I think that convo is important to have. And I will share here exactly what geth is validating:
For block validation, only these fields are validated:
As for tx validation, these are NOT validated:
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'd like to add that these relaxations are all cause for a transaction not to go through. I.e. if I have a call that works in the relax-mode, sign it and submit it to a node, it will not be gossiped to the network at all.
I'd argue if there is any danger to a user it's from all of the flexibility we're providing through state and precompile overrides. E.g. they assume in the simulation to be in a certain condition by using overrides, and submit the transaction when that is not met. That tx will be mined and will revert (most probably).
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.
Our team would like it because it allows us to verify that the transaction is properly constructed and will execute against mainnet without error. In some cases we ship a set of transactions as a "bundle" to searchers, and they need to all execute properly in sequence (including things like balance checks), and in other cases we ship a single transaction to a signer and we want a high degree of confidence that it will execute on-chain just like it did during simulation.