[ETCM-72] Checkpoint Block validation by pslaski · Pull Request #706 · input-output-hk/mantis

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)

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

+      _ <- 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

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

    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

   * @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

  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).

ntallar

Only minor comments, apart from which LGTM!

ntallar

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

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

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

ntallar suggested 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 suggested 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 suggested changes Sep 30, 2020

View reviewed changes

Comment thread ...est/scala/io/iohk/ethereum/consensus/validators/BlockWithCheckpointHeaderValidatorSpec.scala Outdated

Comment thread ...est/scala/io/iohk/ethereum/consensus/validators/BlockWithCheckpointHeaderValidatorSpec.scala Outdated

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

Conversation

pslaski commented Sep 28, 2020

Description

Important Changes Introduced

Testing

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtkaczyk Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntallar Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntallar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtkaczyk Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ntallar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ntallar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rtkaczyk Sep 29, 2020 •

edited

Loading

ntallar Sep 29, 2020 •

edited

Loading

rtkaczyk Oct 1, 2020 •

edited

Loading

ntallar left a comment •

edited

Loading