Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

ethcore: fix ancient block sync #9407

Closed
wants to merge 4 commits into from

Conversation

andresilva
Copy link
Contributor

@andresilva andresilva commented Aug 23, 2018

Fixes #9300 and fixes #7008. Merge after #9381.

@andresilva andresilva added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 23, 2018
@5chdn 5chdn added this to the 2.1 milestone Aug 23, 2018
@5chdn 5chdn added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Aug 23, 2018
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

POA network syncs old blocks successfully. Sync on mainnet however is stuck at #1914466. Though no longer retracting and getting those bad blocks! Investigating.

@ascjones ascjones mentioned this pull request Aug 24, 2018
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

It seems the code is shared with regular block import, would be good to triple check that resetting also works correctly for regular sync.

Overall the change seems reasonable.

let mut imported = HashSet::new();
let blocks = self.blocks.drain();
let count = blocks.len();

for block_and_receipts in blocks {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be worth to turn this loop into a closure and return an enum Importer { Reset, Bad, Imported(HashSet) } instead of setting the flags.

That said, refactoring can be left to a separate PR.

if bad {
return Err(BlockDownloaderImportError::Invalid);
}

if self.blocks.is_empty() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that correct? Seems that if we fail on the last block we are not going to return an error?

@ascjones ascjones removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Aug 24, 2018
@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 24, 2018
@andresilva andresilva added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Aug 27, 2018
@andresilva
Copy link
Contributor Author

@tomusdrw I didn't notice any changes on regular sync, seemed to be working fine. But it seems like this PR doesn't fix the original issue completely (#9300 (comment)). So I'm going to investigate this a bit more and I'll put this onice in the meantime.

This was referenced Aug 30, 2018
@5chdn 5chdn added the A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 8, 2018
@5chdn
Copy link
Contributor

5chdn commented Sep 8, 2018

What's the status?

@5chdn 5chdn removed this from the 2.1 milestone Sep 11, 2018
@5chdn 5chdn added this to the 2.2 milestone Sep 11, 2018
This was referenced Sep 11, 2018
@ascjones
Copy link
Contributor

@5chdn PR for a fix in #9531. Just waiting for mainnet sync to confirm.

@andresilva
Copy link
Contributor Author

After discussing with @ascjones and looking into the sync code a bit more, we've concluded that this PR won't fix the issue properly. Closing and hopefully it is fixed in #9531 (will have a look at it tomorrow 🙂).

@andresilva andresilva closed this Sep 11, 2018
@5chdn 5chdn mentioned this pull request Sep 19, 2018
2 tasks
@5chdn 5chdn deleted the andre/fix-ancient-block-sync branch November 27, 2018 11:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Background verifier sometimes goes back Ancient block sync stalls
4 participants