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

Additional snapshot sync checks #3318

Merged
merged 3 commits into from
Nov 9, 2016
Merged

Additional snapshot sync checks #3318

merged 3 commits into from
Nov 9, 2016

Conversation

arkpar
Copy link
Collaborator

@arkpar arkpar commented Nov 9, 2016

No description provided.

@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. B0-patch M4-core ⛓ Core client code / Rust. labels Nov 9, 2016
let best_block = io.chain().chain_info().best_block_number;
// Make sure the snapshot block is not too far away from our best block, network best block and
// fork detection block
let mut best_block = max(io.chain().chain_info().best_block_number, self.highest_block.unwrap_or(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't highest_block the highest block we've seen broadcasted? how will the check for sn - best_block > SNAPSHOT_RESTORE_THRESHOLD ever pass?

@rphmeier rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 9, 2016
@arkpar arkpar added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Nov 9, 2016
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Nov 9, 2016
.filter(|&(_, p)| p.is_allowed() && p.snapshot_number.map_or(false, |sn|
our_best_block < sn && (sn - our_best_block) > SNAPSHOT_RESTORE_THRESHOLD &&
sn > fork_block &&
self.highest_block.map_or(true, |highest| highest >= sn && (highest - sn) > SNAPSHOT_RESTORE_THRESHOLD)
Copy link
Contributor

Choose a reason for hiding this comment

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

won't this prevent the most recent snapshot from ever being used? This is basically requiring that whatever snapshot is chosen is at least 100k blocks old.

Copy link
Contributor

Choose a reason for hiding this comment

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

is it a typo? < would make sense here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to get some sleep

@rphmeier rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 9, 2016
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Nov 9, 2016
@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 9, 2016
@arkpar arkpar merged commit eba0dd5 into master Nov 9, 2016
@arkpar arkpar deleted the snapshot-sync branch November 9, 2016 18:46
arkpar added a commit that referenced this pull request Nov 9, 2016
* Additional snapshot sync checks

* Proper checks

* Proper highset block check
arkpar added a commit that referenced this pull request Nov 9, 2016
* Additional snapshot sync checks

* Proper checks

* Proper highset block check
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants