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

Make range sync chain Id sequential #6868

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

dapplion
Copy link
Collaborator

Issue Addressed

Currently, we set the chain_id of range sync chains to u64(hash(target_root, target_slot)), which results in a long integer.

Jan 27 00:43:27.246 DEBG Batch downloaded, chain: 4223372036854775807, awaiting_batches: 0, batch_state: [p,E,E,E,E], blocks: 0, epoch: 0, service: range_sync

Proposed Changes

Instead, we can use network_context.next_id() as we do for all other sync items and get a unique sequential (not too big) integer as id.

Jan 27 00:43:27.246 DEBG Batch downloaded, chain: 4, awaiting_batches: 0, batch_state: [p,E,E,E,E], blocks: 0, epoch: 0,  service: range_sync

Also, if a specific chain for the same target is retried later, it won't get the same ID so we can more clearly differentiate the logs associated with each attempt.

@dapplion dapplion requested a review from jxs as a code owner January 27, 2025 00:46
@dapplion dapplion added ready-for-review The code is ready for review syncing labels Jan 27, 2025
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

If this helps debugging moving away from a hash-based identification, then i'm all for it.

The changes here seem fine to me.


match collection
.iter_mut()
.find(|(_, chain)| chain.has_same_target(target_head_slot, target_head_root))
Copy link
Member

Choose a reason for hiding this comment

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

If we have multiple chains with the same target, this will return the first one.
But this isn't possible I think because we only create a new chain when we cannot find one matching the same target (the None case), so I think this is fine.

Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

LGTM

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jan 30, 2025
mergify bot added a commit that referenced this pull request Jan 30, 2025
@mergify mergify bot merged commit 7d54a43 into sigp:unstable Jan 30, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge. syncing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants