[ETCM-127] Regular sync integration tests by biandratti · Pull Request #740 · input-output-hk/mantis

biandratti · 2020-10-15T14:39:30Z

Description

Adds integration test suite for regular sync with 3 main scenarios on happy path i.e

synchornize with two peers which are at same best block and which have same chain
synchronize with two peers which are progressing forward in the same time
synchronize with two peers which ultimalty have divergent chains and node will be forced to resolve branches

Refactoring common behavior between regular and fast sync

lemastero · 2020-10-15T16:37:40Z

Commits should be signed using PGP. Take a look at: https://docs.github.com/en/free-pro-team@latest/github/authenticating-to-github/about-commit-signature-verification

BTW I have a problem with pushing to repo after switching to GPG, b/c I clone the project with HTTPS. I had to update remote to have:

git remote -v
origin	[email protected]:input-output-hk/mantis.git (fetch)
origin	[email protected]:input-output-hk/mantis.git (push)

lemastero · 2020-10-16T10:44:03Z

+      discoveryStatus = ServerStatus.NotListening
+    )
+
+  lazy val tempDir = Files.createTempDirectory("temp-regular-sync")


Temporary directories in some places are cleared like so:

def withDir(testCode: String => Any): Unit = { val path = Files.createTempDirectory("testdb").getFileName.toString try { testCode(path) } finally { val dir = new File(path) assert(!dir.exists() || dir.delete(), "File deletion failed") } }

i wonder if we should do the same here?

lemastero · 2020-10-17T19:23:16Z

After margin [ETCM-141] scalafmt . This PR can be updated by git merge ETCM-127-it-regularsync-fmt
Changes

KonradStaniec · 2020-10-19T12:01:47Z

+
+  object ValidatorsExecutorAlwaysSucceed extends ValidatorsExecutorAlwaysSucceed
+
+  class FakePeer(peerName: String, fakePeerCustomConfig: FakePeerCustomConfig)


to be hones regular sync FakePeer looks almost the same as FastSyncItSpecUtils.FakePeer (except some regular sync specific clasess) maybe we could have only one FakePeer in some kinda SyncItSpecUtils and in regular sync tests start it by startRegularSync and in fast sync tests start it by startFastSync ?

Add new abstract class called “CommonFakePeer”

KonradStaniec · 2020-10-19T12:06:15Z

+        _ <- peer2.waitForRegularSyncLoadLastBlock(blockNumer + 3)
+      } yield {
+        assert(peer1.bl.getBestBlock().number == peer2.bl.getBestBlock().number)
+        (peer1.bl.getBlockByNumber(blockNumer + 1), peer1.bl.getBlockByNumber(blockNumer + 1)) match {


maybe it would be more clear to check assert(peer1.bt.getTotalDifficultyByHash(bestBlock.hash) == peer2.bt.getTotalDifficultyByHash(bestBlock.hash)) ? i.e that peers total difficulties are the same after fork resolution

KonradStaniec · 2020-10-19T12:10:33Z

+        _ <- peer2.importBlocksUntil(blockNumer)(IdentityUpdate)
+        _ <- peer1.connectToPeers(Set(peer2.node))
+        _ <- peer1.startRegularSync().delayExecution(500.milliseconds)
+        _ <- peer2.mineNewBlock()(IdentityUpdate).delayExecution(50.milliseconds)


maybe we could mine several blocks (like 10-15) ? (it will check if if all of them are properly queued and handled on syncer par)

I added the next line, where mine 10 new blocks.
_ <- peer2.mineNewBlocks(100.milliseconds, 10)(IdentityUpdate)

…e branches

KonradStaniec

one last minor comment and lgtm

KonradStaniec · 2020-10-21T06:50:45Z

+
+import scala.concurrent.duration._
+
+class RegularSyncItSpec extends FlatSpecBase with Matchers with BeforeAndAfter {


can we change BeforeAndAfter to BeforeAndAfterAll and then add :

override def afterAll(): Unit = { testScheduler.shutdown() testScheduler.awaitTermination(60.second) }

? ( we should probably do this also in FastSyncItSpec)

KonradStaniec

One last comment. Also for the future, all commits titles should start with ticket number like
[ETCM-XXX] commit message

KonradStaniec · 2020-10-22T13:15:13Z

    )
    .settings(executableScriptName := name.value)
-    .settings(inConfig(Integration)(Defaults.testSettings :+ (Test / parallelExecution := false)): _*)
+    .settings(inConfig(Integration)(Defaults.testSettings


Maybe we could add formatting to all subprojects ?

biandratti requested review from KonradStaniec, lemastero, mirkoAlic and ntallar October 15, 2020 14:39

lemastero reviewed Oct 15, 2020

View reviewed changes

Comment thread src/it/scala/io/iohk/ethereum/sync/FastSyncItSpec.scala Outdated

biandratti removed request for KonradStaniec, mirkoAlic and ntallar October 15, 2020 18:37

biandratti force-pushed the ETCM-127-it-regularsync branch 2 times, most recently from ba0ceb2 to ecfd875 Compare October 15, 2020 19:22

biandratti requested review from KonradStaniec, mirkoAlic and ntallar October 15, 2020 21:36

lemastero reviewed Oct 16, 2020

View reviewed changes

Comment thread src/it/scala/io/iohk/ethereum/sync/util/SyncUtils.scala Outdated

lemastero reviewed Oct 16, 2020

View reviewed changes

lemastero suggested changes Oct 16, 2020

View reviewed changes

Comment thread src/it/scala/io/iohk/ethereum/sync/util/FakePeerFastSync.scala Outdated

KonradStaniec reviewed Oct 19, 2020

View reviewed changes

biandratti removed request for mirkoAlic and ntallar October 19, 2020 19:38

Maximiliano Biandratti and others added 9 commits October 20, 2020 09:57

regular sync - it - should update target block

267eb60

add it - should sync blockchain progressing forward in the same time

f9a4a25

it - should sync peers with divergent chains will be forced to resolv…

21aeff3

…e branches

fix it with divergent chains

8d50af8

refactor it - common behavior regular and fast sync

fa55c1e

style refactor

bd5379b

fix style

03585e5

unify behavior between regular and fast sync

be59c73

unify fast and sync - change it regular sync assert

a1eddfa

biandratti force-pushed the ETCM-127-it-regularsync branch from 841b841 to a1eddfa Compare October 20, 2020 13:23

lemastero reviewed Oct 20, 2020

View reviewed changes

Comment thread src/it/scala/io/iohk/ethereum/sync/RegularSyncItSpec.scala Outdated

lemastero reviewed Oct 20, 2020

View reviewed changes

Comment thread src/it/scala/io/iohk/ethereum/sync/util/SyncCommonItSpecUtils.scala

KonradStaniec reviewed Oct 21, 2020

View reviewed changes

biandratti added 3 commits October 21, 2020 20:42

change test descriptions

1e5499d

merge with develop

a907623

add it scalafmt

c63ef60

KonradStaniec approved these changes Oct 22, 2020

View reviewed changes

lemastero approved these changes Oct 22, 2020

View reviewed changes

Merge branch 'develop' into ETCM-127-it-regularsync

9dfb637

biandratti merged commit 61fbcac into develop Oct 22, 2020


		object ValidatorsExecutorAlwaysSucceed extends ValidatorsExecutorAlwaysSucceed

		class FakePeer(peerName: String, fakePeerCustomConfig: FakePeerCustomConfig)


		import scala.concurrent.duration._

		class RegularSyncItSpec extends FlatSpecBase with Matchers with BeforeAndAfter {

Conversation

biandratti commented Oct 15, 2020

Description

Uh oh!

Uh oh!

lemastero commented Oct 15, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lemastero commented Oct 17, 2020

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

KonradStaniec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants