WIP: No way to specify block number for eth_estimateGas

[ankitchiplunkar]

What was wrong?

No way to specify block number for eth_estimateGas

Related to Issue # #1011

How was it fixed?

Added block_identifier as an input parameter in the estimateGas method(similar to web3.eth.call), this opens a can of worms because:

1. The estimate_gas method in the eth_tester library takes only 2 inputs while we now have an extra input (block_identifier). To enable the passing of all tests we need to also change the https://github.com/ethereum/eth-tester library

E       TypeError: estimate_gas() takes 2 positional arguments but 3 were given

2. A geth node does not accept block_identifier as a parameter and throws an error, this has been documented before internal/ethapi: eth_estimateGas should take block number go-ethereum#2586

Parity

>>> w3 = Web3(Web3.HTTPProvider("https://kovan.infura.io/"))
>>> w3.eth.estimateGas(transaction)
method: eth_estimateGas, parameter: [{'data': '0xacabb9ed000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000a00000000000000000000000000000000000000000000000000000000000000039746869732d69732d7468652d66697273742d737472696e672d77686963682d657863656564732d33322d62797465732d696e2d6c656e67746800000000000000000000000000000000000000000000000000000000000000000000000000003a746869732d69732d7468652d7365636f6e642d737472696e672d77686963682d657863656564732d33322d62797465732d696e2d6c656e677468000000000000', 'from': '0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf', 'to': '0xF2E246BB76DF876Cef8b38ae84130F4F55De395b'}, 'latest']
response: {'jsonrpc': '2.0', 'id': 0, 'result': 29912}
29912

Geth

>>> w3 = Web3(Web3.HTTPProvider("https://mainnet.infura.io/"))
>>> w3.eth.estimateGas(transaction)
method: eth_estimateGas, parameter: [{'data': '0xacabb9ed000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000a00000000000000000000000000000000000000000000000000000000000000039746869732d69732d7468652d66697273742d737472696e672d77686963682d657863656564732d33322d62797465732d696e2d6c656e67746800000000000000000000000000000000000000000000000000000000000000000000000000003a746869732d69732d7468652d7365636f6e642d737472696e672d77686963682d657863656564732d33322d62797465732d696e2d6c656e677468000000000000', 'from': '0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf', 'to': '0xF2E246BB76DF876Cef8b38ae84130F4F55De395b'}, 'latest']
response: {'jsonrpc': '2.0', 'id': 0, 'error': {'code': -32602, 'message': 'too many arguments, want at most 1'}}
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ankit/projects/web3.py/web3/eth.py", line 307, in estimateGas
    [transaction, block_identifier],
  File "/home/ankit/projects/web3.py/web3/manager.py", line 114, in request_blocking
    return response['result']
ValueError: {'code': -32602, 'message': 'too many arguments, want at most 1'}

Changing the two libraries would be a HUGE task.
How do you (@Arachnid , @carver) propose that we should proceed?

Cute Animal Picture

[carver]

Changing geth to accept the block identifier is surely out of scope. py-evm already supports estimating gas at a given header, so it shouldn't be too much work to add to eth-tester.

So I would just mark any geth tests as xfail and get it working otherwise working on eth-tester and parity.

[ankitchiplunkar]

One more point is that geth accepts only one input (transaction) while parity/ py-evm accept two (transaction & block_identifier)

This means we might have to either:

1. Check the node which is being used and modify the inputs accordingly, or
2. Push the method estimateGas in respective geth or parity modules.

Which solution do you prefer?

[carver]

Which solution do you prefer?

I prefer that the json-rpc get standardized in an EIP and that they all conform to it 😆

---

In the meantime...

Parity must work if only one argument is passed, I guess. So, if no block identifier is set in web3, instead of defaulting to latest, we could leave the argument out altogether in the RPC call. This has the weird effect that the number of arguments in the RPC call can be different, which I don't think happens anywhere else. But I'd be curious to see if we could make that work.

Then we don't have to do any node detection, and it would work in both geth and parity. Then if you try with an explicit block-identifier, geth will blow up (which we can't do anything about) and parity will work fine.

[ankitchiplunkar]

This is a wayyy better solution, because then we would not have to tag several geth tests as xfail.

Will work on implementing this and modifying the eth-tester now.

[vs77bb]

Hi @ankitchiplunkar any progress to show here? Hope you're doing well 🙂

[ankitchiplunkar]

@vs77bb yes was busy with EthBerlin last week, getting back to the task.

[ankitchiplunkar]

Have updated the estimateGas function as discussed above and added tests for parity and geth.
But, cannot implement middleware for the block_identifier, since we are supplying variable number of inputs to the final JSON-RPC call.

Implemented a block_identifier middleware by adding the following command to web3.middleware.pythonic.pythonic_middleware

'eth_estimateGas': combine_argument_formatters(
            transaction_param_formatter,
            block_number_formatter,
        ),

But got the following errors, when only one input was provided to method estimateGas

    @return_arg_type(2)
    def apply_formatter_at_index(formatter: Callable[..., Any],
                                 at_index: int,
                                 value: List[Any]) -> Generator[List[Any], None, None]:
        if at_index + 1 > len(value):
            raise IndexError(
                "Not enough values in iterable to apply formatter.  Got: {0}. "
>               "Need: {1}".format(len(value), at_index + 1)
            )
E           IndexError: Not enough values in iterable to apply formatter.  Got: 1. Need: 2

Have not implemented any middleware for the block_identifier as of now, mostly would need to implement a function (similar to combine_argument_formatters) that accepts variable number of inputs in library eth_utils. Do you think we should implement this middleware?

Still need to update:

1. eth_tester to take multiple inputs for estimateGas
2. Possible eth_utils to accept variable number of inputs in combine_arguement_formatters
3. Update docs for the estimateGas

[carver]

I think if we get creative, we can do this with existing formatters, something like:

@curry
def is_length(target_length, value):
  return len(value) == target_length

estimate_gas_without_block_id = apply_formatter_at_index(0, transaction_param_formatter)
estimate_gas_with_block_id = apply_formatters_to_sequence((
  transaction_param_formatter,
  block_number_formatter,
))

...

'eth_estimateGas': apply_one_of_formatters((
  (is_length(1), estimate_gas_without_block_id),
  (is_length(2), estimate_gas_with_block_id),
))

The key part is choosing which formatter to apply with apply_one_of_formatters. It uses the newer preferred apply_formatters_to_sequence.

[carver]

Can you do a rebase on master instead of merging master in? Makes it cleaner to read the PR.

[dylanjw]

I just merged #1033 which will make this rebase a bit complicated. @ankitchiplunkar Let me know if you want help.

[carver]

Can you amend the commit and write a different summary (first) line of the squashed commit? "Require compatible setuptools version " is confusing as a one-liner. Something like "Specify block number for w3.eth.estimateGas".

All the "body" (remaining lines) of the commit message can stay as whatever you want.

[ankitchiplunkar]

Thanks all, for helping with squashing and rebasing.

@carver I found that apply_formatters_to_sequence was not working but combine_argument_formatters worked fine.

The parity-http test is falling for some unknown reason.

Is this approach is acceptable and tests cover everything?

[voith]

Can you amend the commit and write a different summary (first) line of the squashed commit?

Big 👍 for pushing towards cleaner commits and for a better commit message. With clean commit history it becomes really easy to dig history and we can reliably use git bisect.

[voith]

This code can be reduced to:

params = [transaction]
if block_identifier:
    params.append(block_identifier)
return self.web3.manager.request_blocking(
    "eth_estimateGas",
    params
)

[carver]

Rather than mutating the params list, I would go with:

        if block_identifier is None:
            params = [transaction]
        else:
            params = [transaction, block_identifier]

        return self.web3.manager.request_blocking(
            "eth_estimateGas",
            params,
        )

Also, we don't need the second move to middleware TODO.

[carver]

I found that apply_formatters_to_sequence was not working but combine_argument_formatters worked fine.

Ok, not a big deal. Just a nice-to-have.

The parity-http test is falling for some unknown reason.

Yeah, surely not a problem with this PR. parity tests have been flaky.

Is this approach is acceptable and tests cover everything?

Yup, approach seems good, and tests cover enough for merge, I think. Just that one last inline comment to address, and should be good to go.

[carver]

Thanks for the doc update!