feat: syncer: optimize syncFork for one-epoch forks #11533
Merged
Conversation
This file contains 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
Stebalien
approved these changes
Dec 15, 2023
magik6k
approved these changes
Dec 15, 2023
| } | ||
|
|
||
| if commonParent { | ||
| // known contains at least one of incoming's Parents => the common ancestor is known's Parents (incoming's Grandparents) |
There was a problem hiding this comment.
Pretty sure this is a typo, as I parse it we only have the guarantee about common grandparents
Suggested change
| // known contains at least one of incoming's Parents => the common ancestor is known's Parents (incoming's Grandparents) | |
| // known contains at least one of incoming's Parents => the common ancestor is known's Grandparents (also incoming's Grandparents) |
There was a problem hiding this comment.
(the logic does appear to be correct tho)
There was a problem hiding this comment.
@magik6k No, I don't think this is right? The logic in 891-895 is checking whether incoming.Parents() has any blocks in common with known (so known = {A,B,C,D} in your diagram).
|
Merging cuz the only outstanding discussion is about a comment, but happy to revisit. |
8 tasks
3 tasks
rjan90
pushed a commit
that referenced
this pull request
Jan 4, 2024
feat: syncer: optimize syncFork for one-epoch forks
rjan90
pushed a commit
that referenced
this pull request
Jan 4, 2024
feat: syncer: optimize syncFork for one-epoch forks
8 tasks
This was referenced Feb 6, 2024
Closed
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Related Issues
Towards #11530 (comment)
Logging and analysis have shown that nearly 100% of calls to syncFork are for a very specific case: when
incoming.Parents() ⊂ known. Currently, we fetch 900 tipsets in this case. We actually need to fetch ZERO.Proposed Changes
Check if there are any common blocks between
knownandincoming.Parents(). If there are, we simply need to return [incoming.Parents, incoming]. We know we already haveincoming, so we just needincoming.Parents. We now check our local store to fetchincoming.Parents(which according to experiments we almost always do), and if we don't we fallback onto the network requesting the ONE tipset.Additional Info
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>fix: mempool: Introduce a cache for valid signaturesPR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps