[ETCM-927] enhance BlockchainTests/ValidBlocks/bcMultiChainTest/Chain… #1021

strauss-m · 2021-06-21T12:23:17Z

Description

Enhance the test ChainAtoChainB_difficultyB.
Currently, the test can't completely pass due to a missing feature: transactions are not executed on blocks that are not part of the canonical blockchain.

Proposed Solution

a block insertion on an alternate chain is now valid and queryable by hash
fields s, r and v in EthTransactionResponse are now pruned from leading 0.

Testing

The test BlockchainTests/ValidBlocks/bcMultiChainTest/ChainAtoChainB_difficultyB fails later, on the storage check, instead at the first insertion of a block on the non-canonical (best) chain.

Request: {"jsonrpc":"2.0","method":"debug_storageRangeAt","params":["3", 2, "0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b", "0x0000000000000000000000000000000000000000000000000000000000000000", 20],"id":26}
Reply: {"jsonrpc":"2.0","result":{"complete":true,"storage":{}},"id":26}
Error: Compare States: Missing expected address: '0x095e7baea6a6c7c4c2dfeb977efac326af552d87' (bcMultiChainTest/ChainAtoChainB_difficultyB_Istanbul, fork: Istanbul, block: 5)
Error: CompareStates failed with errors: MissingExpectedAccount (bcMultiChainTest/ChainAtoChainB_difficultyB_Istanbul, fork: Istanbul, block: 5)

AurelienRichez

A few remarks. I don't know how it will play out with @jvdp effort to remove Ledger however.

Anyway we have more ETS tests passing so 👍 .

src/main/scala/io/iohk/ethereum/ledger/BlockQueue.scala

src/main/scala/io/iohk/ethereum/testmode/TestModeServiceBuilder.scala

AurelienRichez · 2021-06-22T09:20:54Z

src/main/scala/io/iohk/ethereum/testmode/TestModeComponentsProvider.scala

-      consensus(blockchainConfig, sealEngine),
-      validationExecutionContext
+  private var cache = HashMap.empty[(BlockchainConfig, SealEngineType), Ledger]
+  private var blockQueue = BlockQueue(blockchain, syncConfig)


Why do we need a cache ? We would like to minimize state if possible. I think there is always only one ledger at a given time anyway during tests right ?

I guess we want to keep the same BlockQueue between the calls. Initially I tried to put all the state in the same place in TestService. It's not really a perfect place but it avoids scattering the state everywhere and be able to simplify it when possible. To be consistent we could inject blockQueue in def ledger, or put the current Ledger in TestService ?

In my test case, I got two different ledger instances due to different configs.
But maybe I should investigate deeper into this and make sure that for a given test, only one blockchain config is used, do you think it's a worthwhile effort ?
If there is only one config, one can drop the cache.

Were you able to investigate this?

No, I didn't push further the idea. I don't know if the suite is actually using different config depending on tests, which would prevent a unique one (initial setup + each test config) convergence, and require the current multi-config cache.

jvdp · 2021-06-23T12:46:32Z

I don't know how it will play out with @jvdp effort to remove Ledger however.

It'll be interesting but I'm not afraid!

leo-bogastry · 2021-06-23T13:33:35Z

src/main/scala/io/iohk/ethereum/testmode/TestEthBlockServiceWrapper.scala

-        BlockByBlockHashResponse(blockByBlockResponse.blockResponse.map(response => toEthResponse(fullBlock, response)))
-      })
+        blockByBlockResponse.blockResponse
+          .toRight("missing block response")


Can you add some more specific data to error?

I've added a little bit more context with regard to the error messages

leo-bogastry · 2021-06-28T14:52:46Z

src/main/scala/io/iohk/ethereum/testmode/TestEthBlockServiceWrapper.scala

+          )
+          .left
+          .map(errorMessage => JsonRpcError.LogicError(errorMessage))
+      }).flatten


You can make a few simplifications here:

move the import to the top of the file, please

remove the final .flatten and use flatMap instead

you can remove { after =>, by replacing the ( before blockByBlockResponse by {
In summary from:
_.map(blockByBlockResponse => { to _.flatMap { blockByBlockResponse =>

thanks, update done

jvdp · 2021-06-28T16:02:28Z

src/main/scala/io/iohk/ethereum/jsonrpc/EthBlocksService.scala

@@ -74,8 +74,8 @@ class EthBlocksService(
    */
  def getByBlockHash(request: BlockByBlockHashRequest): ServiceResponse[BlockByBlockHashResponse] = Task {
    val BlockByBlockHashRequest(blockHash, fullTxs) = request
-    val blockOpt = blockchainReader.getBlockByHash(blockHash)
-    val weight = blockchain.getChainWeightByHash(blockHash)


Are these two changes required by ETS? I wonder what would be a convenient way to refactor this with ledger gone...

The underlying need was to be able to get a block by hash, no matter if it's on the canonical chain or an alternate one. The solution was to delegate the work to the ledger, which implements the getBlockByHash exactly as it (first look in the canonical blockchain, then into the BlockQueue). The getChainWeightByHash was added to match the getBlockByHash behaviour.

The feature (being able to query a block by its hash, no matter its location) is required by the ETS. After a successful insert, retesteth queries for the insert block by its hash to do some consistency check.
It is not restricted to the ledger, so if any other component provides this after the refactor, let's use the new one.

jvdp · 2021-06-28T16:02:58Z

src/main/scala/io/iohk/ethereum/jsonrpc/TestService.scala

@@ -157,6 +161,9 @@ class TestService(

    // remove current genesis (Try because it may not exist)
    Try(blockchain.removeBlock(blockchain.genesisHeader.hash, withState = false))
+    // TODO clear the storage ? When relaunching some tests on the same running test mantis client,
+    // we end up with duplicate blocks because they are still present in the storage layer
+    // for example: bcMultiChainTest/ChainAtoChainB_BlockHash_Istanbul


How about when setChainParams is called?

do you mean put the cleaning into the setChainParams call, or into the setChainParams caller ?

jvdp · 2021-06-28T16:04:43Z

src/main/scala/io/iohk/ethereum/ledger/Ledger.scala

+    * @param hash the hash of the block to retrieve the chain weight
+    * @return the chain weight if the block represented by its hash is either stored in the blockchain or enqueued
+    */
+  def getChainWeightByHash(hash: ByteString): Option[ChainWeight]


Yeah please don't add more things to ledger 😅

It is linked to the Ledger.getBlockByHash call.
According to their usage in EthBlocksService, getBlockByHash() and getChainWeightByHash() should both be available on whatever component provides them.
The refactor should be straightforward, wherever the getBlockByHash() method ends up, you can put the getChainWeightByHash() one too. The underlying behaviour is also similar to getBlockByHash: first query the (canonical) BlockChain, then the BlockQueue if not found.

jvdp · 2021-06-28T16:06:30Z

src/main/scala/io/iohk/ethereum/testmode/TestEthBlockServiceWrapper.scala

+          )
+          .left
+          .map(errorMessage => JsonRpcError.LogicError(errorMessage))
+      }


This is a bit hard to follow, might be nicer as for-comprehension?

…AtoChainB_difficultyB test

strauss-m force-pushed the fix/ETCM-927_ChainAtoChainB_difficultyB branch from 5a78c4f to 83ed8c6 Compare June 21, 2021 15:14

AurelienRichez reviewed Jun 22, 2021

View reviewed changes

strauss-m force-pushed the fix/ETCM-927_ChainAtoChainB_difficultyB branch 2 times, most recently from af66053 to f9f5968 Compare June 23, 2021 08:47

leo-bogastry reviewed Jun 23, 2021

View reviewed changes

strauss-m force-pushed the fix/ETCM-927_ChainAtoChainB_difficultyB branch from fc3aba3 to c62a8ce Compare June 28, 2021 09:58

strauss-m requested review from AurelienRichez and leo-bogastry June 28, 2021 13:03

leo-bogastry reviewed Jun 28, 2021

View reviewed changes

leo-bogastry approved these changes Jun 29, 2021

View reviewed changes

jvdp reviewed Jun 29, 2021

View reviewed changes

AurelienRichez approved these changes Jun 30, 2021

View reviewed changes

strauss-m added 6 commits June 30, 2021 16:52

[ETCM-927] enhance BlockchainTests/ValidBlocks/bcMultiChainTest/Chain…

9246833

…AtoChainB_difficultyB test

[ETCM-927] improve error messages in EthBlockService.getBlockByHash()

8eb7258

[ETCM-781] inserted blocks causing chain reorganizion are now valid

6a44fa5

[ETCM-927] fix test compilation error following merge

92a9f30

[ETCM-927] syntax simplification

cdbed23

[ETCM-927] make TestEthBlockServiceWrapper.getBlockByHash easier to read

e8a425f

strauss-m force-pushed the fix/ETCM-927_ChainAtoChainB_difficultyB branch from a78b02a to e8a425f Compare June 30, 2021 15:00

strauss-m merged commit cbf21d2 into develop Jun 30, 2021

strauss-m deleted the fix/ETCM-927_ChainAtoChainB_difficultyB branch June 30, 2021 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ETCM-927] enhance BlockchainTests/ValidBlocks/bcMultiChainTest/Chain… #1021

[ETCM-927] enhance BlockchainTests/ValidBlocks/bcMultiChainTest/Chain… #1021

strauss-m commented Jun 21, 2021

AurelienRichez left a comment

AurelienRichez Jun 22, 2021

strauss-m Jun 22, 2021

leo-bogastry Jun 28, 2021

strauss-m Jun 28, 2021

jvdp commented Jun 23, 2021

leo-bogastry Jun 23, 2021

strauss-m Jun 28, 2021

leo-bogastry Jun 28, 2021

strauss-m Jun 28, 2021

jvdp Jun 28, 2021

strauss-m Jun 29, 2021

jvdp Jun 28, 2021

strauss-m Jun 29, 2021

jvdp Jun 28, 2021

strauss-m Jun 29, 2021

jvdp Jun 28, 2021

[ETCM-927] enhance BlockchainTests/ValidBlocks/bcMultiChainTest/Chain… #1021

[ETCM-927] enhance BlockchainTests/ValidBlocks/bcMultiChainTest/Chain… #1021

Conversation

strauss-m commented Jun 21, 2021

Description

Proposed Solution

Testing

AurelienRichez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvdp commented Jun 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment