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

[DNM] introduce new sync_head for tracking header chain during sync #410

Closed
wants to merge 3 commits into from

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Nov 30, 2017

#413 was just merged onto testnet1 branch and contains some additional changes.
I'll port these across, either here or on another PR to replace this one.

  • init sync_head during chain init
  • rename process_block_header to sync_block_header (more explicit that this is sync only)
  • in sync use sync_header to track the header chain that we are syncing to
  • update sync_head as we process headers during the sync
  • update header_head during sync when processing headers if new header is now the one with most work
  • pass in 2x ctxs (sync and header) to sync_block_header to maintain both head states

Resolves #402.

@antiochp
Copy link
Member Author

@ignopeverell @yeastplume

Testing locally now under various scenarios - looks like it is working as expected so far.

Copy link
Member

@yeastplume yeastplume left a comment

Choose a reason for hiding this comment

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

Looks okay as a start, should hopefully get everyone past these looping issues we're having. Sync_head is definitely better than header_head everywhere.
I hadn't considered keeping the sync_head in the store as opposed to just in-memory, but I guess it makes sense in an environment with more reliable blockchain states as opposed to the sea of special snowflakes we have now (i.e., on restart in the real world you'll be likely to attach to another peer with the mostly the same chain when syncing, so it makes sense)

@ignopeverell
Copy link
Contributor

In don't think you should completely to get rid of the header head here. I think you still want to track the currently most worked head somewhere.

Here is a scenario:

  • You sync all good.
  • A peer either lies about its total work or has an invalid block. You'll follow it a bit before realizing the issue. Your sync head is at a weird position.
  • Someone else comes in with more work than you but now your sync head is in a weird state, so locator generation and work comparisons will get weird.

@antiochp
Copy link
Member Author

antiochp commented Nov 30, 2017

We still maintain header_head when we process blocks though.
In process_block we call update_head and this updates both heads (header and body) in non-SYNC mode (just body head in SYNC mode).

Oh - are you saying this is problematic because header_head gets "left behind" in SYNC mode?

Do we want to update header_head if total_difficulty is greater than we currently have, when syncing? (I think we do, just wondering what the implications are if body_head and header_head are on different forks).

@antiochp
Copy link
Member Author

@ignopeverell pushed some more changes to keep header_head updated as we sync new headers (if more work on new header).

@ignopeverell
Copy link
Contributor

I think you need to reset the sync head to the header head each time we restart sync (in the adapter). Or am I missing something?

@antiochp
Copy link
Member Author

Yeah I think you're right - I don't think its absolutely necessary but there's a lot of wasted effort if we don't do this.

@antiochp antiochp changed the title introduce new sync_head for tracking header chain during sync [DNM] introduce new sync_head for tracking header chain during sync Dec 1, 2017
@antiochp
Copy link
Member Author

antiochp commented Dec 4, 2017

Replaced by #425 (porting the changes back from testnet1).

@antiochp antiochp closed this Dec 4, 2017
@antiochp antiochp deleted the sync_head branch May 2, 2018 11:30
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.

3 participants