- 
                Notifications
    You must be signed in to change notification settings 
- Fork 930
Fix CGC backfill race condition #8267
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
base: release-v8.0
Are you sure you want to change the base?
Conversation
| Updated the target branch to  | 
| // This should verify that a data column sidecar associated to a block root that doesn't exist in the store cannot | ||
| // be imported. | ||
| #[tokio::test] | ||
| async fn test_import_historical_data_columns_batch_no_block_found() { | 
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.
this was a redundant test case
| let store = get_store_generic(&db_path, StoreConfig::default(), spec); | ||
| let start_slot = Epoch::new(0).start_slot(E::slots_per_epoch()) + 1; | ||
| let end_slot = Epoch::new(0).end_slot(E::slots_per_epoch()); | ||
| let cgc = 128; | 
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.
nit: could use either E::number_of_columns or harness.chain.data_availability_checker.custody_context().custody_group_count_at_head(&sepc) instead.
| .forwards_iter_block_roots_until(start_slot, end_slot) | ||
| .unwrap(); | ||
|  | ||
| // Assert that data columns now exist for epoch 0 | 
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.
Nice, these comments help! Thanks
| let store = get_store_generic(&db_path, StoreConfig::default(), spec); | ||
| let start_slot = Slot::new(1); | ||
| let end_slot = Slot::new(E::slots_per_epoch() * 2 - 1); | ||
| let cgc = 128; | 
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.
Same as above
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.
Nice, the logic looks correct to me. Just a small nit on hard coded values - i think the values is unlikely to change but would be good to keep hard coded values to a minimum for maintainability.
Issue Addressed
During custody backfill sync there could be an edge case where we update CGC at the same time where we are importing a batch of columns which may cause us to incorrectly overwrite values when calling
backfill_validator_custody_requirements. To prevent this race condition, the expected cgc is now passed into this function and is used to check if the expected cgc == the current validator cgc. If the values arent equal, this probably indicates that a very recent CGC occurred so we do not prune/update values in theepoch_validator_custody_requirementsmap.