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.

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

internal: add eth_batchCall method #25743

Closed
wants to merge 2 commits into from
Closed

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Sep 12, 2022

Adds eth_batchCall as per #24089. The main characteristics are:

  • You give a block to be used as base state
  • A set of state overrides are applied once before any call is executed
  • On top of this modified state we start executing the calls one after the other. They build on eachother's end state
  • The block metadata (number, basefee, etc.) can be overriden for each individual call

Copy link

@Debu976116 Debu976116 left a comment

Choose a reason for hiding this comment

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

okkk

@s1na s1na changed the title eth/tracers: add trace_batchCall method eth,internal: add eth_batchCall and trace_batchCall methods Sep 21, 2022
@s1na s1na changed the title eth,internal: add eth_batchCall and trace_batchCall methods eth,internal: add eth_batchCall and debug_traceBatchCall methods Sep 21, 2022
Comment on lines +1081 to +1105
// CallResult is the result of one call.
type CallResult struct {
Return hexutil.Bytes
Error error
}
Copy link

Choose a reason for hiding this comment

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

I really like that this is finally being implemented in the core protocol since it makes it very easy to do a number of things.

It'd be really great if this also includes gas consumed by the particular call. It'd help to set more appropriate gas limits when someone needs to sign the next transactions while the initial state-changing transactions are pending/not broadcasted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's useful we can easily return the gas used here. But can you please elaborate on your use-case? I didn't quite understand.

Copy link

@zemse zemse Sep 23, 2022

Choose a reason for hiding this comment

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

An example of simple use-case: erc20 approve + defi interaction.

Problem: eth_estimateGas done before the erc20 approve getting confirmed would usually revert. Hence the normal flow of UX is to send approve tx and wait for it to confirm then do the defi interaction tx. This is well known to be not good UX because of increased user waiting times. Using huge input gas limit is not the good solution because wallets show max eth fees (gas price * gas limit) which looks costly and some users might not have enough eth.

Solution: if eth_batchCall includes gasUsed, then it can be used instead of eth_estimateGas where the erc20 approve tx is mentioned as first call and then the defi interaction as second call and it's gasUsed can be used to very accurately estimate gas even before prervious transaction is confirmed. The improved UX for this becomes: click on button in dapp once, hit confirm on metamask/wallet twice, check back in like few mins if both txs are confirmed. Similarly, if the user had to do a lot of steps like approve + deposit + stake + what not, a dapp could use eth_batchCall to simulate the UI state after a user interaction and create list of txs to submit and get them signed at once and accurately estimate the gas limit (similar to github PR reviews where we can add lot of comments while scrolling at our convenience and it gets submited all at once). This saves a lot of user's waiting time, and hence has the potential to improve the UX considerably.

TLDR including gasUsed basically enables estimating gas on a state updated after a series of calls. I hope the usecase makes sense.

Edit: I just came across a project (created by Uniswap engineer) that exposes an endpoint for batch estimateGas using mainnet fork (link), use-case mentioned in their README beginning is exactly what I am trying to explain above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok now I understand the use-case, thanks for extensive description. I think adding gasUsed to the result is not a good solution. If you look, logic of eth_estimateGas is more complicated than simply doing a eth_call and reporting the gasUsed. This AFAIK is because the gaslimit provided to the tx can change the flow of the tx itself (GAS opcode).

But the use-case is valid IMO and warrants a eth_batchEstimateGas or something of the sort.

Copy link
Member

Choose a reason for hiding this comment

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

@s1na I had a implementation for batchEstimateGas way way back ago #21268

But I think your approach is much better, would be appreciate you can also take over this with the same design(e.g. state overrides, block overrides, etc)

Copy link

Choose a reason for hiding this comment

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

Yeah, it makes sense. So if gas is not specified in a call object it'd assume a large value, in order to ensure complex state-changing calls follow a successful execution path if there exists any, wouldn't users need to use eth_batchEstimateGas (or something like that) for setting the gas field in the calls prior to using eth_batchCall?

Choose a reason for hiding this comment

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

An example of simple use-case: erc20 approve + defi interaction.

Problem: eth_estimateGas done before the erc20 approve getting confirmed would usually revert. Hence the normal flow of UX is to send approve tx and wait for it to confirm then do the defi interaction tx. This is well known to be not good UX because of increased user waiting times. Using huge input gas limit is not the good solution because wallets show max eth fees (gas price * gas limit) which looks costly and some users might not have enough eth.

Solution: if eth_batchCall includes gasUsed, then it can be used instead of eth_estimateGas where the erc20 approve tx is mentioned as first call and then the defi interaction as second call and it's gasUsed can be used to very accurately estimate gas even before prervious transaction is confirmed. The improved UX for this becomes: click on button in dapp once, hit confirm on metamask/wallet twice, check back in like few mins if both txs are confirmed. Similarly, if the user had to do a lot of steps like approve + deposit + stake + what not, a dapp could use eth_batchCall to simulate the UI state after a user interaction and create list of txs to submit and get them signed at once and accurately estimate the gas limit (similar to github PR reviews where we can add lot of comments while scrolling at our convenience and it gets submited all at once). This saves a lot of user's waiting time, and hence has the potential to improve the UX considerably.

TLDR including gasUsed basically enables estimating gas on a state updated after a series of calls. I hope the usecase makes sense.

Edit: I just came across a project (created by Uniswap engineer) that exposes an endpoint for batch estimateGas using mainnet fork (link), use-case mentioned in their README beginning is exactly what I am trying to explain above.

gas used through a wrapper contract is not accurate with Multicall due to EIP-2929, so should be avoided FYI (this is why Uniswap made this endpoint i think)

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

I think it might be better to split these up in different PRs so that we can go through the execution-apis and standardize the eth_batchCall before adding it to geth. It would be nice to get wallet teams and other clients to weigh in.

internal/ethapi/api.go Outdated Show resolved Hide resolved
internal/ethapi/api.go Outdated Show resolved Hide resolved
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM. Since this is a new method, I'm not too worried about potential flaws, so I wouldn't mind merging it and letting people try it out.

@s1na s1na requested a review from gballet as a code owner September 26, 2022 18:44
@s1na s1na changed the title eth,internal: add eth_batchCall and debug_traceBatchCall methods internal: add eth_batchCall method Sep 27, 2022
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
)
for _, call := range config.Calls {
blockContext := core.NewEVMBlockContext(header, NewChainContext(ctx, s.b), nil)
if call.BlockOverrides != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand why we have call-level block overrides. In practice these calls usually have the same block context if they want to be put in a single block by intention?

Copy link
Member

Choose a reason for hiding this comment

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

Because users can simulate the case that transactions are included in different blocks? If so I think this design makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Micah also asked a similar question here: ethereum/execution-apis#312 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

On one hand, it makes sense. For example, if you want to experiment with a time-locked contract. First you create it, then two years pass, now you want to interact again.
It opens up a lot of potential uses which does not fit inside a single block.

However, it might also be a footgun. If you want to simulate a sequence where

  1. Block n: A contract X is selfdestructed,
  2. Block n+1, contrat X is resurrected.

The two steps can never happen in one block. The question is: what happens in the batch-call? Is it possible to make the two calls execute correctly, or will it be some form of "time-shifted single block", where you can override the time and number, but state-processing-wise it's still the same block.... ?

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 am concerned that not all clients will have an easy time implementing this as currently specified. I suspect all clients could have two distinct blocks that they execute against some existing block's post-state, but not all clients may be able to simulate a series of transcations against a cohesive state when the transaction's don't share block fields.

Would be great to get other client feedback on this to verify, but without any feedback I would assume the worst that this will be "hard" to implement in some clients. Having writing some Nethermind plugins, my gut suggests that this would be challenging to do with Nethermind for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about this some more, and I think it would be better to just allow the user to multicall and have multiple blocks, each with different transactions. The model may look something like:

[ { block_n_details, block_n_transactions }, { block_m_details, block_m_transactions }, ... ]

We would still require normal rules to be respected between blocks (like block numbers are incrementing, timestamp in future blocks must be higher number than previous blocks, 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.

or will it be some form of "time-shifted single block", where you can override the time and number, but state-processing-wise it's still the same block.... ?

This is a good point. As the implementation stands there are differences to how a sequence of blocks are executed (one being coinbase fee). As I mentioned here ethereum/execution-apis#312 (comment) I would like to proceed with "only" the single-block-multi-call variant. This would already be a big improvement for users and I would prefer not to delay that for something more complicated at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would already be a big improvement for users and I would prefer not to delay that for something more complicated at the moment.

I think it is only notably more complicated if you try to do different overrides of transaction details within a single block. My proposal is to actually have multiple blocks, each which would follow most consensus rules (like timestamps must increase, block number must increase, etc.). I believe the complexity that Martin is referring to is specifically related to how the original proposal was designed where you have one "block" but each transaction had different block properties reflected in it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about this some more, ...
We would still require normal rules to be respected between blocks (like block numbers are incrementing, timestamp in future blocks must be higher number than previous blocks, etc.

I've also been thinking about this some more - and I reached a different conclusion :) In fact, kind of the opposite. I was thinking that we could make this call very much up to the caller. We would not do any form of sanity checks. If the user wants to do block 1, 500, 498, 1M, 3 in sequence, while letting timestamp go backwards, then fine. It's up to the caller to use this thing "correctly".

In that sense, I don't see any need to enforce "separate blocks". (To be concrete, I think that only means shipping the fees to the coinbase, so that is not a biggie really. )

I do foresee a couple of problems that maybe should be agreed with the other clients:

  1. When EVM invokes the BLOCKHASH(number) opcode. How should we 'resolve' blockhash when the block number is overridden. Possible semantics
    • Always return emptyhash
    • Always return keccak256(num)
    • Return as if it were executed on the current block, ignoring overrides.

Currently, geth does the third option, since the blockcontext.GetHash function is set before any block overrides:

	vmctx := core.NewEVMBlockContext(block.Header(), api.chainContext(ctx), nil)
	// Apply the customization rules if required.
	if config != nil {
		if err := config.StateOverrides.Apply(statedb); err != nil {
			return nil, err
		}
		config.BlockOverrides.Apply(&vmctx)
	}

.... I think there was one more thing I meant to write, but I've forgotten now....

Copy link
Contributor

Choose a reason for hiding this comment

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

I've also been thinking about this some more - and I reached a different conclusion :) In fact, kind of the opposite. I was thinking that we could make this call very much up to the caller. We would not do any form of sanity checks. If the user wants to do block 1, 500, 498, 1M, 3 in sequence, while letting timestamp go backwards, then fine. It's up to the caller to use this thing "correctly".

My concern with this strategy is that some clients (or possibly future clients) may be architected such that disabling basic validation checks like "block number go up" in an area that is harder to override during calls. This is, of course, speculation on my part but it aligns with my general preference toward keeping the multicall as close to actual block building as possible. I also can't think of any good use cases where having block number or time go backwards would help someone, so it feels like unnecessary leniency.

When EVM invokes the BLOCKHASH(number) opcode. How should we 'resolve' blockhash when the block number is overridden. Possible semantics

* Always return emptyhash

* Always return `keccak256(num)`

* Return as if it were executed on the current block, ignoring overrides.

I think there is a fourth option to include it in the potential overrides, so the caller would say "when you execute this block and BLOCKHASH(n) is called, return this value". The caller could provide an array of n to blockhash values (they presumably know what set they need). We could then fallback to one of the "reasonable defaults" that you have listed.

Value: (*hexutil.Big)(big.NewInt(1000)),
},
expectErr: core.ErrInsufficientFunds,
want: 21000,
Copy link
Member

Choose a reason for hiding this comment

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

nitpick, we don't need this want field since error is already expected.

randomAccounts[0].addr: OverrideAccount{Balance: newRPCBalance(big.NewInt(1000))},
},
Calls: []BatchCallArgs{{
TransactionArgs: TransactionArgs{
Copy link
Member

Choose a reason for hiding this comment

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

randomAccounts[0].addr and randomAccounts[1].addr are all have funds to transfer(allocated in genesis), you should use a new address here.

//
// Note, this function doesn't make any changes in the state/blockchain and is
// useful to execute and retrieve values.
func (s *BlockChainAPI) BatchCall(ctx context.Context, config BatchCallConfig) ([]CallResult, error) {
Copy link
Member

Choose a reason for hiding this comment

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

It feels weird that we put all arguments in a config object. I can get the point that it's way more flexible and can be easily extended in the future.

Maybe we can put Block rpc.BlockNumberOrHash and Calls []BatchCallArgs as standalone parameters and with a config object for specifying the additional configurations(state overrides, etc)? Just a braindump though.

@zhiqiangxu
Copy link
Contributor

zhiqiangxu commented Sep 30, 2022

Is there any advantage of using eth_batchCall over just using eth_call and doing batch call inside a temporary contract?

Like this: https://github.com/zhiqiangxu/multicall/blob/master/multicall_test.go#L38

@zemse
Copy link

zemse commented Sep 30, 2022

Is there any advantage of using eth_batchCall over just using eth_call and doing batch call inside a temporary contract?

If I'm understanding correctly, you mean makerdao/multicall kinda way then we can't set msg.sender (since it would always be the multicall contract). For view methods / non-state changing calls, I don't think this method adds much value, other than being a single request like multicall contract. Also it's serial execution, with batched eth_calls you may get parallel execution for non-state changing calls.

@senthilsamrat
Copy link

does each eth_call inside a batch_Call, executes sequentially keeping the previous calls end state ? this will allow to find the result at the end of n number of sequencial calls

@MicahZoltu
Copy link
Contributor

does each eth_call inside a batch_Call, executes sequentially keeping the previous calls end state ? this will allow to find the result at the end of n number of sequencial calls

That is the idea, yes.

@senthilsamrat
Copy link

does each eth_call inside a batch_Call, executes sequentially keeping the previous calls end state ? this will allow to find the result at the end of n number of sequencial calls

That is the idea, yes.

Thats awesome.. I would love to use this api. Can someone tell How can I try this out before the merge with master?

@holiman
Copy link
Contributor

holiman commented Oct 24, 2022

There are still some unresolved questions. For me, this one: #25743 (comment)

Copy link

@Debu976116 Debu976116 left a comment

Choose a reason for hiding this comment

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

Change

@s1na
Copy link
Contributor Author

s1na commented Jul 13, 2023

Closing in favor of #27720.

@s1na s1na closed this Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants