[ETCM-75] Support for checkpoint blocks in Blockchain #716

pslaski · 2020-10-01T13:44:18Z

Description

added saving last checkpoint number during block save and rollback

Important Changes Introduced

saving last checkpoint number to the memory and persisting it with best block number after cache persist during block save
persisting last checkpoint number and best block number immediately during block remove

Testing

unit tests

rtkaczyk · 2020-10-01T15:15:49Z

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

    else
      bestBlockNum
  }

+  override def getLatestCheckpointBlockNumber(): BigInt = {
+    val latestCheckpointNumberInStorage = appStateStorage.getLatestCheckpointBlockNumber()
+    if (bestKnownBlockAndLatestCheckpoint.get().latestCheckpointNumber > latestCheckpointNumberInStorage)


Could you explain in what situation these numbers can be different?

I'm not sure every border case here, but as far as I understand the best block number is firstly saved in memory and then persisted to the storage only when it's time to persist cache. I want to save the best block number and latest checkpoint transactionally so getLatestCheckpointBlockNumber behaves the same. We can have the latest checkpoint number in memory bigger than the number in storage because the cache wasn't persisted yet.

Ok, maybe add a comment about it?

rtkaczyk · 2020-10-02T11:56:21Z

src/test/scala/io/iohk/ethereum/domain/BlockchainSpec.scala

@@ -11,6 +12,9 @@ import org.scalatest.matchers.should.Matchers

 class BlockchainSpec extends AnyFlatSpec with Matchers {

+  val checkpoint = ObjectGenerators.fakeCheckpointGen(2, 5).sample.get
+  //val keys = Seq(generateKeyPair(secureRandom))


Minor: remove

rtkaczyk

Apart from minor issues LGTM!

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

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

KonradStaniec · 2020-10-08T06:13:37Z

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

+          prev.map { num =>
+            // side effect
+            saveLatestCheckpointNumber(num)
+            appStateStorage.putLatestCheckpointBlockNumber(num)


I am not sure about this bit.
Lets say we have saved X blocks with all the mpt nodes i.e best checkpoint and best known block is X. (for simplicity checkpoint and bestblock are the same)
Now we are on X + 20 (without updating bestblock and checkpoint in db) , last checkpoint was on X + 10. We start to rollback and and we find this checkpint an we save into db. Now node dies.
After restart, best checkpoint in db will be X + 10 but best block will be X as this was last time we sved all data.
Imo if we must to update best checkpoint in db, we should also update best block.

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

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

ntallar · 2020-10-08T19:32:54Z

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

      .commit()

    // not transactional part
+    saveBestKnownBlocks(newBestBlockNumber, prevCheckpointNumber)


Should we also add this to our save function as well?

in save is different logic which controls what blocks data will be saved, so it's better to use low-level methods there

And much more error prone... I think we'll suffer from this eventually 😕

ntallar · 2020-10-08T19:39:10Z

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

+    val checkpointNumber = oldBranch
+      .filter(_.block.hasCheckpoint)
+      .sortBy(_.block.number)
+      .lastOption


Minor: instead of sortBy and lastOption, should we maybe use max or maxBy?

that was my first option, but maxBy/max are always returning value - in this case, we could have an empty list.

Yeah, that's a pain of usage, but maybe we could have done Try(oldBranch.collect { case b if b.block.hasCheckpoint => b.block.number }).max).toOption?

IMHO your proposition is less readable, but it's probably doing one less iteration (filter, sortBy, lastOption vs collect, max). WDYT about .filter(_.block.hasCheckpoint).sortBy(-_.block.number).headOption ?

What about https://typelevel.org/cats/api/cats/Foldable.html#maximumByOption[A,B](fa:F[A])(f:A=%3EB)(implicitevidence$3:cats.Order[B]):Option[A] ?

ntallar

LGTM!

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

pslaski requested review from rtkaczyk, KonradStaniec and ntallar October 1, 2020 13:44

pslaski force-pushed the etcm-75-blockchain-support-checkpoint branch from 6ed0963 to c40bc7b Compare October 1, 2020 14:21

rtkaczyk reviewed Oct 1, 2020

View reviewed changes

pslaski force-pushed the etcm-75-blockchain-support-checkpoint branch from c40bc7b to 1529908 Compare October 2, 2020 11:36

rtkaczyk reviewed Oct 2, 2020

View reviewed changes

rtkaczyk approved these changes Oct 2, 2020

View reviewed changes

ntallar requested changes Oct 5, 2020

View reviewed changes

src/main/scala/io/iohk/ethereum/db/storage/StateStorage.scala Outdated Show resolved Hide resolved

src/main/scala/io/iohk/ethereum/domain/Blockchain.scala Outdated Show resolved Hide resolved

KonradStaniec reviewed Oct 8, 2020

View reviewed changes

pslaski force-pushed the etcm-75-blockchain-support-checkpoint branch 3 times, most recently from 67348f8 to a9c3dea Compare October 8, 2020 16:44

ntallar requested changes Oct 8, 2020

View reviewed changes

pslaski force-pushed the etcm-75-blockchain-support-checkpoint branch from a9c3dea to 643d0c9 Compare October 9, 2020 12:56

rtkaczyk mentioned this pull request Oct 9, 2020

[ETCM-77] checkpoint syncing #728

Merged

pslaski force-pushed the etcm-75-blockchain-support-checkpoint branch from 643d0c9 to a2edd59 Compare October 9, 2020 14:18

ntallar approved these changes Oct 9, 2020

View reviewed changes

pslaski force-pushed the etcm-75-blockchain-support-checkpoint branch 2 times, most recently from 9af95a2 to 913a042 Compare October 13, 2020 11:10

etcm-75 added support for saving latest checkpoint number in blockchain

28fa411

pslaski force-pushed the etcm-75-blockchain-support-checkpoint branch from 913a042 to 28fa411 Compare October 13, 2020 11:14

KonradStaniec approved these changes Oct 13, 2020

View reviewed changes

pslaski merged commit dd6d8e0 into develop Oct 13, 2020

pslaski deleted the etcm-75-blockchain-support-checkpoint branch October 13, 2020 14:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ETCM-75] Support for checkpoint blocks in Blockchain #716

[ETCM-75] Support for checkpoint blocks in Blockchain #716

pslaski commented Oct 1, 2020 •

edited

Loading

rtkaczyk Oct 1, 2020 •

edited

Loading

pslaski Oct 2, 2020

rtkaczyk Oct 2, 2020

rtkaczyk Oct 2, 2020

rtkaczyk left a comment

KonradStaniec Oct 8, 2020

ntallar Oct 8, 2020

pslaski Oct 9, 2020

ntallar Oct 9, 2020

ntallar Oct 8, 2020

pslaski Oct 9, 2020

ntallar Oct 9, 2020

pslaski Oct 12, 2020

kapke Oct 13, 2020

ntallar left a comment

[ETCM-75] Support for checkpoint blocks in Blockchain #716

[ETCM-75] Support for checkpoint blocks in Blockchain #716

Conversation

pslaski commented Oct 1, 2020 • edited Loading

Description

Important Changes Introduced

Testing

rtkaczyk Oct 1, 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

rtkaczyk 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

ntallar left a comment

Choose a reason for hiding this comment

pslaski commented Oct 1, 2020 •

edited

Loading

rtkaczyk Oct 1, 2020 •

edited

Loading