Make range sync chain Id sequential#6868
Merged
mergify[bot] merged 1 commit intosigp:unstablefrom Jan 30, 2025
Merged
Conversation
AgeManning
approved these changes
Jan 28, 2025
Member
AgeManning
left a comment
AgeManning
left a comment
There was a problem hiding this comment.
If this helps debugging moving away from a hash-based identification, then i'm all for it.
The changes here seem fine to me.
jimmygchen
reviewed
Jan 30, 2025
|
|
||
| match collection | ||
| .iter_mut() | ||
| .find(|(_, chain)| chain.has_same_target(target_head_slot, target_head_root)) |
Member
There was a problem hiding this comment.
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.
jimmygchen
added
ready-for-merge
10 tasks
mergify bot
pushed a commit
that referenced
this pull request
Feb 10, 2025
- Re-opened PR from #6869 Writing and running tests I noted that the sync RPC requests are very verbose now. `DataColumnsByRootRequestId { id: 123, requester: Custody(CustodyId { requester: CustodyRequester(SingleLookupReqId { req_id: 121, lookup_id: 101 }) }) }` Since this Id is logged rather often I believe there's value in 1. Making them more succinct for log verbosity 2. Make them a string that's easy to copy and work with elastic Write custom `Display` implementations to render Ids in a more DX format _ DataColumnsByRootRequestId with a block lookup_ ``` 123/Custody/121/Lookup/101 ``` _DataColumnsByRangeRequestId_ ``` 123/122/RangeSync/0/5492900659401505034 ``` - This one will be shorter after #6868 Also made the logs format and text consistent across all methods
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue Addressed
Currently, we set the
chain_idof range sync chains tou64(hash(target_root, target_slot)), which results in a long integer.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.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.