Feature - State Sync manager #1067

ferranbt · 2022-12-21T18:19:20Z

Description

Introduced a concept of StateSyncManager, a struct that handles state sync saving workflow, and building and submitting commitments to the StateReceiver contract.

StateSyncManager is one of the building blocks in consensus_runtime, but it encapsulates the state sync logic mentioned above.

This makes testing easier (as we can mock the state sync workflow, in consensus_runtime tests, much more easily), makes code more granular and separates logic to concerning parts.

Changes include

Bugfix (non-breaking change that solves an issue)
Hotfix (change that solves an urgent issue, and requires immediate attention)
New feature (non-breaking change that adds functionality)
Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

I have assigned this PR to myself
I have added at least 1 reviewer
I have added the relevant labels
I have updated the official documentation
I have added sufficient documentation in code

Testing

I have tested this code with the official test suite
I have tested this code manually

consensus/polybft/state_sync_manager.go

consensus/polybft/consensus_runtime.go

consensus/polybft/state_sync_manager.go

vcastellm

I like it overall aside from the comments

codecov · 2022-12-23T09:46:16Z

Codecov Report

❗ No coverage uploaded for pull request base (feature/v3-parity@a65d2d0). Click here to learn what that means.
The diff coverage is n/a.

@@                 Coverage Diff                  @@
##             feature/v3-parity    #1067   +/-   ##
====================================================
  Coverage                     ?   54.23%           
====================================================
  Files                        ?      173           
  Lines                        ?    22990           
  Branches                     ?        0           
====================================================
  Hits                         ?    12469           
  Misses                       ?     9524           
  Partials                     ?      997

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Stefan-Ethernal · 2022-12-27T11:39:48Z

This function can be safely removed from consensusRuntime (used only in the unit test)

func validateVote(vote *MessageSignature, epoch *epochMetadata) error {

Stefan-Ethernal

Looking much better 🔥

consensus/polybft/state_sync_manager.go

consensus/polybft/consensus_runtime.go

vcastellm

stateSyncCommitmentSize can be moved to the manager

vcastellm

All functions that are not used in the state anymore and are used only in the manager, for example decodeStateSyncEvent, would be great to move them to the manager, so we isolate all statesync things in it.

consensus/polybft/state_sync_manager.go

vcastellm

LGTM aside from comment

consensus/polybft/consensus_runtime.go

ferranbt · 2022-12-29T07:46:38Z

consensus/polybft/state_sync_manager.go

+)
+
+// StateSyncManager is an interface that defines functions for state sync workflow
+type StateSyncManager interface {


I do not see the use case to do this abstraction here. The only reason would be to put it inside consensus_runtime but even that I think this is lightweight enough to not mock it and use it directly.

I agree, it's really lightweight to mock it. This interface is added only because of the nil implementation of stateSyncManager, which we discuss in comment: #1067 (comment)

ferranbt · 2022-12-29T07:48:37Z

consensus/polybft/consensus_runtime.go

+
+		c.stateSyncManager = stateSyncManager
+	} else {
+		c.stateSyncManager = &dummyStateSyncManager{}


If the stateSyncManager is not enabled because the bridge is not enabled, it should never be called. I understand this is a safety measure in case it is ever called. But, I would prefer if we ensure on tests that never happens instead of having extra code to ensure that.

Note that once we have hooks (if we ever do it though) we do not need this check anymore.

While I do understand your point, I must agree with Stefan here. Nullable pattern is a good practice, since it reduces the unnecessary checks and conditions in code, makes testing easier, and it doesn't really add any more complexity (since that nil or dummy implementation doesn't do anything). Plus, as you said, it keeps as out of harms way of nil exceptions. To me it makes sense to have it here, and if we ever implement that hook system, then I would agree, we can remove this nil implementation since we would not need it.

The other option would be to make StateSyncManager aware that he is not active and do nothing when PostEpoch and PostBlock get called.

Yes, but that means we will just be moving those checks to the stateSyncManager 😕 . That hook system is looking better by the minute 😄 .

ferranbt

Cannot Approve because I made the PR. LGTM except for the comments about the abstraction.

Stefan-Ethernal reviewed Dec 22, 2022

View reviewed changes

consensus/polybft/state_sync_manager.go Outdated Show resolved Hide resolved

ferranbt changed the title ~~initial work~~ Feature - State Sync manager Dec 22, 2022

goran-ethernal reviewed Dec 22, 2022

View reviewed changes

consensus/polybft/consensus_runtime.go Outdated Show resolved Hide resolved

ferranbt marked this pull request as ready for review December 22, 2022 12:37

vcastellm reviewed Dec 22, 2022

View reviewed changes

consensus/polybft/state_sync_manager.go Outdated Show resolved Hide resolved

