[ETCM-143] Redefine block header structure and encoding #709

ntallar · 2020-09-29T16:33:13Z

Description

Improves both the block header structure and the way it's encoded

Changes are strongly based on the assumption that ECIP1098 will be activated before ECIP1097. However, if that assumption is not true, it can be easily re-adjusted, for that reason I'd consider an overkill providing a full mechanism to support both orders. [1]

Important Changes Introduced

Block header encoding

Block header encoding is done as if we had 3 different types of headers:

PreECIP1098 (treasury and checkpointing disabled): the block is encoded as currently on the mainnet
PostECIP1098 (treasury enabled but checkpointing disabled): the block is encoded as if it's fields were all the current fields from ETC :+ (treasuryOptOut: Boolean)
PostECIP1097 (treasury and checkpointing enabled): the block is encoded as if it's fields were all the current fields from ETC :+ (treasuryOptOut: Boolean) :+ (checkpoint: Option[Checkpoint])

In this way the type of the block can be determined exclusively by the number of fields the block has. Note that this (with the block number validations) is identical to determining the block header type through the block number

This mechanism is based on the fact that each ECIP activated added new fields to the block header, if future ECIPs want to also remove fields they should follow other mechanisms [1]

Block header structure

Instead of having 3 different block headers structures, as mentioned on the encoding section, this is analogously done as an extra fields field on the block header, with one possible value for each of the 3 header structures

Testing

Syncing to ETC mainnet and mordor testnet should continue working
Treasury feature should continue working as expected

Pending

Manual tests on ETC compatibility
Manual tests on treasury working as expected

Notes

[1] Two possible mechanism that would allow ECIP1097 to be activated before ECIP1098 as well as removing fields:

Include an only encoding field of blockType, depending on which we know the type of the block and so appropriately select it's decoder
Have the decoder depend on the ECIPs configuration, and so determine the block type by the block number (this is probably preferred as it doesn't involve further protocol changes)

rtkaczyk · 2020-09-30T07:01:22Z

src/main/scala/io/iohk/ethereum/domain/BlockHeader.scala

+
+  sealed trait HeaderExtraFields
+  object HeaderExtraFields {
+    case object HefPreEcip1098 extends HeaderExtraFields


I think a better name would be HefEmpty - it would immediately stand out from the others and be associated with the default case, whereas the current name requires some attention to be distinguished from others.

rtkaczyk · 2020-09-30T07:05:07Z

src/main/scala/io/iohk/ethereum/domain/BlockHeader.scala

  }

+  // scalastyle:off parameter.number
+  def buildPreECIP1098Header(parentHash: ByteString,


Is it worth adding all these methods? Maybe we could just have a default param extraFields: HeaderExtraFields = HefEmpty on the default constructor? And as for the extra fields, I don't think wrapping them in the HEF-constructor is that tedious to necessitate these methods

I found them useful when changing the block header structure as that required less changes. Given that and that we might eventually further change it (in case we want 3 different header structures) I saw no reason to delete them once I had them

But I don't have too much of a strong opinions right now, if you have stronger opinions on deleting them I can do so

Well, I think that using a default arg for extraFields would result in even fewer changes. Any future changes should also require less maintenance. Do you think there is another advantage to having these factories?

Tbh I don't like much default arg in general (exceptuating some use cases) as I see them too error prone... but given the subjectivity of this and that the PR has been open for a long time I'll change this

…-143-defining-blockheader-structure

KonradStaniec · 2020-10-05T12:50:35Z

src/main/scala/io/iohk/ethereum/consensus/validators/BlockHeaderValidatorSkeleton.scala

@@ -77,7 +78,7 @@ abstract class BlockHeaderValidatorSkeleton(blockchainConfig: BlockchainConfig)
      _ <- validateGasUsed(blockHeader)
      _ <- validateGasLimit(blockHeader, parentHeader)
      _ <- validateNumber(blockHeader, parentHeader)
-      _ <- validateOptOut(blockHeader)
+      _ <- validateExtraFields(blockHeader)


not sure if this is important, but this is basically validating if the whole structure of header is valid. Should't we do it as first thing ?

For attack purposes or for facilitating our usage of it?

I'm not sure if the order of the validations should matter that much here as they all are quite simple and easy to check (except from the PoW validation that probably takes more). So I'm not convinced if it would be better for them to have a particular order

KonradStaniec · 2020-10-05T12:54:07Z

src/main/scala/io/iohk/ethereum/domain/BlockHeader.scala

+    val rlpItemsWithoutNonce = blockHeader.extraFields match {
+      case HefPostEcip1097(_ , _) =>
+        // Post ECIP1098 & ECIP1097 block
+        rlpList.items.dropRight(4) ++ rlpList.items.takeRight(2)


those constants could be defined somwhere and named, as i am not sure what this rlpList.items.takeRight(2) bit means

KonradStaniec · 2020-10-05T13:18:15Z

src/test/scala/io/iohk/ethereum/ObjectGenerators.scala

@@ -139,6 +141,16 @@ trait ObjectGenerators {
    td <- bigIntGen
  } yield NewBlock(Block(blockHeader, BlockBody(stxs, uncles)), td)

+  def extraFieldsGen: Gen[HeaderExtraFields] = for {


I know we have property tests for it, but maybe it is wortth while to create some specific encoding cases like in: https://github.com/input-output-hk/mantis/blob/develop/src/test/scala/io/iohk/ethereum/network/p2p/messages/ReceiptsSpec.scala#L38, it will give someone inspecting it in the future idea how encoded payload looks like.

…getEncodedWithoutNonce for greater readability

ntallar added BREAKS PROTOCOL Affects Node2Node interactions in a way that makes them not interoperate BREAKS STATE Affects self state retro-compatibility, needs data wiping labels Sep 29, 2020

ntallar requested review from kapke, KonradStaniec and rtkaczyk September 29, 2020 16:33

ntallar force-pushed the etcm-143-defining-blockheader-structure branch from 355e8ec to 7d2c569 Compare September 29, 2020 16:34

ntallar mentioned this pull request Sep 29, 2020

[ETCM-72] Checkpoint Block validation #706

Merged

[ETCM-143] Redefine block header structure and encoding

357e5ad

ntallar force-pushed the etcm-143-defining-blockheader-structure branch from 7d2c569 to 357e5ad Compare September 29, 2020 18:31

rtkaczyk reviewed Sep 30, 2020

View reviewed changes

Nicolas Tallar added 3 commits September 30, 2020 11:02

[ETCM-143] Renaming HefPreEcip1098 to HefEmpty

0267c8c

Merge branch 'develop' of github.com:input-output-hk/mantis into etcm…

8d98f57

…-143-defining-blockheader-structure

[ETCM-143] Merged with develop: adapted validations to new structure

637d6e2

KonradStaniec reviewed Oct 5, 2020

View reviewed changes

Nicolas Tallar added 2 commits October 7, 2020 15:48

[ETCM-143] Merged with develop

9019f17

[ETCM-143] Added specific encoding test cases for header; refactored …

1c61f3c

…getEncodedWithoutNonce for greater readability

ntallar requested review from rtkaczyk and KonradStaniec October 7, 2020 19:35

KonradStaniec approved these changes Oct 8, 2020

View reviewed changes

Nicolas Tallar added 2 commits October 9, 2020 14:56

[ETCM-143] Merged with develop

bc3924e

[ETCM-143] Replaced BlockHeader builders with default argument

4321bc7

rtkaczyk approved these changes Oct 12, 2020

View reviewed changes

kapke approved these changes Oct 13, 2020

View reviewed changes

Nicolás Tallar added 2 commits October 14, 2020 10:33

Merge branch 'develop' into etcm-143-defining-blockheader-structure

efae23a

[ETCM-143] Fixed automatic merge compilation not working

c4c9977

ntallar merged commit a66c59f into develop Oct 14, 2020

ntallar deleted the etcm-143-defining-blockheader-structure branch October 14, 2020 18:26

ntallar mentioned this pull request Oct 28, 2020

[Fix] Non checkpoint blocks after ECIP1097 should have HefPostEcip1097 extra fields #762

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ETCM-143] Redefine block header structure and encoding #709

[ETCM-143] Redefine block header structure and encoding #709

ntallar commented Sep 29, 2020 •

edited

Loading

rtkaczyk Sep 30, 2020

rtkaczyk Sep 30, 2020

ntallar Sep 30, 2020

rtkaczyk Oct 8, 2020

ntallar Oct 9, 2020

KonradStaniec Oct 5, 2020

ntallar Oct 7, 2020

KonradStaniec Oct 5, 2020

KonradStaniec Oct 5, 2020

[ETCM-143] Redefine block header structure and encoding #709

[ETCM-143] Redefine block header structure and encoding #709

Conversation

ntallar commented Sep 29, 2020 • edited Loading

Description

Important Changes Introduced

Block header encoding

Block header structure

Testing

Pending

Notes

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

ntallar commented Sep 29, 2020 •

edited

Loading