Etcm 175 sync reporting issues by kapke · Pull Request #735 · input-output-hk/mantis

kapke · 2020-10-14T12:17:26Z

Description

Update eth_syncing method to standarized output (https://playground.open-rpc.org/?schemaUrl=https://raw.githubusercontent.com/etclabscore/ethereum-json-rpc-specification/master/openrpc.json&uiSchema%5BappBar%5D%5Bui:input%5D=false)) and move sync progress reporting into sync module.

Important Changes Introduced

A SyncProtocol is used now for high-level communication between sync controller and specific implementations as well as outside sync controller. Probably it makes sense to further extend that change and hide it behind a well-typed task-returning interfaces.

State breakage is related to changed datatype for tracking state nodes progress - from Int to Long (as in sync state scheduler)

Testing

All automatic tests should pass (most notably unit and it ones)
When sync is actually happening (data is being actively fetched from network) - eth_syncing should return full response, including information about state nodes.

KonradStaniec · 2020-10-16T11:18:35Z

  ): Receive = {
+    case ReportStats =>
+      syncInitiator ! StateSyncStats(
+        currentStats.saved + currentState.memBatch.size,


again question of granularity, as if restart happen and membatch won't be persisted then user may end up with lesser number of nodes reporter after restart. If someone is paranoic enought it can mean for him that some data was lost (i kinda already see QA reporting it that some nodes were lost while testing restarting :D )

I see your point. Let me test that as I'm not 100% sure whether that's the wanted behavior

lemastero · 2020-10-17T19:12:21Z

After margin [ETCM-141] scalafmt . This PR can be updated by git merge etcm-175-sync-reporting-issues-fmt2

Changes:
+1296 -540
+1172 -477

mmrozek · 2020-10-20T11:15:41Z

      ec: ExecutionContext
  ): Future[(List[Block], Option[Any])] =
    if (blocks.isEmpty) {
+      supervisor ! ProgressProtocol.ImportedBlock(importedBlocks.map(_.number).max)


It could be importedBlocks.headOption.map(_.number).getOrElse(0). In the case when we ex. try to import 1 block and it will be a duplicate importedBlocks.map(_.number).max will return an exception

mmrozek · 2020-10-20T11:20:43Z

-            highestBlock = highestBlock
+  def syncing(req: SyncingRequest): ServiceResponse[SyncingResponse] =
+    syncingController
+      .ask(SyncProtocol.GetStatus)(getTransactionFromPoolTimeout)


Why getTransactionFromPoolTimeout ?

mmrozek · 2020-10-20T11:25:01Z

-    peers-to-choose-pivot-block-margin = 1
-    pivot-block-offset = 500
+    min-peers-to-choose-target-block = 2
+    target-block-offset = 500


Why did you make this change? I think it is incorrect. min-peers-to-choose-target-block and target-block-offset are not used anymore

Wrong resolution after merging in changes in pivot block selection. Fixed

mmrozek · 2020-10-20T11:26:32Z

    val defaultState =
-      SyncState(defaultpivotBlockHeader, defaultSafeDownloadTarget, bestBlockHeaderNumber = defaultBestBlock)
+      SyncState(
+        defaultpivotBlockHeader,


Very minor: it should be defaultPivotBlockHeader

KonradStaniec · 2020-10-28T09:12:38Z

  it should "sync state to different tries when peer provide mixed responses" in new TestSetup() {
    forAll(ObjectGenerators.genMultipleNodeData(1000)) { nodeData =>
      val initiator = TestProbe()
+      initiator.ignoreMsg { case SyncStateSchedulerActor.StateSyncStats(_, _) => true }


this need to be added also in last test start state sync when receiving start signal while bloom filter is loading

mmrozek

LGTM!

[ETCM-175][WIP] Introduce sync protocol, provide (not working yet :() tests for both regular sync and fast sync, provide initial implementation of reporting sync progress through SyncController [ETCM-175] Make RegularSync reporting progress properly [ETCM-175] Fix most of sync reporting tests [ETCM-175] Make all tests pass 🎇 [ETCM-175] Switch eth_syncing to use SyncProtocol now [ETCM-175] Cleanup [ETCM-175] Update state sync progress with each response received [ETCM-175] Rename best known block in regular sync progress state [ETCM-175] Filer out empty state progress in fast sync [ETCM-175] Return initial incomplete state on fast sync [ETCM-175] Introduce config for ask timeout

kapke requested review from KonradStaniec and mmrozek October 14, 2020 12:17

kapke self-assigned this Oct 14, 2020

kapke added the BREAKS STATE Affects self state retro-compatibility, needs data wiping label Oct 14, 2020

KonradStaniec reviewed Oct 16, 2020

View reviewed changes

kapke force-pushed the etcm-175-sync-reporting-issues branch 2 times, most recently from 2691c6c to 3b81587 Compare October 16, 2020 17:00

mmrozek reviewed Oct 20, 2020

View reviewed changes

kapke force-pushed the etcm-175-sync-reporting-issues branch 2 times, most recently from 631d3a1 to 443ea00 Compare October 27, 2020 15:11

KonradStaniec approved these changes Oct 28, 2020

View reviewed changes

KonradStaniec reviewed Oct 28, 2020

View reviewed changes

kapke force-pushed the etcm-175-sync-reporting-issues branch from 443ea00 to e9f1032 Compare October 28, 2020 09:46

mmrozek approved these changes Oct 28, 2020

View reviewed changes

kapke force-pushed the etcm-175-sync-reporting-issues branch 5 times, most recently from b57793f to dde2b38 Compare October 28, 2020 16:11

kapke force-pushed the etcm-175-sync-reporting-issues branch from dde2b38 to 6a67888 Compare October 29, 2020 08:20

kapke merged commit c340728 into develop Oct 29, 2020

kapke deleted the etcm-175-sync-reporting-issues branch October 29, 2020 08:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Etcm 175 sync reporting issues#735

Etcm 175 sync reporting issues#735
kapke merged 1 commit intodevelopfrom
etcm-175-sync-reporting-issues

kapke commented Oct 14, 2020 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KonradStaniec Oct 16, 2020

Uh oh!

kapke Oct 16, 2020

Uh oh!

lemastero commented Oct 17, 2020 •

edited

Loading

Uh oh!

mmrozek Oct 20, 2020 •

edited

Loading

Uh oh!

mmrozek Oct 20, 2020

Uh oh!

mmrozek Oct 20, 2020

Uh oh!

kapke Oct 21, 2020

Uh oh!

mmrozek Oct 20, 2020

Uh oh!

KonradStaniec Oct 28, 2020

Uh oh!

mmrozek left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

kapke commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Important Changes Introduced

Testing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KonradStaniec Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

kapke Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

lemastero commented Oct 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmrozek Oct 20, 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!

mmrozek Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

mmrozek Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

kapke Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

mmrozek Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

KonradStaniec Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

mmrozek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kapke commented Oct 14, 2020 •

edited

Loading

lemastero commented Oct 17, 2020 •

edited

Loading

mmrozek Oct 20, 2020 •

edited

Loading