Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@arkpar
Copy link
Member

@arkpar arkpar commented Dec 7, 2018

The sync module needs to track the best queued block, it does not care so much about the best imported. Before the block queue was introduced these were effectively the same. With the block queue this small change is required.

@arkpar arkpar added the A0-please_review Pull request needs code review. label Dec 7, 2018
@gnunicorn
Copy link
Contributor

The change looks fine,

I am just wondering about two scenarios:

  1. what is said test fails to import? E.g. because the block provided doesn't actually verify. Before we have confirmed that, the block provided my be malicious. How do we ensure we recover from that scenario?
    • but that is more general I suppose - how do we deal with forks in the chain. as from what I can see right now, the code assumes there is only ever one block at a specific height. Is that still to be sure with Aura? Isn't it allowed for two authorities to have potentially created different blocks at the same height (because the second didn't see the previous before it authored).

Arguable neither of these two problem were introduced in this PR. However if acknowledge these problem, we should create tickets for them, now?

//cc @rphmeier

@gnunicorn gnunicorn added A8-looksgood Z7-question Issue is a question. Closer should answer. and removed A8-looksgood labels Dec 10, 2018
@arkpar arkpar added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. A7-looksgoodtestsfail labels Dec 10, 2018
@arkpar
Copy link
Member Author

arkpar commented Dec 10, 2018

Originally tie syncing code was supposed to correctly download blocks from all forks. For that it keeps track of most recent common block with every peers and downloads from that point. This appears to broken now indeed as indicated by the tests. I've pushed another commit that fixes it.

As for detecting malicious behaviour the block queue keeps track of BlockOrigin for each block and indeed bans peers that give bad blocks. If such block is detected the sync process restarts.

@arkpar arkpar added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. Z7-question Issue is a question. Closer should answer. labels Dec 10, 2018
@gavofyork gavofyork merged commit 4bc45ee into master Dec 10, 2018
@arkpar arkpar mentioned this pull request Dec 10, 2018
@arkpar arkpar deleted the a-fix-sync branch December 20, 2018 13:15
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Apr 12, 2019
* Fixed common block tracking when syncing

* Fixed fork resolution
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants