Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add eth_batchCall #312

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Add eth_batchCall #312

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cb23322
Add eth_batchCall schema
s1na Sep 26, 2022
d7bb95f
add block override
s1na Sep 26, 2022
f8c2444
fix spelling
s1na Sep 26, 2022
9bf4970
rename to multicall
s1na Oct 27, 2022
7aba9c3
allow one block overrides per multicall
s1na Oct 27, 2022
9861fd8
fix err desc
s1na Oct 27, 2022
12e530f
Revert "fix err desc"
s1na Oct 27, 2022
57697c3
rename difficulty to prevrandao
s1na Oct 28, 2022
4e680a6
add prevRandao to wordlist
s1na Oct 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/eth/execute.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,14 @@
gasUsed:
title: Gas used
$ref: '#/components/schemas/uint'
- name: eth_multicall
summary: Executes a sequence of message calls building on eachother's state immediately without creating transactions on the block chain.
params:
- name: Arguments
required: true
schema:
$ref: '#/components/schemas/MulticallArgs'
result:
name: Result of calls
schema:
$ref: '#/components/schemas/CallResults'
112 changes: 112 additions & 0 deletions src/schemas/execute.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
MulticallArgs:
title: Arguments for multi call
type: object
required:
- block
- calls
properties:
block:
title: Block tag
$ref: '#/components/schemas/BlockNumberOrTag'
stateOverrides:
title: State overrides
$ref: '#/components/schemas/StateOverrides'
Comment on lines +11 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels out of place here. What is the reasoning for having this as a parameter? It also greatly complicates implementation in some clients I believe compared to not including it so I would like to see a compelling reason for its inclusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a greatly used feature of eth_call. I agree it is less necessary in multicall since you can build up the state in previous calls. However not all state mutations are possible (e.g. simply replacing a contract code).

I agree this is extra work for client devs. However I would like to hear from them if this is indeed a complication. It seems to me all have a way to build a temporary state for serving eth_call. This goes a step further to modify parts of that state.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the state override feature is probably useful enough to warrant the complexity. I can imagine a security researcher breaking a problem down into two parts. If this value ever can become X, there is a vulnerability to report. They could research that aspect first (as it might be simpler) then research "can this value ever become X".

Could also fake signers of a multi-sig that use store approvals (as many do).

blockOverrides:
title: Overrides for block metadata
$ref: '#/components/schemas/BlockOverrides'
calls:
title: List of calls
$ref: '#/components/schemas/Calls'
Calls:
title: List of message calls
type: array
items:
$ref: '#/components/schemas/GenericTransaction'
StateOverrides:
title: Accounts in state to be overridden
type: object
additionalProperties:
$ref: '#/components/schemas/AccountOverride'
AccountOverride:
title: Details of an account to be overridden
type: object
oneOf:
- $ref: '#/components/schemas/AccountOverrideState'
- $ref: '#/components/schemas/AccountOverrideStateDiff'
AccountOverrideState:
title: Account override with whole storage replacement
properties:
nonce:
title: Nonce
$ref: '#/components/schemas/uint64'
balance:
title: Balance
$ref: '#/components/schemas/uint256'
code:
title: Code
$ref: '#/components/schemas/bytes'
state:
title: Storage
$ref: '#/components/schemas/AccountStorage'
AccountOverrideStateDiff:
title: Account override with partial storage modification
properties:
nonce:
title: Nonce
$ref: '#/components/schemas/uint64'
balance:
title: Balance
$ref: '#/components/schemas/uint256'
code:
title: Code
$ref: '#/components/schemas/bytes'
stateDiff:
title: Storage difference
$ref: '#/components/schemas/AccountStorage'
AccountStorage:
title: Storage slots for an account
type: object
additionalProperties:
- $ref: '#/components/schemas/hash32'
BlockOverrides:
title: Context fields related to the block being executed
type: object
properties:
number:
title: Number
$ref: '#/components/schemas/uint256'
difficulty:
title: Difficulty
$ref: '#/components/schemas/uint256'
s1na marked this conversation as resolved.
Show resolved Hide resolved
time:
title: Time
$ref: '#/components/schemas/uint256'
gasLimit:
title: Gas limit
$ref: '#/components/schemas/uint64'
coinbase:
title: Coinbase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
title: Coinbase
title: FeeRecipient

I believe fee recipient is the nomenclature we are using now (as of The Merge) for the person who received the transaction fees in the block. This is separate from proposer, which I don't think the execute layer has access to.

$ref: '#/components/schemas/address'
random:
title: Random
$ref: '#/components/schemas/hash32'
baseFee:
title: Base fee
$ref: '#/components/schemas/uint256'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a mechanism for indicating optionality? I feel like all of these should be optional and have reasonable defaults provided by the execution client.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also feel like these should be constrained to values that otherwise pass validation so clients don't have to write code that skips over any more validation than necessary (skipping block signature for example is necessary). For example, block number must be monotonically increasing, time must be strictly greater than previous block, gasLimit cannot change more than 1/1024 from previous block, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a mechanism for indicating optionality? I feel like all of these should be optional and have reasonable defaults provided by the execution client.

The default value will be taken from the block the calls will be executed on top of.

I also feel like these should be constrained to values that otherwise pass validation so clients don't have to...

Is this still a concern now that there's one set of block overrides for the whole batch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be clearly indicated in this specification, perhaps in a description field? Some of them cannot be defaulted to the parent block (like number and timestamp), so I think we should have a description for each indicating where it gets its default from. Also, I would really like to see it indicated that these are optional somehow, ideally in the schema itself, but alternatively in a description field or comment of some kind.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still a concern now that there's one set of block overrides for the whole batch?

I think these are still relevant. If you are building off of block n, then you cannot have a number that is less than or equal to parent.number. Similarly, you cannot have a timestamp that is less than or equal to the parent.timestamp. Similar restriction on gas limit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: On Gas limit there may be value in expressly allowing changes that are against consensus rules so that people can do series of calls that are more than the current gas limit.

CallResults:
title: Results of multi call
type: array
items:
$ref: '#/components/schemas/CallResult'
CallResult:
title: Result of a call
type: object
required:
- return
properties:
return:
title: Return data
$ref: '#/components/schemas/bytes'
error:
title: Execution error
type: string
Comment on lines +110 to +112
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does string here mean? I believe revert reason is just a byte array. While Solidity has some helpers like require(..., message) that make it easy to encode a string in those bytes, there are some contracts out there that return error codes as numbers, and it could even contain data arrays.

I think this should be bytes, and it is left up to the recipient to decode that as appropriate for the contract in question.

Copy link
Contributor Author

@s1na s1na Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify this is not directly evm revert error. It encompasses other errors such as OOG. If call reverts the error will be "execution reverted" and the return data field will contain the bytes that you mentioned.

Happy to change the description of the field if it is confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to indicate that in the case of an EVM revert, return will contain the revert return bytes.

Is there a way in this schema to express that error is optional (may not be defined in the resulting object)?

Comment on lines +101 to +112
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result for each transaction should include logs and gas spent as well. I suspect that all clients are generating this information anyway, and throwing it away is a waste when we could just return it all. In many cases, this will also make it so the caller doesn't need to do multiple round trips (e.g., once for gas estimate and again for speculative execution and again to read a deterministic event like contract creation details).

If the call is a contract creation (to: null), it should also return the address of the created contract for the same reason.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include logs and gas spent as well

@s1na mentioned an argument against including gas spent in the return interface (here). The intention I mentioned was to use the gas spent as is for gas limits, which I agree would fail when tx. However, including gas spent in the response could still be useful if utilized with care (passing a factor of more gas for e.g. to account for 63/64 gas passed to child msg calls). And in the documentation, a note can be mentioned that gas spent is not accurate to be used as gas limit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't think it is worth trying to optimize the edge case where someone does if (GAS) ... because that is very rare and when it is used, the dapp almost always explicitly sets the GAS required anyway so the returned gas used won't matter.

I think the value add of returning gas used far outweighs the downsides of it being wrong in a handful of esoteric scenarios. I do recommend that we standardize on the amount of gas provided to each call in eth_multicall though. Perhaps block.gas_limit - gas_used_in_previous_transactions_in_multicall? Essentially, assume that the whole set of transactions is intended to fit inside a block (this also provides a sanity check on total gas used) and they "burn through" the block's gas as they are executed.

This would prevent people from using this to do mega gas operations, but I think we can address that by simply having eth_multicall accept a gas_limit parameter for the block and each transaction so this isn't a hard limiter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think gasUsed should not be used as an estimate. But I can sympathise with adding this info to the call. It comes at no overhead. Right now all this can be fetched in one call through geth's tracing API (via the callTracer), but I agree it should be part of eth_. I'm happy to add these.

Re gas_limit there is a need for a block gas limit set in the client itself. Because providers like Infura will want to cap the runtime of API methods. Geth has both a gas limit and a deadline in seconds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting an upper bound on the gas able to be used by an eth_call or eth_multicall feels like it is out of scope of this schema. On my local node, I should be able to do calls that use trillions of gas, and the fact that a centralized service (which we should not be catering our designs toward) needs rate limiting should not negatively impact a self-hosted operator from doing whatever they want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify: what I meant was there is a CLI flag anyway that will set a limit for all eth_calls and eth_multicalls. On your local machine you're free to set it to trillions. Question is whether a per-invocation limit is also useful.

This would prevent people from using this to do mega gas operations,

Adding a gasLimit parameter won't solve this for public facing nodes. They will simply set a high gas limit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify: what I meant was there is a CLI flag anyway that will set a limit for all eth_calls and eth_multicalls. On your local machine you're free to set it to trillions. Question is whether a per-invocation limit is also useful.

Such a setting feels like it is out of scope of this standard? I might be able to be convinced that it is relevant, but rate limiting features and DoS protection generally aren't standardized.

What we need to standardize is "how much gas is given when the parameter is missing" and picking something "reasonable" (e.g., not 2^256, and not 30,000) as a default feels like the right way to go to me. If a particular client has limits that are lower than that (such as from configuration) then they should return an appropriate rate limiting error and require the user to explicitly set the gas limits in their requests I think if the user exceeds those limits.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the if (GAS) case is not really worth considering but the 63/64 and the refunds are.

However, there's really two uses for gas that should be handled:
1.) what you need to add to transaction for it to succeed (eth_estimateGas/eth_batchEstimateGas)
2.) What you actually pay for, some way to determine what my EOA is actually going to be charged for the transaction. It feels most natural for that to occur in the eth_batchCall . If there is a back-run, it is very important to the sender how much gas is going to actually be charged as mev profit can often be extremely small and the difference between the actual gas used and the gas estimation is the difference between profit and loss.