refactor Sync top level functions #5328
Conversation
schomatis
requested review from
Kubuxu,
magik6k and
whyrusleeping
as code owners
| // `iterFullTipsets`). We should extract it out and probably request | ||
| // *both* block headers and messages together through the corresponding | ||
| // chain exchange interface. We can do very little validation without the | ||
| // messages so it shouldn't have much impact. |
There was a problem hiding this comment.
Fetching messages has quite an impact on sync speed. We should not be doing it eagerly.
One of the most important parts of validation is not syncing headers we already failed to sync (and not fetching messages for them because we did not persist them).
There was a problem hiding this comment.
Fair point. We probably shouldn't request as much messages as chain we are fetching but it still seems to me we could benefit from eagerly requesting some amount of messages in advance for upcoming blocks. We are currently at the other extreme of only requesting at the same moment of validation stalling the pipeline. Anyway, just a note, not going to change any of that here.
| StageHeaders | ||
| StagePersistHeaders |
There was a problem hiding this comment.
We should leave it here, for now, removing it will re-number the enum resulting in incompatibility.
We should open an issue for API breaking changes we want to do, mention this renumbering and link to the issue from here.
|
@magik6k would you say this no longer makes sense given your work refactoring lotus/chain? |
|
Yeah, we can close this. |
Homogenize
Sync()calls to better reflect the different states.Remove
StagePersistHeaders, absorbed byStageHeadersnow (the first being local is of low complexity, execution time and possibilities of error compared to the second).Extract
syncMessagesAndCheckStateout ofcollectChain()and into the mainSync()call flow.