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

eth/downloader: make fast sync resilient to critical section fails #2647

Merged
merged 1 commit into from
Jun 6, 2016

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented Jun 2, 2016

This PR makes fast sync more resilient to failures in the critical section (i.e. when downloading the pivot states) by allowing up to 10 failures to be registered before bailing out as a potential attack. It is important to note that to retain the security guarantees of fast sync, the pivot block must not change between retries.

As such, this PR drops the downloader.noFast flag that we used to force-disable fast syncing with in favor of downloader.fsPivotFails counter to accumulate failures until a threshold is reached. At the same time, we also introduce downloader.fsPivotLock that will be initialized to the block header of the pivot on the first failure in the critical section.

Subsequent fast sync attempts will:

  • Choose the pivot block number to be the one locked in during the first failure
  • Validate any headers at the pivot height to match the one locked in during the first failure

The PR also contains two tiny downloader fixes:

  • When a sync cycle starts, empty out any buffered data packets in the delivery channels from the previous sync cycle. This works around an extremely rare issue whereby the next cycle's head header retrieval is actually satisfied by a previous cycle's buffered header delivery. The probability of this happening is negligible (only 1 packet is ever buffered, and that single one must be from the same peer that we sync with in the next cycle), further the issue can also happen due to network latency which we can't detect either way. The only place this makes a difference is in the test suite where detecting specific failure scenarios is desirable.
  • Fixes a possible deadlock if header downloads is finished ok simultaneously with a sync cancellation, whereby the header processor quits due to the abortion and never reads the success signal pending by the header downloader.

@robotally
Copy link

robotally commented Jun 2, 2016

Vote Count Reviewers
👍 0
👎 0

Updated: Mon Jun 6 09:13:37 UTC 2016

@karalabe karalabe added this to the 1.4.6 milestone Jun 2, 2016
@codecov-io
Copy link

Current coverage is 56.89%

Merging #2647 into develop will increase coverage by <.01%

@@            develop      #2647   diff @@
==========================================
  Files           216        216          
  Lines         24539      24560    +21   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          13953      13971    +18   
- Misses        10585      10587     +2   
- Partials          1          2     +1   

Powered by Codecov. Last updated by 16a23ff...61ee9f2

@fjl fjl mentioned this pull request Jun 2, 2016
7 tasks
if height > uint64(fsMinFullBlocks)+pivotOffset.Uint64() {
pivot = height - uint64(fsMinFullBlocks) - pivotOffset.Uint64()
if d.fsPivotLock == nil {
pivotOffset, err := rand.Int(rand.Reader, big.NewInt(int64(fsPivotInterval)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need crypto rand here? Can't you use math's rand package and omit the big int to uint casting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, this part is quite critical to be truly random and non guessable. As it's executed once per sync cycle max, performance wise it doesn't matter and I figured that it's better to be on the safe side.

@obscuren
Copy link
Contributor

obscuren commented Jun 6, 2016

LGTM 👍

@karalabe karalabe merged commit 780bdb3 into ethereum:develop Jun 6, 2016
@obscuren obscuren removed the review label Jun 6, 2016
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.

4 participants