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

Fix /eth/v1/node/syncing #3720

Merged
merged 2 commits into from
Jun 14, 2022
Merged

Fix /eth/v1/node/syncing #3720

merged 2 commits into from
Jun 14, 2022

Conversation

cheatfate
Copy link
Contributor

Fix REST server /eth/v1/node/syncing call to return proper values even if SyncManager is not running.

distance = wallSlot - headSlot
info = RestSyncInfo(
head_slot: headSlot, sync_distance: distance,
is_syncing: distance != 0'u64
Copy link
Member

Choose a reason for hiding this comment

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

this is too strict, ie it'll show up as syncing at the start of every slot, in the period between the start of the slot and until we've received a block. why is syncmanager itself not giving a correct value here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because there so many different type of sync, that syncmanager is not always running.

Copy link
Member

Choose a reason for hiding this comment

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

This call should be reporting the progress of the ordinary forward sync that we use to chase head after the node goes offline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Requests the beacon node to describe if it's currently syncing or not, and if it is, what block it is up to.

There is no mention on type of sync.
Also people are using this function to check progress of checkpoint sync.

Copy link
Member

Choose a reason for hiding this comment

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

I also think the expected definition of "synced" is "able to attest for the current head" (in other words, how far is our current head from the wall clock head). For the other types of syncing, we probably need separate API calls.

@github-actions
Copy link

github-actions bot commented Jun 8, 2022

Unit Test Results

     12 files  ±0     842 suites  ±0   58m 2s ⏱️ + 1m 3s
1 699 tests ±0  1 647 ✔️ ±0    52 💤 ±0  0 ±0 
9 893 runs  ±0  9 765 ✔️ ±0  128 💤 ±0  0 ±0 

Results for commit 711b479. ± Comparison against base commit 7ec1521.

@arnetheduck arnetheduck merged commit 1b6651d into unstable Jun 14, 2022
@arnetheduck arnetheduck deleted the fix-rest-syncing branch June 14, 2022 20:26
etan-status added a commit that referenced this pull request Jun 14, 2022
…/dag-statevars

* commit '6bf7d6468036c3f010c6ce08727d63331ada5e0d':
  update CI timeouts (#3751)
  Fix /eth/v1/node/syncing (#3720)
  use block ID vs full block in LC data caching (#3741)
  cleanup LC data helpers (#3746)
  fix LC data import for Altair fork period (#3744)
  fix CI archive step (#3752)
  Support displaying the version number in the status bar; Implements #2959 (#3747)
  ci: install cmake on windows in daily job
  don't use slow-to-compress bzip2 with larger Jenkins artifacts
  rename light client config parameters (#3740)
  error handling for local testnet REST query (#3739)
  keep fcU consistent with actual DAG (#3748)
  remove unused `getExistingForkedBlock` overload (#3742)
  Version 22.5.2
  change proposer boost to 40%
  bump merge-testnets to get new ropsten TTD (#3668)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants