[ETCM-263] improve BlockImport wrt checkpoints - ChainWeight by rtkaczyk · Pull Request #769 · input-output-hk/mantis

rtkaczyk · 2020-11-02T11:22:50Z

Description

Improves branch comparison wrt checkpoints during block import

Proposed Solution

Introduces a unified idea of ChainWeight

Testing

Unit tests

pslaski

only minor things

pslaski · 2020-11-02T15:31:44Z

      val newBlockNr = block.number
      val nextExpectedBlock = state.lastFullBlockNumber + 1

+      println("CODO KURWY???")


💯 ❤️ 💟

pslaski · 2020-11-02T15:37:52Z

+  def getChainWeightByNumber(blockNumber: BigInt): Option[ChainWeight] =
+    getHashByBlockNumber(blockNumber).flatMap(getChainWeightByHash)
+
+//  /**


can be removed now

pslaski · 2020-11-02T15:39:35Z

+
+object ChainWeight {
+  //FIXME: a shorter name?
+  def totalDifficultyOnly(td: BigInt): ChainWeight =


hmm totalDifficulty for me looks ok too.

I'd rather have these extra 4 letters to distinguish from ChainWeight#totalDifficulty

pslaski · 2020-11-02T15:42:54Z

+
+  //Test API
+
+  def increaseTd(td: BigInt): ChainWeight =


increaseTd => increaseTotalDifficulty

pslaski · 2020-11-02T15:45:15Z

+    ChainWeight(0, 0)
+}
+
+case class ChainWeight(


pslaski · 2020-11-02T16:08:05Z

+            RLPList(
+              protocolVersion,
+              networkId,
+              //TODO: should it be an RLPList? Should we define encoding in ChainWeight object?


👍 for encoding in ChainWeight object but move it to the end or merge both RLPLists into one

I'm not yet sure how to handle that while keeping the compatibility with v63 messages

pslaski · 2020-11-02T16:09:20Z

              ),
-              totalDifficulty,
-              latestCheckpointNumber
+              //TODO: should it be an RLPList? Should we define encoding in ChainWeight object?


pslaski · 2020-11-02T16:10:27Z

    ByteString(compactPickledBytesToArray(buffer))
  }

+  def byteSequenceToBuffer(bytes: IndexedSeq[Byte]): ByteBuffer =


Can you use it in other KeyValueStorages ?

pslaski · 2020-11-02T16:10:58Z

    <logger name="io.iohk.ethereum.vm.VM" level="OFF" />

-    <root level="INFO">
+    <root level="DEBUG">


is it spamming too much ?

pslaski · 2020-11-02T16:12:39Z


        peerEventBus.expectMsgClass(classOf[Subscribe])
-        peerEventBus.reply(MessageFromPeer(NewBlock(testBlocks.last, testBlocks.last.number), defaultPeer.id))
+        // It's weird that we're using block number for total difficulty but I'm too scared to fight this dragon


This test continues to be the most difficult one to follow, at least among the ones I've dealt with recently

ntallar

First round of comments

ntallar · 2020-11-02T17:42:46Z

+            go(newBlockData :: executedBlocks, remainingBlocks.tail, newWeight, None)
          case Left(executionError) =>
-            go(executedBlocks, remainingBlocks, 0, Some(executionError))
+            go(executedBlocks, remainingBlocks, parentWeight, Some(executionError))


Instead of parentWeight shouldn't it be parentWeight.increase(blockToExecute.header)? The next block to be executed will be the son of the current one

Although I'm not sure what's the purpose of continuing executing a branch on which one of the blocks failed to be executed

ntallar · 2020-11-02T18:09:13Z

      case (_, Hello.code) => payload.toHello

+      //FIXME: I still have an issue with protocolVersion, we are use PV62 and PV63 and code names for Status and NewBlock
+      // suggest that there is PV64, but there isn't


Hmm now that you say it makes sense to have a PV64

It's weird that ETC has some duplication between the capability from Hello messages and this status field (we even use the same Versions.PV63 variable), but in copying them we should probably have that duplication as well

It might also be worth researching the difference in purposes between the 2 in case we are missing something

Should we research that as part of 280? Or maybe a separate task?

Hmm maybe it would be better to make it part of ETCM-280? In order to be sure of the approach we are taking there

We could split it later into more tasks if we detect that it's taking longer than expected

pslaski · 2020-11-04T09:22:26Z

        storage.persist()
        stateStorage.forcePersist(GenesisDataLoad)
-        blockchain.save(Block(header, BlockBody(Nil, Nil)), Nil, header.difficulty, saveAsBestBlock = true)
+        // TODO: factory for ChainWeight?


you have it already ;) ChainWeight.totalDifficultyOnly

pslaski · 2020-11-04T09:24:45Z


  override def valueDeserializer: IndexedSeq[Byte] => BlockHeader =
-    bytes => Unpickle[BlockHeader].fromBytes(ByteBuffer.wrap(bytes.toArray[Byte]))
+    byteSequenceToBuffer _ andThen Unpickle[BlockHeader].fromBytes


WDYT about creating helper for byteSequenceToBuffer _ andThen Unpickle[Type].fromBytes ?

I'd prefer to merge it ASAP, but added a TODO

pslaski · 2020-11-04T09:40:19Z

-      case (None, None) =>
-        compareByDifficulty(newBranch, oldBranch)
+      case None =>
+        // TODO: should we log if parentWeight not found?


Cause the code would be ugly :) This match doesn't capture the situation - there could be no parent to get the weight for

Maybe we could change it a bit it to detect that case? More of this logs never bothered, we never know when it will be useful

pslaski · 2020-11-04T09:42:15Z

-    def genesisHash: ByteString
+        genesisHash: ByteString
+    ): Status =
+      //TODO: explain, remove if possible


pslaski · 2020-11-04T09:42:53Z

-    def as63: NewBlock63 =
-      NewBlock63(block, totalDifficulty)
+    def apply(block: Block, chainWeight: ChainWeight): NewBlock =
+      //TODO: explain, remove if possible


ntallar

Last comments, got to take a look at all the changes!

Code is looking much nicer ❤️

ntallar · 2020-11-04T12:49:56Z

-      case (None, None) =>
-        compareByDifficulty(newBranch, oldBranch)
+      case None =>
+        // TODO: should we log if parentWeight not found?


Maybe we could change it a bit it to detect that case? More of this logs never bothered, we never know when it will be useful

ntallar · 2020-11-04T13:49:34Z

      case (_, Hello.code) => payload.toHello

+      //FIXME: I still have an issue with protocolVersion, we are use PV62 and PV63 and code names for Status and NewBlock
+      // suggest that there is PV64, but there isn't


Hmm maybe it would be better to make it part of ETCM-280? In order to be sure of the approach we are taking there

We could split it later into more tasks if we detect that it's taking longer than expected

ntallar

Synced 50k blocks from mordor without any problems! Though I had to fix an issue

ntallar · 2020-11-05T18:39:11Z

+        .orElse(newHeaders.headOption)
+        .map { header =>
+          blockchain
+            .getChainWeightByHash(header.hash)


Shouldn't we fetch here for the header.parentHash? Syncing to mordor fails if not due to ChainWeight for 1 not found when resolving branch

ntallar

LGTM! Syncing to mordor now works, played a bit around with the mocked miner and qa checkpoints and it all worked

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

rtkaczyk requested review from ntallar and pslaski November 2, 2020 11:23

pslaski suggested changes Nov 2, 2020

View reviewed changes

ntallar suggested changes Nov 2, 2020

View reviewed changes

rtkaczyk force-pushed the etcm-263-improve-blockimport-checkpoints branch from a914015 to 7dd7354 Compare November 3, 2020 16:41

pslaski suggested changes Nov 4, 2020

View reviewed changes

ntallar added the BREAKS STATE Affects self state retro-compatibility, needs data wiping label Nov 4, 2020

ntallar suggested changes Nov 4, 2020

View reviewed changes

rtkaczyk force-pushed the etcm-263-improve-blockimport-checkpoints branch from 7dd7354 to 5ef06b3 Compare November 5, 2020 16:35

ntallar suggested changes Nov 5, 2020

View reviewed changes

pslaski force-pushed the etcm-263-improve-blockimport-checkpoints branch from 5ef06b3 to ee341ce Compare November 6, 2020 11:04

pslaski approved these changes Nov 6, 2020

View reviewed changes

ntallar approved these changes Nov 6, 2020

View reviewed changes

[ETCM-263] improve BlockImport wrt checkpoints

6e722c8

pslaski force-pushed the etcm-263-improve-blockimport-checkpoints branch from ee341ce to 6e722c8 Compare November 6, 2020 12:52

pslaski merged commit d6a38d6 into develop Nov 6, 2020

pslaski deleted the etcm-263-improve-blockimport-checkpoints branch November 6, 2020 13:19

Conversation

rtkaczyk commented Nov 2, 2020

Description

Proposed Solution

Testing

Uh oh!

pslaski 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!

Choose a reason for hiding this comment

Uh oh!

rtkaczyk Nov 3, 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!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

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!

rtkaczyk Nov 3, 2020 •

edited

Loading