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

Revert "Handle staking info since kaia fork in downloader." #2182

Merged
merged 1 commit into from
May 22, 2024

Conversation

hyeonLewis
Copy link
Contributor

@hyeonLewis hyeonLewis commented May 21, 2024

Reverts #2160

Initially, I mis-interpreted the testing results so I merged it. Sorry for the confusion.

During several snap sync tests, it was found that #2160 can't solve the block header verification issue correctly. The block header initially comes to processHeaders, which inserts the header into the header chain first. Then, the contents for fetched headers will be scheduled by Schedule, which includes staking info. So, the staking info needs to be fetched with headers and ready before insertHeaderChain. But the current implementation fetches staking info after insertHeaderChain, which means it does nothing. (It worked before kaia fork since it required staking info before staking update interval)

Additionally, it seems that randao also breaks snap sync since it requires state at a block to retrieve the BLS registry. https://github.com/klaytn/klaytn/blob/dev/consensus/istanbul/backend/randao.go#L63

To sum up, I concluded it would be better to consider all the cases carefully to fix the snap sync. If you have suggestions, please let me know. Thanks.

@blukat29
Copy link
Contributor

I'm okay with turning off the snapsync again. It was our first plan wasn't it?
Let's sit back and try other solutions over time. Such as skipping header verification for the pivot block? which would require additional argument all the way through headerchain though.

@hyeonLewis
Copy link
Contributor Author

@blukat29 Thanks for your comment. Yes, disabling snap sync was our first plan.

We actually need to skip all the blocks inserted via insertHeaderChain, not only pivot block. So I think we have two options:

  1. Skip verifying headers, and verify it after fetching staking info.
  2. Adjust fetching timing of staking info after Kaia fork.

As I mentioned, we must include the bls registry (+ temporal DB for BLS registry) fetcher.

@hyeonLewis hyeonLewis merged commit a7808fc into dev May 22, 2024
11 checks passed
@hyeonLewis hyeonLewis deleted the revert-2160-fix-stakinginfo-sync branch May 22, 2024 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants