[ETCM-70-71] Checkpointing domain + Checkpointing configuration #695

pslaski · 2020-09-24T09:01:52Z

Description

Ported checkpoint domain and configuration

Important Changes Introduced

checkpoint class
configuration fields for checkpointing
update block header RLP decoder / encoder according to https://input-output.atlassian.net/wiki/spaces/EMU/pages/1681686614/Checkpointing#RLP-encoding-(affects-network-messages-with-headers)

Testing

sync with mainnet with a fresh database

src/main/resources/application.conf

src/main/scala/io/iohk/ethereum/consensus/ConsensusConfig.scala

src/main/scala/io/iohk/ethereum/db/storage/BlockHeadersStorage.scala

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

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

ntallar · 2020-09-24T14:44:42Z

src/main/scala/io/iohk/ethereum/rlp/RLPImplicits.scala

+    case Some(value) => RLPList(enc.encode(value))
+  }
+
+  implicit def optionDec[T](implicit dec: RLPDecoder[T]): RLPDecoder[Option[T]] = {


Maybe we should start using this for the optOut field eventually 🤔

I think we can't as we are using custom Boolean encoding/decoding here.

Is it custom? I copied it from how it's done on the reverse field of GetBlockHeaders

we don't have any default one, but IMHO default Boolean could be true/false not 1/0 ;)

rtkaczyk · 2020-09-25T10:52:22Z

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

@@ -102,24 +128,42 @@ object BlockHeader {
  }

  implicit class BlockheaderEncodableDec(val rlpEncodeable: RLPEncodeable) extends AnyVal {


Not directly related to this PR, but I'm not fond of this naming: BlockheaderEncodableDec is by types symmetrical to BlockHeaderEnc (RLPEncodable => BlockHeader vs BlockHeader => RLPEncodable), so it should be named BlockHeaderDec.

What is currently BlockheaderDec should be renamed to BlockHeaderByteArrayDec, especially that it's only used in tests.

Note: Blockheader* -> BlockHeader*

rtkaczyk · 2020-09-25T11:03:02Z

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

+    case _ => throw new RuntimeException("Cannot decode Checkpoint")
+  }
+
+  implicit class CheckpointEnc(checkpoint: Checkpoint) extends RLPSerializable {


Do we need this class? It would be more concise to express it as a SAM type (same as the decoder).

rtkaczyk · 2020-09-25T11:16:51Z

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

+        case (None, Some(_)) =>
+          // Post ECIP1097 block with checkpoint, treasury disabled, block with checkpoint is encoded
+          RLPList(parentHash, ommersHash, beneficiary, stateRoot, transactionsRoot, receiptsRoot,
+            logsBloom, difficulty, number, gasLimit, gasUsed, unixTimestamp, extraData, mixHash, nonce, RLPList(), checkpoint)


It's a bit awkward. I wonder how other clients would deal with this - if they can easily switch encoding on a basis of block number then they might contest this design as unintuitive.

What if we had a single optional field interpreted as "block header extensions"? It would be always present after ECIP-X. Then we could explicitly state optionality of the new extensions in all cases, and it would be safe for any future extensions as well.

do you mean one additional field for BlockHeader where we will pack all extensions ? I'm ok with that, but as it will touch also treasury opt, IMHO it should be done in different PR as it will add a lot of noise to the PR.

Yes, definitely. This would be an important breaking change, so I'd like to consult that with others and plan it as proper a task.

I'm also not sure if this is the best we can do. @ntallar WDYT?

IMHO it would be also good to do something similar in BlockchainConfig

Wouldn't a block header extensions field only move this complexity there, with the same matching on it's size as here? Why would that simplify any future extensions?

The only thing I think would simplify quite a lot the enconding/decoding design is having a block version field as with bitcoin (we could add it as the optional 17th field). Then:

Version 1: no version field, traditional ETC block

Version 2: treasuryOptOut as 18th field + checkpoint as 19th field

Future version 3: sth as 20th field, ...

...

The spec didn't include that as I thought it might be an overkill, I haven't heard of future block headers changes for now

(I would also add any changes resulting from this discussion in a separate PR)

Why would that simplify any future extensions?

Not necessarily simplify, but we would always have explicitness regarding optionality. Anyway, I do like the version field idea better. I think this is the way to go 💯

ntallar

Apart from the discussion on versioning (for a separate PR I think), LGTM!

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

…encoder + added checkpointing config

pslaski added the BREAKS STATE Affects self state retro-compatibility, needs data wiping label Sep 24, 2020

pslaski requested review from kapke and ntallar September 24, 2020 09:01

pslaski force-pushed the etcm-70-etcm-71-checkpoint-domain-and-config branch from 793249b to 4110fad Compare September 24, 2020 09:12

pslaski requested review from rtkaczyk and removed request for kapke September 24, 2020 11:48

ntallar added BREAKS CONFIG Affects the default configuration BREAKS PROTOCOL Affects Node2Node interactions in a way that makes them not interoperate labels Sep 24, 2020

ntallar requested changes Sep 24, 2020

View reviewed changes

rtkaczyk reviewed Sep 25, 2020

View reviewed changes

pslaski force-pushed the etcm-70-etcm-71-checkpoint-domain-and-config branch from 4110fad to 33e15ed Compare September 25, 2020 12:41

ntallar approved these changes Sep 25, 2020

View reviewed changes

rtkaczyk approved these changes Sep 28, 2020

View reviewed changes

etcm-70-71 added checkpoint to block header + updated header decoder/…

d2e159e

…encoder + added checkpointing config

pslaski force-pushed the etcm-70-etcm-71-checkpoint-domain-and-config branch from 33e15ed to d2e159e Compare September 28, 2020 12:12

Merge branch 'develop' into etcm-70-etcm-71-checkpoint-domain-and-config

79984ea

pslaski merged commit e2d301f into develop Sep 28, 2020

pslaski deleted the etcm-70-etcm-71-checkpoint-domain-and-config branch September 28, 2020 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ETCM-70-71] Checkpointing domain + Checkpointing configuration #695

[ETCM-70-71] Checkpointing domain + Checkpointing configuration #695

pslaski commented Sep 24, 2020

ntallar Sep 24, 2020

pslaski Sep 25, 2020 •

edited

Loading

ntallar Sep 25, 2020

pslaski Sep 25, 2020

rtkaczyk Sep 25, 2020

rtkaczyk Sep 25, 2020

rtkaczyk Sep 25, 2020 •

edited

Loading

pslaski Sep 25, 2020

rtkaczyk Sep 25, 2020

pslaski Sep 25, 2020

ntallar Sep 25, 2020 •

edited

Loading

rtkaczyk Sep 25, 2020

ntallar left a comment

		@@ -102,24 +128,42 @@ object BlockHeader {
		}

		implicit class BlockheaderEncodableDec(val rlpEncodeable: RLPEncodeable) extends AnyVal {

[ETCM-70-71] Checkpointing domain + Checkpointing configuration #695

[ETCM-70-71] Checkpointing domain + Checkpointing configuration #695

Conversation

pslaski commented Sep 24, 2020

Description

Important Changes Introduced

Testing

Choose a reason for hiding this comment

pslaski Sep 25, 2020 • edited Loading

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

rtkaczyk Sep 25, 2020 • edited Loading

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 Sep 25, 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 Sep 25, 2020 •

edited

Loading

rtkaczyk Sep 25, 2020 •

edited

Loading

ntallar Sep 25, 2020 •

edited

Loading