blockhash always returns zero in solidity tests for constantinople

[ekpyron]

In ethereum/solidity#4799 we introduce semantics tests for blockhash() in solidity, but the blockhash opcode always returns zero in our tests when setting the EVM version to constantinople.

This may be related to #4066 and may also just be a problem in solidity's test setup.

In the solidity tests the constantinople fork block is set to zero (https:/ethereum/solidity/blob/43db88b8363d73ee2f5ffa094ff506414261bd11/test/RPCSession.cpp#L238) and we rewind the chain to block zero before every test (https:/ethereum/solidity/blob/43db88b8363d73ee2f5ffa094ff506414261bd11/test/ExecutionFramework.cpp#L59).

[ekpyron]

Pre-deploying the blockhash contract during genesis seems to help, but caused other complications for our tests, so we will postpone this issue and disable the blockhash test for constantinople for now.

[gumb0]

To summarize what we discussed verbally:

1. Current spec for BLOCKHASH refactoring EIP requires some irregular state changes, first of them is deployment of the blockhash contract at the beginning of the fork block. So this would work correctly only if Constantinople fork block is set to block number >= 1 (block number 0 is genesis and it is processed differently from all the rest, and it's not possible to do this step for genesis)
2. When Constantinople fork block number = 0, probably this should work fine, too, if you include the blockhash contract into genesis config (with the correct address, code and probably also storage containing the result of the first call to it (not sure if we support giving storage in genesis config))
3. It is very probable that this EIP is not included into Constantinople in the end, then I'll just remove this feature and BLOCKHASH will work as usual.

[chfast]

Can you move the feature to "experimental" chain config that is after the Constantinople. I remember we discussed creating it.

[gumb0]

I'll move it, that config already exists, I moved EIP86 account abstraction into it.

[chfast]

I was not sure. 👍

[winsvega]

If constantinople block is 0 you could hardcode that genesis state has this blockhash contract by default.
I dont like the idea of splitting hardfork changes into configs. if you put this contract into genesis config this means that you could initialize constantinople fork without this contract or with some other contract, but the contract itself would be hardcoded anyway.

As we do not have a standart of adding precompiled contracts with genesis config (and we dont have a standart of genesis config itself) I say all hardfork feautures should be hardcodede then. and triggered by hardfork block number.

[gumb0]

BLOCKHASH refactoring moved to "exerimental" fork, @ekpyron please check that it solved your issue

[chfast]

Is this fixed by #5225 ?

[axic]

@ekpyron won't be back before next week and I don't know where he had the issue.

[ekpyron]

Hm. The issue was that we added a blockhash semantics test to our test suite, but blockhash always returned zero for constantinople. This is indeed fixed with recent versions of aleth in that I now get consistent values for all evm versions. However, there seems to be some change in the behavior between the solidity tester release of aleth and current develop (consistently across evm versions).

It looks like with current develop I get the last 255 hashes, whereas with "solidity tester" I get the last 256 hashes. Furthermore there seems to be an offset of one.

blockhash(block.number - 257) is zero in both cases, but in develop blockhash(block.number - 256) is zero as well, but non-zero for solidity tester. Then what used to be blockhash(block.number - 256) in solidity tester is now blockhash(block.number - 255) on develop and so on, until blockhash(block.number - 1) on develop is what blockhash(block.number - 2) was for "solidity tester" and the last hash I get with solidity tester is not present on current develop...

[ekpyron]

What we do is to run the solidity code in https:/ethereum/solidity/blob/410d288dfc2e08c42df58c7e01ad5c332ce92727/test/libsolidity/SolidityEndToEndTest.cpp#L3049 and to compare with eth_getBlockByNumber with the block number of the transaction receipt minus some offsets.

[ekpyron]

Seems that we get different bock numbers than before. I'll recheck whether I can find out why that is.

[ekpyron]

I'm closing this one, since blockhash indeed works as expected now. I'll look into the block number issue separately and open another issue, if appropriate.

[gumb0]

@ekpyron test_mineBlocks was changed recently, maybe that could affect block numbers you see.
(It was changed to be more reliable - now it mines synchronously the exact number of blocks requested; previously it could mine more than that. Also I believe soltest should be able now to get rid of actively polling aleth with eth_getBlockNumber to wait for mining to finish)

[winsvega]

wait when it was mining more blocks then asked?

[gumb0]

I don't know if it did in practice, but it certainly could - checking how many we mined + stopping mining was not atomic.

(Actually I saw this happening but not with the RPC method, but with another similar method in unit tests)

[ekpyron]

I created ethereum/solidity#4951 for the changes needed in solidity for the tests to work against recent aleth - it seems like the block numbers are different, but actually make more sense than before...