Skip to content

Deduplicate blocksync#3543

Merged
vicsn merged 49 commits intoProvableHQ:stagingfrom
kaimast:deduplicate-blocksync
Apr 9, 2025
Merged

Deduplicate blocksync#3543
vicsn merged 49 commits intoProvableHQ:stagingfrom
kaimast:deduplicate-blocksync

Conversation

@kaimast
Copy link
Contributor

@kaimast kaimast commented Mar 18, 2025

Motivation

There are two different code paths that perform block synchronization: one for validators, and one for clients/provers.
In the case of validators, there exist two different instances of BlockSync.

This PR aims to address this by moving code that specific to validators or clients. It also improves documentation of the block synchronization logic.
Finally, the PR serves as the basis for upcoming changes to block synchronization that will harden security and improve performance of the protocol.

Overview

The following is an outline of the structs affected by this PR. It also highlights which structs received significant changes.

Outline

Changes

There have been a multiple updates to this PR, and I would prefer if it gets squashes before a merge to not clutter the commit history too much.

Below is a list of the first set of commits that implemented the vast majority of the PR.

  • 509c266 changes the validator code to only use one instance of BlockSync.
  • 258ffcb and 173e8e4 perform minor clean-ups and improve code documentation/comments.
  • 4adaf15 removes duplicated code from testing, because we need to pass BlockSync every time due to the very first commit.
  • Finally, 0c75996 significantly refactors BlockSync and moves all client specific code to the client.

Test Plan

Existing tests should suffice, as this PR does not change any functionality.

kaimast and others added 29 commits February 26, 2025 21:14
Co-authored-by: Niklas Long <niklaslong@users.noreply.github.com>
Signed-off-by: Kai Mast <kai@kaimast.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
Refactor BlockSync and move client-specific, or validator-specific, code to the client, or validator, logic.
@kaimast kaimast force-pushed the deduplicate-blocksync branch from 62ec311 to 0c75996 Compare March 18, 2025 21:27
@howardwu
Copy link
Member

^You can add this to the main PR description for improved visibility.

Follow up questions for main PR description:

  • What memory or time performance improvements to syncing (if any) are improved in this PR?
  • What unit or integration tests were introduced to validate the changes made to syncing in this PR?

@raychu86
Copy link
Collaborator

raychu86 commented Mar 27, 2025

@kpandl Could you run the sync tests on this PR to check if there are any improvements/regressions to syncing after the change?

@kaimast kaimast force-pushed the deduplicate-blocksync branch from 046decd to bff7a23 Compare April 7, 2025 18:14
@kaimast
Copy link
Contributor Author

kaimast commented Apr 7, 2025

I updated the PR description to better reflect the current state of this PR and added a simplified diagram. Hope we can get it merged this week. @vicsn

raychu86
raychu86 previously approved these changes Apr 7, 2025
Copy link
Collaborator

@raychu86 raychu86 left a comment

Choose a reason for hiding this comment

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

Great cleanup and unification of disjoint usages! Code overall looks logically correct.

Would wait to merge until proper sync stress tests have been run and reported. @kpandl would appreciate any updates once you've had a change to run the tests

@kpandl
Copy link
Collaborator

kpandl commented Apr 9, 2025

I've ran 2 clients syncing on mainnet.
Screenshot 2025-04-09 at 16 11 42

Yellow is staging, and green is this feature branch. So syncing generally works. For this feature branch, syncing was ~3% slower, so not much. Would need more runs to have significance, but since this PR mostly reorganizes, might be noise. LGTM.

Note that I haven't run the classic stress-test, since we are currently having issues with the automated test infra.

Co-authored-by: Raymond Chu <14917648+raychu86@users.noreply.github.com>
Signed-off-by: Kai Mast <kai@kaimast.com>
@vicsn vicsn merged commit c7232f5 into ProvableHQ:staging Apr 9, 2025
2 checks passed
kaimast added a commit to kaimast/snarkOS that referenced this pull request Apr 29, 2025
vicsn added a commit that referenced this pull request Apr 30, 2025
kaimast added a commit to kaimast/snarkOS that referenced this pull request May 3, 2025
kaimast added a commit to kaimast/snarkOS that referenced this pull request May 7, 2025
kaimast added a commit to kaimast/snarkOS that referenced this pull request May 9, 2025
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.

8 participants