Add integration tests on block processing

[ahamlat]

PR description

This PR aims to enhance block processing testing, inspired by this comment from @garyschulte regarding test coverage. The goal is to develop unit tests that focus on executing block transactions without concerning header and body validations. For each scenario specified by @matkt, we test both sequential and parallel processing to ensure state changes remain consistent.

The tested cases are:

• Test 1

  • Trx1: Transfer from a to b
  • Trx2: Transfer from a to c

• Test 2

  • Trx1: Transfer from a to b
  • Trx2: Transfer from b to c

• Test 3

  • Trx1: Transfer from a to b
  • Trx2: Transfer from c to d

• Test 4

  • Trx1: Transfer from a to b
  • Trx2: Transfer from c to coinbase

• Test 5

  • Trx1: Contract with read from account A
  • Trx2: Contract with update to account A

• Test 6

  • Trx1: Contract with update to account A
  • Trx2: Contract with read from account A

• Test 7

  • Trx1: Contract with update to account A
  • Trx2: Contract with read from account B

• Test 8 (Slots)

  • Trx1: Contract with read from slot S1
  • Trx2: Contract with update to slot S1

• Test 9 (Slots)

  • Trx1: Contract with update to slot S1
  • Trx2: Contract with read from slot S1

• Test 10 (Slots)

  • Trx1: Contract with update to slot S1
  • Trx2: Contract with read from slot S2

The Solidity contract file has been added for reference. It represents the smart contract that the bytecode is included in the genesis file. Including this file in the PR helps in understanding the tests.

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

• Checked out our contribution guidelines?
• Considered documentation and added the doc-change-required label to this PR if updates are required.
• Considered the changelog and included an update if required.
• For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

• unit tests: ./gradlew build
• acceptance tests: ./gradlew acceptanceTest
• integration tests: ./gradlew integrationTest
• reference tests: ./gradlew ethereum:referenceTests:referenceTests

[siladu]

Since it's a large test class, I think readability would be improved if it was a parameterized test with sequentialBlockProcessor and parallelBlockProcessor as the two parameters - what you do think?

[ahamlat]

Since it's a large test class, I think readability would be improved if it was a parameterized test with sequentialBlockProcessor and parallelBlockProcessor as the two parameters - what you do think?

To be honest, I believe that using parameterized tests may reduce readability. This is because the same test will be executed twice with identical names for both sequential and parallel block processing. At present, each test has a distinct name that clearly indicates its purpose.

[macfarla]

think renaming the keyPair variables to match the corresponding accounts would help readability

[siladu]

This is because the same test will be executed twice with identical names for both sequential and parallel block processing. At present, each test has a distinct name that clearly indicates its purpose.

That's true and it's personal preference whether you like parameterized tests or not. IMO, reducing 20 methods -> 10 methods in a big file is worth it. Also the framework allows you to make the distinction clear, e.g. you can label each processor...

 private static Stream<Arguments> blockProcessors() {
  ...
    return Stream.of(
        Arguments.of("Sequential", sequentialBlockProcessor),
        Arguments.of("Parallel", parallelBlockProcessor)
    );

  @ParameterizedTest
  @MethodSource("blockProcessors")
  void testBlockProcessingWithTransfers(final MainnetBlockProcessor blockProcessor) {
    processSimpleTransfers(blockProcessor);
  }

and it appears in the test results as such...

testBlockProcessingWithTransfers
 - [1] Sequential
 - [2] Parallel

Might we end up with more variants of BlockProcessors as we develop new strategies for parallel processing/conflict detection?

[siladu]

LGTM, all comments are non-blocking regarding naming and style.

[ahamlat]

and it appears in the test results as such...

testBlockProcessingWithTransfers

• [1] Sequential
• [2] Parallel

Ah nice, I haven't tried before, in this case, it makes sens.