getBlockByHash and getBlockByNumber will return a block number if block is pending

[kclowes]

What was wrong?

As of geth 1.9.11, eth_getBlockByHash and eth_getBlockByNumber will be returning a block number if the block is pending, instead of null. Older versions of geth were returning a number, but as of 1.9.7, geth returns null if the block is pending.

https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_getblockbyhash

Context:
ethereum/go-ethereum#20587

How can it be fixed?

Check to make sure that web3 can handle either a null response or a block number. This probably means adding some testing. Will also need to check parity's responses.

[tmckenzie51]

Hi. I would like to write tests for this fix (#20616). However, I have a few questions on how to go about this.

1. I went through the web3 codebase to see where getBlockByHash/getBlockByNumber/getBlock are called and tested. I came across the test below for pending blocks. Given the different return values in the different geth versions, how is it that the test below is currently passing?

def test_eth_getBlockByNumber_pending(
    self, web3: "Web3", empty_block: BlockData
) -> None:
    current_block_number = web3.eth.blockNumber
    block = web3.eth.getBlock('pending')
    assert block['number'] == current_block_number + 1

2. I'm not understanding how merge #20616 fixes the issue. From what I understand based on the changed files in the merge it seems to be returning null for pending blocks. How does this ensure that web3 can handle both null and number return values?

3. Regarding adding appropriate tests in web3 for this issue and its fix, I've attached the small block of code below. Is there a way to specify the version of geth in the code below? How may I go about testing against different versions of geth?

def test_eth_getBlockByNumber_pending(
    self, web3: "Web3", empty_block: BlockData
) -> None:
    #TODO: SPECIFY VERSION
    current_block_number = web3.eth.blockNumber
    block = web3.eth.getBlock('pending')
    assert block['number'] == None

Any help would be appreciated. Thank you!

[kclowes]

Given the different return values in the different geth versions, how is it that the test below is currently passing?

If I'm reading the geth releases correctly, it looks like the change from #20616 went in to geth version 1.9.11. Currently, the geth 1.9 version that we're testing against is 1.9.7, so this test is passing because 1.9.7 returns a block number instead of null. Since that's the case, actually testing this PR just got a little more complicated. I'm not sure that it's a great use of our time since I'm pretty sure that this line is what allows us to handle either null or a block number: https://github.com/ethereum/web3.py/blob/master/web3/_utils/method_formatters.py#L380-L381. What I'll do is spend a little bit of time next week making a new fixture to test against 1.9.11+ and if there are a bunch of breakages I'll probably just keep the test at 1.9.7, otherwise fixing a few failing tests might be a good task for you if you're up for it. I'll keep you posted!

I'm not understanding how merge #20616 fixes the issue.

#20616 doesn't fix this issue, it just fixes an issue in the go-ethereum code base, where they wanted to return null for a pending block. It's just referenced here.

3. That is exactly how you would test it! Here is an example of testing for a specific geth version: https://github.com/ethereum/web3.py/blob/master/tests/integration/go_ethereum/common.py#L76. Let me know if you need more clarification than that.

Thank you!

[kclowes]

@tmckenzie51 I forgot we'll have to add the most recent geth versions to py-geth before we can make a fixture. If you're interested, you can follow this PR to add the most recent geth versions to py-geth, and then I can generate a new fixture for you. Aside from this issue, we have another issue that was raised this week related to versions of geth 1.9.12+, so I think that would be a good use of your time!

[tmckenzie51]

@kclowes

That is exactly how you would test it! Here is an example of testing for a specific geth version: https://github.com/ethereum/web3.py/blob/master/tests/integration/go_ethereum/common.py#L76. Let me know if you need more clarification than that.

Thank you! That's very helpful! I'll use that to test against the different versions.

As of geth 1.9.11, eth_getBlockByHash and eth_getBlockByNumber will be returning a block number if the block is pending, instead of null. Older versions of geth were returning a number, but as of 1.9.7, geth returns null if the block is pending.

I think I’m having a bit confusion about the return values of the different geth versions. My understanding from the issue description here is:

>= 1.9.11 : returns number
>= 1.9.7 and < 1.9.11: returns null
< 1.9.7 : returns number

Currently, the geth 1.9 version that we're testing against is 1.9.7, so this test is passing because 1.9.7 returns a block number instead of null.

However, based on this it seems that geth 1.9.7 returns a number rather than null, but that seems to contradict with my initial understanding of the return values of the different versions that I outlined above. Is it then as outlined below instead? :

 >= 1.9.11 : returns number
> 1.9.7 and < 1.9.11: returns null
=< 1.9.7 : returns number

#20616 doesn't fix this issue, it just fixes an issue in the go-ethereum code base, where they wanted to return null for a pending block. It's just referenced here.

If I'm reading the geth releases correctly, it looks like the change from #20616 went in to geth version 1.9.11.

Also, based on this it seems that merge #20616 which returns null for pending blocks was applied to geth version 1.9.11. Does this then mean that 1.9.11 actually returns null rather than a number? Is it then as outlined below, instead? I've based this PR on this last interpretation.

>1.9.11 : returns number
> 1.9.7 and <= 1.9.11: returns null
=< 1.9.7 : returns number

I'm pretty sure that this line is what allows us to handle either null or a block number: https://github.com/ethereum/web3.py/blob/master/web3/_utils/method_formatters.py#L380-L381.

Thank you for sharing that line of code with me. I’m interpreting that line of code of taking a non-null value then doing block-formatting on it. How would this handle null values?

[tmckenzie51]

@kclowes I also found a package online called 'semver' that could help with semantic versioning. Is this necessary here? Is it possible for me to add new libraries? If so, how may I go about adding a library such that it is incorporated in the CI tests?

[kclowes]

Looks like this was fixed in #1790