[ETCM-284] Pay no money to coinbase address when processing checkpoint blocks #767

pslaski · 2020-10-29T15:40:33Z

Description

All checkpoint blocks will currently be marked as invalid, as:

the block state root is set as the same as the parent
the resulting state root includes money provided to the coinbase address

Proposed Solution

The solution is a small optimization - block with checkpoint is skipping execution of transactions and paying the reward

Important Changes Introduced

block with checkpoint is not executing transactions, rewards and after execution validation
[bonus] added information about treasury and checkpoint to the json BlockResponse (I found it useful for testing)

Testing

unit tests + local tests with mocked miner and QA endpoints

rtkaczyk · 2020-10-29T16:34:49Z

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

    *
    * @param block the block being processed
    * @param worldStateProxy the initial state
    * @return the state after paying the appropriate reward to who corresponds
    */
+  // scalastyle:off method.length


Will we ever grow the balls to remove this useless tool altogether?

rtkaczyk · 2020-10-29T16:46:16Z

src/test/scala/io/iohk/ethereum/ledger/BlockExecutionSpec.scala

+        val (blocks, error) = blockExecution.executeBlocks(List(blockWithCheckpoint), defaultBlockHeader.difficulty)
+
+        blocks.size shouldBe 1
+        blocks.head.block shouldBe blockWithCheckpoint


Should we check that world state is unchanged? Can we do that here?

we can't as all intermediate world states are created and hidden inside components.
But we know that state is unchanged because the previous failing state root validation is running and passing normally (it is not mocked, MockValidatorsAlwaysSucceed is the validator of header and body)

we can't as all intermediate world states are created and hidden inside components.

But blockchain is not mocked and accessible here. Could we use that?

But we know that state is unchanged because the previous failing state root validation is running and passing normally (it is not mocked, MockValidatorsAlwaysSucceed is the validator of header and body)

I'm not quite sure which part of the code you are referring to

rtkaczyk · 2020-10-29T16:48:16Z

src/test/scala/io/iohk/ethereum/jsonrpc/JsonRpcControllerEthSpec.scala

+    response should haveResult(expectedBlockResponse)
+  }
+
+  it should "handle eth_getBlockByHash request (block with treasuryOut)" in new JsonRpcControllerFixture {


I'd prefer to be consistent with our naming so treasuryOptOut (besides treasuryOut sounds weird). This relates to several val names and string occurrences.

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

rtkaczyk · 2020-10-30T08:49:44Z

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

-        )
-        worldAfterTreasuryReward
-      }
+    if (block.hasCheckpoint) worldStateProxy


I wonder if we should move this condition outside to the prepareBlock method, so as to avoid running persist on an unchanged world state?

Since BlockExecutionSpec is possibly not the best place to test that the account is unchanged, can we add an appropriate test to BlockPreparatorSpec?

ntallar

Minor changes and LGTM!

ntallar · 2020-11-02T12:12:07Z

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

@@ -56,6 +56,7 @@ class BlockPreparator(
    * @param worldStateProxy the initial state
    * @return the state after paying the appropriate reward to who corresponds
    */
+  // scalastyle:off method.length


Should we remove this now this method remains unchanged?

ntallar · 2020-11-02T12:15:41Z

src/test/scala/io/iohk/ethereum/jsonrpc/JsonRpcControllerFixture.scala

@@ -113,6 +115,11 @@ class JsonRpcControllerFixture(implicit system: ActorSystem)
    unixTimestamp = 0
  )

+  val checkpoint = ObjectGenerators.fakeCheckpointGen(2, 5).sample.get
+  val blockWithCheckpoint = CheckpointingTestHelpers.createBlockWithCheckpoint(Fixtures.Blocks.Block3125369.header, checkpoint)


I thought we had removed this as it's duplicated with CheckpointBlockGenerator

I think it's my bad that it wasn't removed in the end. But most of its usages have been replaced with CheckpointBlockGenerator so it should be easy to remove now

ntallar · 2020-11-02T12:18:06Z

src/test/scala/io/iohk/ethereum/ledger/BlockExecutionSpec.scala

+        val blockExecution =
+          new BlockExecution(blockchain, blockchainConfig, newConsensus.blockPreparator, blockValidation)
+
+        val (blocks, error) = blockExecution.executeBlocks(List(blockWithCheckpoint), defaultBlockHeader.difficulty)


Should we rename this method to executeAndValidateBlocks for consistency?

ntallar · 2020-11-02T12:23:25Z

src/test/scala/io/iohk/ethereum/jsonrpc/JsonRpcControllerEthSpec.scala

+    response should haveResult(expectedBlockResponse)
+  }
+
+  it should "handle eth_getBlockByHash request (block with treasuryOptOut)" in new JsonRpcControllerFixture {


It's odd that the test from this file are integration tests, instead of the usual per service and jrc

it's because we have only one huge JRC service and here we have ~EthJRC tests

rtkaczyk · 2020-11-02T21:02:06Z

src/test/scala/io/iohk/ethereum/checkpointing/CheckpointingTestHelpers.scala

-      checkpoint: Checkpoint
-  ): Block = {
-    Block(createBlockHeaderWithCheckpoint(parentHeader, checkpoint), BlockBody(Nil, Nil))
-  }


Could we also remove the one below?

ntallar

LGTM! Was able to generate checkpoint blocks without any problem

 .\\            //.
. \ \          / /.
.\  ,\     /` /,.-
 -.   \  /'/ /  .
 ` -   `-'  \  -
   '.       /.\`
      -    .-
      :`//.'
      .`.'
      .' BP

pslaski requested review from rtkaczyk and ntallar October 29, 2020 15:40

rtkaczyk reviewed Oct 29, 2020

View reviewed changes

ntallar reviewed Oct 29, 2020

View reviewed changes

src/main/scala/io/iohk/ethereum/jsonrpc/BlockResponse.scala Show resolved Hide resolved

rtkaczyk reviewed Oct 30, 2020

View reviewed changes

pslaski force-pushed the etcm-284-no-coinbase-money-checkpoint-block branch from 28ed295 to 104775c Compare October 30, 2020 13:04

ntallar requested changes Nov 2, 2020

View reviewed changes

pslaski force-pushed the etcm-284-no-coinbase-money-checkpoint-block branch 3 times, most recently from 32768dc to da3b169 Compare November 2, 2020 16:32

rtkaczyk reviewed Nov 2, 2020

View reviewed changes

ETCM-284 block with checkpoint will not generate coinbase reward

2a457a9

pslaski force-pushed the etcm-284-no-coinbase-money-checkpoint-block branch from da3b169 to 2a457a9 Compare November 3, 2020 11:13

rtkaczyk approved these changes Nov 3, 2020

View reviewed changes

ntallar approved these changes Nov 3, 2020

View reviewed changes

ntallar merged commit 3b2198e into develop Nov 3, 2020

ntallar deleted the etcm-284-no-coinbase-money-checkpoint-block branch November 3, 2020 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ETCM-284] Pay no money to coinbase address when processing checkpoint blocks #767

[ETCM-284] Pay no money to coinbase address when processing checkpoint blocks #767

pslaski commented Oct 29, 2020 •

edited

Loading

rtkaczyk Oct 29, 2020

rtkaczyk Oct 29, 2020

pslaski Oct 29, 2020

rtkaczyk Oct 30, 2020

rtkaczyk Oct 29, 2020 •

edited

Loading

rtkaczyk Oct 30, 2020

ntallar left a comment

ntallar Nov 2, 2020

ntallar Nov 2, 2020

rtkaczyk Nov 2, 2020

ntallar Nov 2, 2020

ntallar Nov 2, 2020

pslaski Nov 2, 2020 •

edited

Loading

rtkaczyk Nov 2, 2020

ntallar left a comment

[ETCM-284] Pay no money to coinbase address when processing checkpoint blocks #767

[ETCM-284] Pay no money to coinbase address when processing checkpoint blocks #767

Conversation

pslaski commented Oct 29, 2020 • edited Loading

Description

Proposed Solution

Important Changes Introduced

Testing

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

rtkaczyk Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntallar 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

Choose a reason for hiding this comment

pslaski Nov 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntallar left a comment

Choose a reason for hiding this comment

pslaski commented Oct 29, 2020 •

edited

Loading

rtkaczyk Oct 29, 2020 •

edited

Loading

pslaski Nov 2, 2020 •

edited

Loading