[ETCM-263] improve BlockImport wrt checkpoints - ChainWeight #769

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

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcher.scala

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

+      println("CODO KURWY???")


💯 ❤️ 💟

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

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

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


can be removed now

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

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

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

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

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


increaseTd => increaseTotalDifficulty

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

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

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


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

src/main/scala/io/iohk/ethereum/network/p2p/messages/CommonMessages.scala

+            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

src/main/scala/io/iohk/ethereum/network/p2p/messages/CommonMessages.scala

            RLPList(
              RLPList(
                block.header.toRLPEncodable,
                RLPList(block.body.transactionList.map(_.toRLPEncodable): _*),
                RLPList(block.body.uncleNodesList.map(_.toRLPEncodable): _*)
              ),
-              totalDifficulty,
-              latestCheckpointNumber
+              //TODO: should it be an RLPList? Should we define encoding in ChainWeight object?


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

src/main/scala/io/iohk/ethereum/utils/ByteUtils.scala

@@ -103,6 +103,9 @@ object ByteUtils {
    ByteString(compactPickledBytesToArray(buffer))
  }

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


Can you use it in other KeyValueStorages ?

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

src/test/resources/logback-test.xml

@@ -10,7 +10,7 @@

    <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

src/test/scala/io/iohk/ethereum/blockchain/sync/regular/RegularSyncSpec.scala

@@ -92,7 +92,10 @@ class RegularSyncSpec
        regularSync ! SyncProtocol.Start

        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

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

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

src/main/scala/io/iohk/ethereum/ledger/BlockExecution.scala

          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

src/main/scala/io/iohk/ethereum/ledger/BlockQueue.scala

src/main/scala/io/iohk/ethereum/ledger/BlockImport.scala

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

src/main/scala/io/iohk/ethereum/network/p2p/MessageDecoders.scala

@@ -43,6 +43,8 @@ object EthereumMessageDecoder extends MessageDecoder {
      //wire protocol
      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

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockImporter.scala

src/main/scala/io/iohk/ethereum/network/handshaker/EtcNodeStatusExchangeState.scala

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

src/main/scala/io/iohk/ethereum/blockchain/data/GenesisDataLoader.scala

@@ -118,7 +118,13 @@ class GenesisDataLoader(blockchain: Blockchain, blockchainConfig: BlockchainConf
      case None =>
        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

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

@@ -31,7 +29,7 @@ class BlockHeadersStorage(val dataSource: DataSource)
    blockHeader => compactPickledBytes(Pickle.intoBytes(blockHeader))

  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

src/main/scala/io/iohk/ethereum/ledger/BranchResolution.scala

-      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

src/main/scala/io/iohk/ethereum/network/p2p/messages/CommonMessages.scala

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


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

src/main/scala/io/iohk/ethereum/network/p2p/messages/CommonMessages.scala

-    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

src/main/scala/io/iohk/ethereum/ledger/BranchResolution.scala

-      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

src/test/scala/io/iohk/ethereum/blockchain/sync/regular/RegularSyncSpec.scala

src/test/scala/io/iohk/ethereum/db/storage/ChainWeightStorageSuite.scala

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

src/main/scala/io/iohk/ethereum/network/p2p/MessageDecoders.scala

@@ -43,6 +43,8 @@ object EthereumMessageDecoder extends MessageDecoder {
      //wire protocol
      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

src/main/scala/io/iohk/ethereum/network/p2p/messages/CommonMessages.scala

src/test/scala/io/iohk/ethereum/blockchain/sync/BlockBroadcastSpec.scala

ntallar

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

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

src/main/scala/io/iohk/ethereum/ledger/BranchResolution.scala

+        .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 requested 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 requested 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 requested 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

[ETCM-263] improve BlockImport wrt checkpoints - ChainWeight #769

[ETCM-263] improve BlockImport wrt checkpoints - ChainWeight #769

Conversation

rtkaczyk commented Nov 2, 2020

Description

Proposed Solution

Testing

pslaski 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

Choose a reason for hiding this comment

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

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

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 left a 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

ntallar left a comment

Choose a reason for hiding this comment

rtkaczyk Nov 3, 2020 •

edited

Loading