[ETCM-72] Checkpoint Block validation #706

pslaski · 2020-09-28T17:12:07Z

Description

Added block with checkpoint validation

Important Changes Introduced

checkpoint block header validation
checkpoint block validation

Testing

only unit tests (as we don't have checkpoint block generator now)

src/main/scala/io/iohk/ethereum/consensus/blocks/BlockGenerator.scala

ntallar · 2020-09-28T17:45:55Z

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

+      _ <- blockWithCheckpointHeaderValidator.validate(blockHeader, parentHeader)
+      _ <- validateExtraData(blockHeader)
+      _ <- validateDifficulty(blockHeader, parentHeader)
+      _ <- validateGasLimit(blockHeader, parentHeader)


This differs from the original spec:

Extra data is determined to only be of size less than 32, can't anyone then change this value and still generate a valid checkpoint block? This easiness of generating valid blocks could be exploited, should we minimize that as much as possible and have this be deterministic?

Gas limit is determined to be in a range, doesn't that allow the same attack as in 1.?

Shouldn't it be better if checkpoint blocks had instead no impact on the block difficulty? The one second gap between this block and it's parent will increase the difficulty of this block if not

Expanding from 3. even if we do that there's likely a difference in timestamp between this block and the next (as checkpoint generation will take longer than 1 second), and that would also affect the chains difficutly. Ideally, given an average checkpoint generation time of X, shouldn't we set the timestamp of the checkpoint block to be parentHeader.unixTimestamp + X?

Regarding X, that's idealistic far beyond being realistic. ~~A more realistic approach could be: given chain B0 -> C -> B1 take (timestamp(B1) - timestamp(B0)) / 2 as the interval in the difficulty calculation.~~

Edit: nah, doesn't make sense, the interval between B1 and B0 will not be twice the average. I think the easiest and a sane thing to do would be to assume that the checkpoint does not affect the difficulty of the succeeding block so: difficulty(B1) == difficulty(B0).

We could later adjust it based on empirical data.

Separate task, right?

difficulty(B1) == difficulty(B0).

Doesn't this remove the adjusting possibility of ETC's block difficulty? The block B1 will take the same to mine independently on how much after C it was mined. If checkpoints will be as often as we've done so far (a checkpoint per 5 blocks) that might affect it

Separate task, right?

Yes, (here it is), but definitely with some discussions before on which exact rule we propose to have here

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

rtkaczyk · 2020-09-29T09:19:20Z

src/main/scala/io/iohk/ethereum/crypto/ECDSASignature.scala

@@ -48,6 +56,25 @@ object ECDSASignature {
    ECDSASignature(r, s, pointSign)
  }

+  def sign2(message: Array[Byte], keyPair: AsymmetricCipherKeyPair, chainId: Option[Byte] = None): ECDSASignature = {


What's this? I'm not seeing it used anywhere?

sorry I used it to generate signature with random k, will remove it

rtkaczyk · 2020-09-29T09:33:11Z

src/main/scala/io/iohk/ethereum/consensus/validators/std/StdBlockValidator.scala

-   * section 4.4.2 of http://paper.gavwood.com/):
-   *   - BlockValidator.validateTransactionRoot
-   *   - BlockValidator.validateOmmersHash
+   * This method allows validate that a BlockHeader matches a BlockBody.
   *
   * @param blockHeader to validate
   * @param blockBody to validate
   * @return The block if the header matched the body, error otherwise
   */
  def validateHeaderAndBody(blockHeader: BlockHeader, blockBody: BlockBody): Either[BlockError, BlockValid] = {


I hate the method names in this class. This one says that it validates the header but it does not. There are several others like that. Maybe we should create a task to address it?

I agree, I had some problems to understand what's going on there and what's a purpose because of naming. I will create a task.

https://jira.iohk.io/browse/ETCM-171

ntallar

Only minor comments, will continue reviewing it tomorrow

ntallar · 2020-09-29T16:42:54Z

src/main/scala/io/iohk/ethereum/crypto/ECDSASignature.scala

@@ -136,6 +144,15 @@ case class ECDSASignature(r: BigInt, s: BigInt, v: Byte) {
    */
  def publicKey(message: ByteString): Option[ByteString] =
    ECDSASignature.recoverPubBytes(r, s, v, None, message.toArray[Byte]).map(ByteString(_))
+
+  def toBytes: ByteString = {


Minor: as this is for testing purposes, shouldn't we re-use the rlp encoders instead of multiple serializers?

I'm not quite sure what you mean, this is different than RLP encoding. I need/desire/can't live without this method here, even if it's not used a single time in the codebase.

this is different than RLP encoding

This is why I'm asking, given that RLP is our default serializing way, it's easy to assume that any methods toBytes and fromBytes on our objects use RLP for serialization. If we want to keep them, shouldn't we rename them (maybe as toRawBytes and fromRawBytes?) or move them to a separate codec object?

move them to a separate codec object?

No! I need this here and you will too :)
As for renaming, I'm not sure, can't recall what this signature format is called (I don't think it's DER).

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

src/main/scala/io/iohk/ethereum/consensus/validators/std/StdBlockValidator.scala

ntallar

Only minor comments, apart from which LGTM!

...est/scala/io/iohk/ethereum/consensus/validators/BlockWithCheckpointHeaderValidatorSpec.scala

ntallar

Only minor pending discussion is the toBytes and fromBytes from ECDSASignature, apart from which LGTM!

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

pslaski requested review from rtkaczyk and ntallar September 28, 2020 17:12

ntallar requested changes Sep 28, 2020

View reviewed changes

rtkaczyk reviewed Sep 29, 2020

View reviewed changes

pslaski force-pushed the etcm-72-checkpoint-block-validator branch 2 times, most recently from 8603a1d to 5b500bf Compare September 29, 2020 14:03

ntallar requested changes Sep 29, 2020

View reviewed changes

ntallar added the BREAKS PROTOCOL Affects Node2Node interactions in a way that makes them not interoperate label Sep 29, 2020

pslaski force-pushed the etcm-72-checkpoint-block-validator branch 2 times, most recently from d0d0194 to 9bcb971 Compare September 30, 2020 12:30

ntallar requested changes Sep 30, 2020

View reviewed changes

...est/scala/io/iohk/ethereum/consensus/validators/BlockWithCheckpointHeaderValidatorSpec.scala Outdated Show resolved Hide resolved

...est/scala/io/iohk/ethereum/consensus/validators/BlockWithCheckpointHeaderValidatorSpec.scala Outdated Show resolved Hide resolved

pslaski force-pushed the etcm-72-checkpoint-block-validator branch from 9bcb971 to eb1ee83 Compare September 30, 2020 13:55

ntallar approved these changes Sep 30, 2020

View reviewed changes

rtkaczyk approved these changes Oct 1, 2020

View reviewed changes

pslaski force-pushed the etcm-72-checkpoint-block-validator branch from eb1ee83 to ba02fcc Compare October 1, 2020 12:26

etcm-72 added checkpoint block validator

f6079a4

pslaski force-pushed the etcm-72-checkpoint-block-validator branch from ba02fcc to f6079a4 Compare October 1, 2020 12:57

pslaski merged commit 8b0b08f into develop Oct 1, 2020

pslaski deleted the etcm-72-checkpoint-block-validator branch October 1, 2020 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ETCM-72] Checkpoint Block validation #706

[ETCM-72] Checkpoint Block validation #706

pslaski commented Sep 28, 2020

ntallar Sep 28, 2020

rtkaczyk Sep 29, 2020 •

edited

Loading

ntallar Sep 29, 2020 •

edited

Loading

rtkaczyk Sep 29, 2020

pslaski Sep 29, 2020

rtkaczyk Sep 29, 2020

pslaski Sep 29, 2020

pslaski Sep 29, 2020

ntallar left a comment

ntallar Sep 29, 2020

rtkaczyk Sep 30, 2020

ntallar Sep 30, 2020

rtkaczyk Oct 1, 2020 •

edited

Loading

ntallar left a comment

ntallar left a comment •

edited

Loading

[ETCM-72] Checkpoint Block validation #706

[ETCM-72] Checkpoint Block validation #706

Conversation

pslaski commented Sep 28, 2020

Description

Important Changes Introduced

Testing

Choose a reason for hiding this comment

rtkaczyk Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

ntallar Sep 29, 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

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

rtkaczyk Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

ntallar left a comment

Choose a reason for hiding this comment

ntallar left a comment • edited Loading

Choose a reason for hiding this comment

rtkaczyk Sep 29, 2020 •

edited

Loading

ntallar Sep 29, 2020 •

edited

Loading

rtkaczyk Oct 1, 2020 •

edited

Loading

ntallar left a comment •

edited

Loading