Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Sync Checker #13580

Merged
merged 9 commits into from
Feb 6, 2024
Merged

Add Sync Checker #13580

merged 9 commits into from
Feb 6, 2024

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Feb 3, 2024

What type of PR is this?

Cleanup

What does this PR do? Why is it needed?

To allow the blockchain service to be aware of the current node's sync status, the sync checker object is shared between both services to allow this status to be determined and disseminated across them.

Which issues(s) does this PR fix?

N.A

Other notes for review

@nisdas nisdas added the Ready For Review A pull request ready for code review label Feb 3, 2024
@nisdas nisdas requested a review from a team as a code owner February 3, 2024 00:50
Comment on lines +84 to +86
type SyncChecker struct {
Svc *Service
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checker having the full service as a dependency doesn't look right to me... It seems like the initial sync service already implements the SyncChecker interface, you can use it as the blockchain's service dependency. One service depending on another is more natural.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason you cant do it is because initial sync has a dependency on the blockchain service. It is the reason the checker has been implemented

potuz
potuz previously approved these changes Feb 5, 2024
Copy link
Contributor

@potuz potuz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am approving it but with the worry that if we are not setting and unsetting correctly the synced bool when we fall back or when we are actually synced to head, this may cause a lot of pain

Comment on lines 90 to 92
if s.Svc == nil {
return false
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a warning here? Seems like this should never be desired at runtime.

@nisdas nisdas added this pull request to the merge queue Feb 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 6, 2024
@nisdas nisdas enabled auto-merge February 6, 2024 02:07
@nisdas nisdas added this pull request to the merge queue Feb 6, 2024
Merged via the queue into develop with commit 6fa656c Feb 6, 2024
17 checks passed
@nisdas nisdas deleted the addSyncChecker branch February 6, 2024 02:40
rkapka pushed a commit that referenced this pull request Feb 6, 2024
* fix it

* add it in

* typo

* fix tests

* fix tests

* export and add test

* preston's review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants