[ETCM-77] checkpoint syncing #728

rtkaczyk · 2020-10-09T12:59:42Z

Description

Handling new checkpoint messages to RegularSync/BlockImporter
Checkpoint block generation
Branch comparison logic in Ledger/BranchResolution.

Relevant unit tests added.

pslaski

Can you port CheckpointBlockGeneratorSpec too ?

pslaski · 2020-10-14T11:10:02Z

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

+    case nc @ NewCheckpoint(parentHash, signatures) =>
+      if (state.importing) {
+        //TODO: is this ok? What delay?
+        context.system.scheduler.scheduleOnce(100.millis, self, nc)


IMHO we can treat it like other block types and don't try to re-import it.

What if all the federation nodes happen to be importing other blocks when a new checkpoint appears? Wouldn't it be lost forever?

assuming that we know OBFT nodes and federation nodes, they shouldn't start producing checkpoints before being on top of the chain, so I'm ok with your restart logic. The only thing worth to consider is higher restart time, maybe something ~1 s, to give to node more time to re-sync

pslaski · 2020-10-14T11:13:17Z

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

+  import ImportMessages._
+  override def preImport(): LogEntry = (DebugLevel, s"Importing new checkpoint block (${block.idTag})")
+  override def importedToTheTop(): LogEntry =
+    (DebugLevel, s"Added new checkpont block $number to top of the chain")


typo -> checkpont

pslaski · 2020-10-14T11:19:27Z

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

+
+  def generate(parent: Block, checkpoint: Checkpoint): Block = {
+    val blockNumber = parent.number + 1
+    // we are using a predictable value for timestamp so that each federation node generates identical block


you can add information about ETCM-173 here

pslaski · 2020-10-14T11:20:01Z

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

+    val header = BlockHeader(
+      parentHash = parent.hash,
+      ommersHash = BlockHeader.EmptyOmmers,
+      beneficiary = Address(0).bytes,


BlockHeader.EmptyBeneficiary

pslaski · 2020-10-14T11:47:05Z

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

  }

-  private def isBetterBranch(block: Block, bestBlock: Block, newTd: BigInt, currentTd: BigInt): Boolean =
-    newTd > currentTd || (blockchainConfig.gasTieBreaker && newTd == currentTd && block.header.gasUsed > bestBlock.header.gasUsed)
+  private def isBetterBranch(block: Block, bestBlock: Block, newTd: BigInt, currentTd: BigInt): Boolean = {


why do we need this ? what about BranchResolution.compareByCheckpoints / compareByDifficulty ?

Well, that's what it is. Unlike in the reference implementation, we have the branch comparison logic in 2 places. I'm a bit hesitant to changing anything with this regard, as syncing on the whole could use a bit of revamping

pslaski · 2020-10-14T11:50:14Z

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

+        val blockPromise: Promise[BlockImportResult] = Promise()
+        ledger.setImportResult(block, () => blockPromise.future)
+
+        // FIXME: Why the hell is this needed???


ntallar

Due to the core changes, have you done any manual tests with mainnet or mordor?

ntallar · 2020-10-16T16:54:48Z

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

+import io.iohk.ethereum.domain._
+import io.iohk.ethereum.ledger.BloomFilter
+
+class CheckpointBlockGenerator(


Isn't this duplicated with the CheckpointingTestHelpers?

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

ntallar · 2020-10-16T16:59:56Z

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

+      blockchainConfig.gasTieBreaker && newTd == currentTd && block.header.gasUsed > bestBlock.header.gasUsed
+
+    (block.hasCheckpoint, bestBlock.hasCheckpoint) match {
+      case (true, true) => false


Shouldn't we untie here by betterTd || tieBreaker? For the case that we are comparing checkpoint of block 5 and checkpoint of block 10 for example

But this way we are opening the door to reverting checkpoints. I'd rather not do that until we have a clear plan on how to deal with that.

Hmm and probably it can never be the case that we are comparing blocks from the same branch? That is C5 and C10 from B1<-...<-C5<-...<-C10

Should we at least add a comment then?

I also find it odd that we are allowing reverting checkpoints in all our codebase (i.e. blockchain object and https://github.com/input-output-hk/mantis/pull/728/files#diff-be955b26fb84ed57e0ea600e45c78fb94d45dad720746529236701d67b14e843R49) but here

True, there is a door for a revert. What if we changed this expression to something like:

newCheckpoint.number > oldCheckpoint.number && newBranch.contains(oldCheckpoint)

and compare checkpoints by hashes (rather than by numbers) a few lines above.

WDYT?

Sounds good to me

However wasn't a rule like this what originally caused the issue on our testnet some time ago? Was that fixed on the OBFT side?

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

rtkaczyk · 2020-10-20T11:56:18Z

SyncControllerSpec is failing non-deterministically. Waiting for #742 which fixes the issue (cc @KonradStaniec )

Edit: Fixed!

ntallar

Last comments and LGTM!

ntallar · 2020-10-20T11:48:00Z

src/test/scala/io/iohk/ethereum/consensus/validators/BlockValidatorSpec.scala

@@ -170,10 +171,12 @@ class BlockValidatorSpec extends AnyFlatSpec with Matchers with SecureRandomBuil
  val validCheckpoint = Checkpoint(CheckpointingTestHelpers.createCheckpointSignatures(keys, validBlockHeader.hash))

  val validBlockHeaderWithCheckpoint =
-    CheckpointingTestHelpers.createBlockHeaderWithCheckpoint(


It seems we are still using the CheckpointingTestHelpers.createBlockHeaderWithCheckpoint on the BlockchainSpec, should we remove it from there as well?

ntallar · 2020-10-20T14:55:51Z

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

+    lazy val tieBreaker =
+      blockchainConfig.gasTieBreaker && newTd == currentTd && block.header.gasUsed > bestBlock.header.gasUsed
+
+    (block.hasCheckpoint, bestBlock.hasCheckpoint) match {


Shouldn't we check for the presence of checkpoints in the both branches and not just the top block of them?

Maybe there's something that I'm missing here, this component is difficult to follow

You are right! Let's consider 2 branches:

B0 -> C -> B1

B0 -> B2 -> B3

If we're considering B3 for import and it has a greater TD than B1, then checkpoint C would be reverted by a regular block.

This simple method cannot be adapted to handling checkpoints. What I think we could do is obtain the branch for the newly enqueued block (BlockQueue#getBranch: here) and then compare it using BranchResolution#resolveBranch, which as a bonus would DRY the code.

WDYT?

It would be great to remove this code duplication, my only comments are:

I feel like we are doing something unnecesary by having both the importing and resolving process do this sort of comparisons. However at this stage is probably something we'll have to live with

For comparison shouldn't we use BranchResolution#compareByCheckpoints instead? There a several thing surrounding BranchResolution#resolveBranch that are maybe not necessary

But the above are just technical questions, totally agree with re-using code here!

However at this stage is probably something we'll have to live with

The only way I see around is to take advantage of the improvements in our testnet

There a several thing surrounding BranchResolution#resolveBranch that are maybe not necessary

Not that much really:

checking the parent

checking that headers from queue form a chain

Avoiding that shouldn't be worth refactoring

The only way I see around is to take advantage of the improvements in our testnet

What improvements do you mean?

Btw yesterday I thought of an alternative to your idea, which is closer to what is currently in place on the BlockQueue; if our leaves would be as Leaf(hash: ByteString, totalDifficulty: BigInt, lastCheckpointBlockNumber) can't this function be adapted to compare those lastCheckpointBlockNumber? Obviously, if we decide that mantis doesn't allow reverts then that information is not enough... but maybe we could rely on OBFT being the one that doesn't produce forked checkpoints.

I'm always in favour of reducing replicated codebase but your proposed change probably should be associated with more refactoring (for example, is there any purpose for tracking total difficulty of the leaves if it's not used here?) and maybe impact to the performance (just guessing, it might not be the case).

Wdyt?

ntallar · 2020-10-20T15:25:25Z

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

+      blockchainConfig.gasTieBreaker && newTd == currentTd && block.header.gasUsed > bestBlock.header.gasUsed
+
+    (block.hasCheckpoint, bestBlock.hasCheckpoint) match {
+      case (true, true) => false


Hmm and probably it can never be the case that we are comparing blocks from the same branch? That is C5 and C10 from B1<-...<-C5<-...<-C10

Should we at least add a comment then?

I also find it odd that we are allowing reverting checkpoints in all our codebase (i.e. blockchain object and https://github.com/input-output-hk/mantis/pull/728/files#diff-be955b26fb84ed57e0ea600e45c78fb94d45dad720746529236701d67b14e843R49) but here

ntallar

As this PR is blocking #729, #738 and the creation of new PRs for other issues, we agreed on merging it as is, further comments will be addressed on a new PR.

Only syncing with checkpointing will remain buggy, but it won't be used on the short term

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

rtkaczyk mentioned this pull request Oct 9, 2020

[ETCM-76] network messages for checkpointing #729

Merged

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

rtkaczyk force-pushed the etcm-77-checkpoint-syncing branch from 5da21d9 to a922376 Compare October 9, 2020 14:23

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

Base automatically changed from etcm-75-blockchain-support-checkpoint to develop October 13, 2020 14:07

rtkaczyk force-pushed the etcm-77-checkpoint-syncing branch 2 times, most recently from 4cbeed0 to fbd1980 Compare October 14, 2020 08:05

rtkaczyk marked this pull request as ready for review October 14, 2020 08:06

rtkaczyk requested review from ntallar and pslaski October 14, 2020 08:35

pslaski suggested changes Oct 14, 2020

View reviewed changes

rtkaczyk force-pushed the etcm-77-checkpoint-syncing branch from fbd1980 to b59b3af Compare October 15, 2020 16:10

pslaski mentioned this pull request Oct 16, 2020

[ETCM-174] Sorting of checkpoint signatures #738

Merged

ntallar requested changes Oct 16, 2020

View reviewed changes

rtkaczyk force-pushed the etcm-77-checkpoint-syncing branch 2 times, most recently from e7200e4 to 414f1cc Compare October 19, 2020 20:33

pslaski approved these changes Oct 20, 2020

View reviewed changes

rtkaczyk force-pushed the etcm-77-checkpoint-syncing branch from 414f1cc to 70672f2 Compare October 20, 2020 09:16

[ETCM-77] checkpoint sync

f0416e0

rtkaczyk force-pushed the etcm-77-checkpoint-syncing branch from 70672f2 to f0416e0 Compare October 20, 2020 13:25

ntallar requested changes Oct 20, 2020

View reviewed changes

ntallar approved these changes Oct 21, 2020

View reviewed changes

Nicolás Tallar added 2 commits October 21, 2020 12:51

Merge branch 'develop' into etcm-77-checkpoint-syncing

dffb884

Merge branch 'develop' into etcm-77-checkpoint-syncing

274e14e

ntallar merged commit 18c25a4 into develop Oct 21, 2020

ntallar deleted the etcm-77-checkpoint-syncing branch October 21, 2020 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ETCM-77] checkpoint syncing #728

[ETCM-77] checkpoint syncing #728

rtkaczyk commented Oct 9, 2020 •

edited

Loading

pslaski left a comment

pslaski Oct 14, 2020

rtkaczyk Oct 15, 2020

pslaski Oct 16, 2020

pslaski Oct 14, 2020

pslaski Oct 14, 2020

pslaski Oct 14, 2020

pslaski Oct 14, 2020

rtkaczyk Oct 15, 2020

pslaski Oct 14, 2020

ntallar left a comment

ntallar Oct 16, 2020

ntallar Oct 16, 2020

rtkaczyk Oct 19, 2020

ntallar Oct 20, 2020

rtkaczyk Oct 20, 2020

ntallar Oct 20, 2020

rtkaczyk commented Oct 20, 2020 •

edited

Loading

ntallar left a comment

ntallar Oct 20, 2020

ntallar Oct 20, 2020

rtkaczyk Oct 20, 2020 •

edited

Loading

ntallar Oct 20, 2020 •

edited

Loading

rtkaczyk Oct 21, 2020

ntallar Oct 21, 2020 •

edited

Loading

ntallar Oct 20, 2020

ntallar left a comment •

edited

Loading

[ETCM-77] checkpoint syncing #728

[ETCM-77] checkpoint syncing #728

Conversation

rtkaczyk commented Oct 9, 2020 • edited Loading

Description

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

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

rtkaczyk commented Oct 20, 2020 • edited Loading

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

rtkaczyk Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

ntallar Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntallar Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntallar left a comment • edited Loading

Choose a reason for hiding this comment

rtkaczyk commented Oct 9, 2020 •

edited

Loading

rtkaczyk commented Oct 20, 2020 •

edited

Loading

rtkaczyk Oct 20, 2020 •

edited

Loading

ntallar Oct 20, 2020 •

edited

Loading

ntallar Oct 21, 2020 •

edited

Loading

ntallar left a comment •

edited

Loading