-
Notifications
You must be signed in to change notification settings - Fork 957
Custody backfill sync #7907
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
Custody backfill sync #7907
Conversation
pawanjay176
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went through the remaining changes for peer attribution and the conditions look good to me. Lets get this in!
|
Nice work all, lets go 🚀 |
|
This pull request has been removed from the queue for the following reason: Pull request #7907 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (details: 1 review requesting changes and 1 approving review by reviewers with write access.). You can check the last failing draft PR here: #8258. You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
|
@mergify requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
|
|
||
| /// Updates the `epoch_validator_custody_requirements` map by pruning all values on/after `effective_epoch` | ||
| /// and updating the map to store the latest validator custody requirements for the `effective_epoch`. | ||
| pub fn backfill_validator_custody_requirements(&mut self, effective_epoch: Epoch) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this file affects the CustodyContext behaviour and i think it's worth covering in unit tests.
I suspect this may not work well with the semi-supernode changes. I'll make a PR with tests.
Open PRs to include for the release - #7907 - #8247 - #8251 - #8253 - #8254 - #8265 - #8269 - #8266 Co-Authored-By: Jimmy Chen <[email protected]> Co-Authored-By: Jimmy Chen <[email protected]>

Issue Addressed
#7603
Proposed Changes
Custody backfill sync service
Similar in many ways to the current backfill service. There may be ways to unify the two services. The difficulty there is that the current backfill service tightly couples blocks and their associated blobs/data columns. Any attempts to unify the two services should be left to a separate PR in my opinion.
SyncNeworkContextSyncNetworkContextmanages custody sync data columns by range requests separetly from other sync RPC requests. I think this is a nice separation considering that custody backfill is its own service.Data column import logic
The import logic verifies KZG committments and that the data columns block root matches the block root in the nodes store before importing columns
New channel to send messages to
SyncManagerNow external services can communicate with the
SyncManager. In this PR this channel is used to trigger a custody sync. Alternatively we may be able to use the existingmpscchannel that theSyncNetworkContextuses to communicate with theSyncManager. I will spend some time reviewing this.TODOs
ValidatorRegistrations.epoch_validator_custody_requirementswhile custody syncing, or once its completedspeedologicAddtional notes
This needs to be throughouly tested before being included in
8.0.0-rc.0.