support gas estimates with block identifier

[wolovim]

What was wrong?

Re: ethereum/web3.py#1639, py-evm supports gas estimation with block identifiers, but eth-tester didn't yet pass those values through.

How was it fixed?

estimate_gas now accepts and processes a block_number.

Feedback appreciated @carver (and anyone else interested); I processed the minimum surface area of this package required to get this far and get passing tests on the Web3 side.

To-Do:

• Add entry to the CHANGELOG

Cute Animal Picture

[carver]

Nice that the block number was already supported by _get_normalized_and_unsigned_evm_transaction. Makes this a short and sweet PR.

Only other thing to add is a test. See this for inspiration:

https://github.com/ethereum/eth-tester/blob/master/eth_tester/utils/backend_testing.py#L670-L687

The best kind of test would be one that checks that the gas cost is actually different depending on the block supplied. One way to do it would be to estimate the gas cost of running the transaction before the contract was deployed.

[carver]

Yeah, looking at the test, I'm realizing that the assertion is pretty weak, because the estimates are allowed to vary somewhat randomly.

I guess I should have said the best kind of test would be to write a contract that burns gas in a way that's easily detectable by block number, like burning (100k * block_number) gas. But given how much other low-hanging fruit there is for improvement in other places & projects, I'm okay calling this good enough and moving on. 👍 if you want to add an issue to do that ^. (it's right on the border of a Good First Issue: great that nothing else will block on its progress, not good that it will surely require a lot of back and forth as a new test contract is added)

[pipermerriam]

Bike-shedding over the the way to test:

Contract with a toggle-able flag which just returns immediately or burns a bunch of gas in a loop. Then you can set it and test on either side of the block number where the flag was toggled. Agreed on this being lower priority for testing. Like @carver said, probably better to document the testing approach in a "good first issue".

[wolovim]

👍 new issue coming following the merge.