[ETCM-878] blockchain tests : BlockchainTests/vmArithmeticTest

[AurelienRichez]

Description

This PR is part of the ogoing effort to make ETS pass all tests. The vmArithmeticTest suite did not pass because of several problems :

• mining reward: We did not handle the sealEngineparameter and were always running with NoReward when mantis was running in test mode
• The output of getAccountStorage sometime wrongly had leading zeros due to a sign bit added by BigInt.toByteAray.
• storageRangeAt did not return a range of value and took a key instead of a hash as input. This kind of worked because most test used 0x00 as storage key
• getAccountsInRange did not included the coinbase account. It also depended on some state instead of taking the hash given as a parameter to find the start of its range.

Fixing those problem also fixes lots of other tests

Important Changes Introduced

Most of the changes are external to the production code. The most impactful change is probably in BlockchainImpl where we now remove the leading zeros. I don't think this impact other parts of the application.

Regarding TestService.scala, I removed the variable accountRangeOffset. I also added some state in testmode with a map preimageCache. This map is used to keep track of what storage address were used during one test run and be able to implement storageRangeAt. This is not really elegant so I will be happy for suggestion on how to make it better.

Important note

While this PR improves the implementation of storageRangeAt and getAccountsInRange, they are still not completely functional. They only work in test mode. There are also some edge case not handled :

• getAccountsInRange is only able to return account known in the genesis state and the account used for coinbase (So it won't return account created in a transaction)
• storageRangeAt should be able to get the state not only on a given block but also after an arbitrary transaction inside the block. We currently only handle the first case. I'm not sure the tests actually need the second part so it might be alright for ETS.

Also the storageRangeAt implementation is not really smart. It just iterate over known keys instead of actually reading the Merkle Patricia Trie. For tests with few values in storage It's good enough though.

[jvdp]

While this PR improves the implementation of storageRangeAt and getAccountsInRange, they are still not completely functional. They only work in test mode.

With the knowledge you've gained fixing these tests so far: with so much of this stuff being test-mode specific, do you think there's a missed opportunity for covering more of the production code by ETS?

Also, is there an issue that the testservice now has very different operation compared to the other RPC services? (eg. the regular eth_ methods listed here: https://github.com/ethereum/retesteth/wiki/RPC-Methods) Or am I misunderstanding how this works?

[jvdp]

Cool! 👍

[jvdp]

Looks like a job for regexes to me!

[AurelienRichez]

You mean we should change the output when transforming to a String in test service ? Here we are using BigInt and byte arrays. As It seems not really correct that getAccountStorageAt might return 33 bytes I fixed it here.

[jvdp]

Do we have a ticket to track this lack of support? And are tests failing because of it?

[AurelienRichez]

We have ETCM-784 and ETCM-758 which are related to this. I'll add a comment there.

I did not find any test specifically failing because of this but I did not really look why other suites are failing. So I went with an iterative approach by implementing the minimum to make vmArithmeticTest pass.

[AnastasiiaL]

a couple non-blocking comments, otherwise looks good 👍🏻