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

Fix missing staking info #225

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

hyeonLewis
Copy link
Contributor

Proposed changes

We've found there's a missing staking info issue after deleting CreateSnapshot at node startup. The staking info for the current block (=stateAt(parent)) is needed to validate that the peer is in validator sets if the node is CN. To fix this, I modified kaiax/staking module slightly.

  • Cache the staking info at Preload.
  • Purge cache at RewindTo for setHead operation.
    • This is needed since the cached staking info will be purged at Start without this.

And in the downloader, it doesn't request a block range that contains current block when it is lower than MaxHeaderFetch, which results in missing staking info. Let's assume the current head is 74.

  • As-Is:
    • It requests peer the block for [0, 16, 32, 48, 64], not including 74.
    • When the peer returns the header, it'll select 64 as a common ancestor since it's the highest.
    • We can't insert block 64 since we don't have states at 63 (staking info) and 64.
  • To-Be:
    • It requests peer the block for [10, 26, 42, 58, 74], including 74.
    • When the peer returns the header, it'll select 74 as a common ancestor since it's the highest.
    • Successfully insert new block since 75.

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have read the CLA and signed by comment I have read the CLA Document and I hereby sign the CLA in first time contribute
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

  • Please leave the issue numbers or links related to this PR here.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

Copy link
Contributor

@hyunsooda hyunsooda left a comment

Choose a reason for hiding this comment

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

To understand the trigger condition, can it occur when the current block number does not exceed MaxHeaderFetch (192)?

node/cn/backend.go Show resolved Hide resolved
@hyeonLewis
Copy link
Contributor Author

@hyunsooda Yes, and the startup issue can happen on any block.

@hyunsooda
Copy link
Contributor

@hyeonLewis Uhm..? https://github.com/kaiachain/kaia/pull/225/files#diff-99163d68e23ed18d94f9b098d41c9d8c23eeb3e71299102c5c25c9a4ef6245e8R832-R833 To be from value negative, I thought that the head must be less than 192. So, if the current block number is larger than it, it will not be triggered.

@hyeonLewis
Copy link
Contributor Author

@hyunsooda Downloader has no issue after 192, but I mean missing staking info to validate peer when starting up the node.

@hyunsooda
Copy link
Contributor

hyunsooda commented Jan 23, 2025

Yes, state regeneration is a prerequisite (1. resolving startup issue to validate peer). The question was about the trigger condition against selecting the ancestor header number, which should have state. (2. resolving header selection which has state, but prerequisite is still required)

@hyeonLewis
Copy link
Contributor Author

Will be rebased on top of #229 to fix the test failure.

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.

2 participants