consensus/polybft/state_sync_manager.go Outdated Show resolved Hide resolved

vcastellm reviewed Dec 22, 2022

View reviewed changes

goran-ethernal force-pushed the feature/state-sync-manager branch from 2e71bdd to 5032240 Compare December 26, 2022 14:06

goran-ethernal added the feature New update to Polygon Edge label Dec 26, 2022

goran-ethernal assigned ferranbt Dec 26, 2022

goran-ethernal requested review from Stefan-Ethernal, vcastellm and goran-ethernal December 26, 2022 15:44

Stefan-Ethernal reviewed Dec 27, 2022

View reviewed changes

goran-ethernal requested a review from Stefan-Ethernal December 27, 2022 14:44

vcastellm reviewed Dec 27, 2022

View reviewed changes

consensus/polybft/state_sync_manager.go Show resolved Hide resolved

goran-ethernal requested a review from vcastellm December 28, 2022 07:35

vcastellm approved these changes Dec 28, 2022

View reviewed changes

goran-ethernal self-assigned this Dec 28, 2022

Stefan-Ethernal reviewed Dec 28, 2022

View reviewed changes

consensus/polybft/consensus_runtime.go Outdated Show resolved Hide resolved

Stefan-Ethernal approved these changes Dec 28, 2022

View reviewed changes

goran-ethernal approved these changes Dec 28, 2022

View reviewed changes

goran-ethernal force-pushed the feature/state-sync-manager branch from 562b685 to e3243b3 Compare December 28, 2022 11:50

ferranbt commented Dec 29, 2022

View reviewed changes

ferranbt added 2 commits December 30, 2022 13:24

initial work

e82ed57

Rework on event tracker and tests

ee06892

ferranbt and others added 14 commits December 30, 2022 13:26

More stuff

8dd8c72

Migrate last test

ed3c60d

More changes

f259d9a

Remove unused test

5851ade

Use correct core-contracts version

e34bd68

Remove logs

460f59a

Use eventTracker from tracker package, code cleanup, bug fixes

34b6d70

Code cleanup

229a057

Code cleanup

e4f3f2c

Comments fix

810d7fc

Dummy state sync manager when bridge is not enabled

e5ad2bc

Comments fix

03f4356

Comments fix

215092a

Rebase fix

02d89b2

goran-ethernal force-pushed the feature/state-sync-manager branch from e3243b3 to 02d89b2 Compare December 30, 2022 12:33

goran-ethernal requested review from igorcrevar and Nemanja0x December 30, 2022 12:36

ferranbt merged commit 4c5d0d9 into feature/v3-parity Jan 3, 2023

vcastellm deleted the feature/state-sync-manager branch January 3, 2023 09:09

github-actions bot locked and limited conversation to collaborators Jan 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature - State Sync manager #1067

Feature - State Sync manager #1067

ferranbt commented Dec 21, 2022 •

edited by goran-ethernal

Loading

vcastellm left a comment

codecov bot commented Dec 23, 2022 •

edited

Loading

Stefan-Ethernal commented Dec 27, 2022

Stefan-Ethernal left a comment

vcastellm left a comment

vcastellm left a comment

vcastellm left a comment

ferranbt Dec 29, 2022

goran-ethernal Dec 29, 2022

ferranbt Dec 29, 2022

goran-ethernal Dec 29, 2022

ferranbt Dec 29, 2022

goran-ethernal Dec 30, 2022 •

edited

Loading

ferranbt left a comment

Feature - State Sync manager #1067

Feature - State Sync manager #1067

Conversation

ferranbt commented Dec 21, 2022 • edited by goran-ethernal Loading

Description

Changes include

Checklist

Testing

vcastellm left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 23, 2022 • edited Loading

Codecov Report

Stefan-Ethernal commented Dec 27, 2022

Stefan-Ethernal left a comment

Choose a reason for hiding this comment

vcastellm left a comment

Choose a reason for hiding this comment

vcastellm left a comment

Choose a reason for hiding this comment

vcastellm left a comment

Choose a reason for hiding this comment

ferranbt Dec 29, 2022

Choose a reason for hiding this comment

goran-ethernal Dec 29, 2022

Choose a reason for hiding this comment

ferranbt Dec 29, 2022

Choose a reason for hiding this comment

goran-ethernal Dec 29, 2022

Choose a reason for hiding this comment

ferranbt Dec 29, 2022

Choose a reason for hiding this comment

goran-ethernal Dec 30, 2022 • edited Loading

Choose a reason for hiding this comment

ferranbt left a comment

Choose a reason for hiding this comment

ferranbt commented Dec 21, 2022 •

edited by goran-ethernal

Loading

codecov bot commented Dec 23, 2022 •

edited

Loading

goran-ethernal Dec 30, 2022 •

edited

Loading