eth/downloader: separate state sync from queue #14460

fjl · 2017-05-11T09:57:25Z

Scheduling of state node downloads hogged the downloader queue lock when
new requests were scheduled. This caused timeouts for other requests.
With this change, state sync is fully independent of all other downloads
and doesn't involve the queue at all.

State sync is started and checked on in processContent. This is slightly
awkward because processContent doesn't have a select loop. Instead, the
queue is closed by an auxiliary goroutine when state sync fails. We
tried several alternatives to this but settled on the current approach
because it's the least amount of change overall.

Handling of the pivot block has changed slightly: the queue previously
prevented import of pivot block receipts before the state of the pivot
block was available. In this commit, the receipt will be imported before
the state. This causes an annoyance where the pivot block is committed
as fast block head even when state downloads fail. Stay tuned for more
updates in this area ;)

karalabe · 2017-05-11T11:55:04Z

eth/downloader/queue.go

-	queued := q.blockTaskQueue.Size() + q.receiptTaskQueue.Size() + q.stateTaskQueue.Size()
-	pending := len(q.blockPendPool) + len(q.receiptPendPool) + len(q.statePendPool)
+	queued := q.blockTaskQueue.Size() + q.receiptTaskQueue.Size()
+	pending := len(q.blockPendPool) + len(q.receiptPendPool)
 	cached := len(q.blockDonePool) + len(q.receiptDonePool)


The invariant of this function changed a bit. Idle will now return true even if state sync is still in progress. Not sure yet how this affects sync overall, but you should be aware. If it's fine, the docs need to be updated to state so.

karalabe · 2017-05-11T12:03:19Z

eth/downloader/queue.go

 				// Stop before processing the pivot block to ensure that
 				// resultCache has space for fsHeaderForceVerify items. Not
 				// doing this could leave us unable to download the required
 				// amount of headers.
-				if i > 0 || len(q.stateTaskPool) > 0 || q.pendingNodeDataLocked() > 0 {
-					return i
-				}


Again a tiny invariant change. I don't yet see how this could have an adverse effect, just thought I'd mention it. Previously the code handled the pivot block fully independently from any other blocks. The new code places the pivot block possibly to the end of a fast sync import batch. It should work, just mentioning the behavioral change.

karalabe · 2017-05-11T12:13:18Z

eth/downloader/statesync.go

+	for ; n >= 0 && len(s.retry) > 0; n-- {
+		items = append(items, s.retry[len(s.retry)-1])
+		s.retry = s.retry[:len(s.retry)-1]
+	}


You are not maintaining a set of items that a node does not have. If you have for example a single peer, which does not have any data (e.g. it's also fast syncing), this loop will request the same data over and over again indefinitely from the peer. You need to mark any data unavailable from a peer and not request it ever again from there.

Note however that maintaining such a list and avoiding reassigning the same hash over and over again will cause the next line to potentially OOM the node, because there would be no memory cap on the total size of the pending state retrievals.

The original code moved trie requests from the scheduler.Missing to an intermediate pool (similar to your retry slice, but one that contained all pending ones), and only requested new missing ones if it fit into the pool. You could do a similar thing by maintaining a counter with the total pending requests, and do:

return append(items, s.sched.Missing(min(n, maxPend - curPending))...)

That would ensure that we don't grow the number of outstanding requests unboundedly.

karalabe · 2017-05-11T12:21:08Z

eth/downloader/statesync.go

+			s.fetching[peer.id] = s.spawnFetch(peer, items, rtt)
+		}
+		peers = peers[1:]
+	}


You need to handle the case when you have literally no nodes that can services the requests defined by the trie root. This is an attack vector where someone feeds you an invalid head block (you can't verify its validity when starting the sync). In such a case when no peers can actually provide data for the head root hash, it must be considered invalid/attack and the peer you're syncing with (the master peer) dropped.

karalabe · 2017-05-11T12:21:59Z

eth/downloader/statesync.go

+
+func (s *stateSync) spawnFetch(p *peer, nodes []common.Hash, timeout time.Duration) *stateReq {
+	log.Trace("Requesting node data", "peer", p.id, "items", len(nodes), "timeout", timeout)
+	req := &stateReq{p, nodes, make(chan struct{})}


Please use named fields.

karalabe · 2017-05-11T12:29:51Z

eth/downloader/statesync.go

+			s.requestDone(req)
+			s.retry = append(s.retry, req.items...)
+			for _, hash := range req.items {
+				req.peer.MarkLacking(hash)


Timeout should not mark them lacking. The node will try to request smaller batches. The same nodes can be retrieved later too potentially, no need to never retry.

karalabe · 2017-05-11T12:34:05Z

eth/downloader/statesync.go

+
+func (s *stateSync) requestDone(req *stateReq) {
+	delete(s.fetching, req.peer.id)
+	close(req.cancelTimeout)


Convert req.cancelTimeout into req.timeoutTimer and do a req.timeoutTimer.Stop() instead here.

karalabe · 2017-05-11T12:37:05Z

eth/downloader/statesync.go

+		batch       = s.db.NewBatch()
+	)
+	for _, blob := range pack.states {
+		hash := common.BytesToHash(crypto.Keccak256(blob))


You probably want to reuse a hashes throughout the for loop. There are almost 400 states per request, so there's no reason to do so many hasher allocs and deallocs.

karalabe · 2017-05-11T12:44:19Z

eth/downloader/statesync.go

+			}
+			s.updateStats(n, time.Since(procStart))
+
+		case req := <-s.timeout:


This select construct has the same issue as the current code:

select { case packI := <-deliver: // Process case req := <-s.timeout: }

Process can take a long time. By that time all timeouts will fire (even if all the nodes actually delivered too). The end result is that this select statement picks deliver/timeouts randomly if both are available, so it will consider requests timed out that are actually queued up on the deliver channel.

karalabe · 2017-05-11T12:46:21Z

eth/downloader/statesync.go

+			for _, hash := range req.items {
+				req.peer.MarkLacking(hash)
+			}
+			req.peer.SetNodeDataIdle(0)


Previously if a node timed out while requesting a single entry, it was dropped. This is required, otherwise a single really bad peer has the capacity to stall the entire sync. It's fine to have slow peers after sync is doe, but sync requires heavy downloads, so we can't afford slow peers. That's where the hard timeouts came into play in the original code.

fjl · 2017-05-29T15:37:02Z

@Arachnid PTAL

Arachnid · 2017-05-12T14:11:55Z

eth/downloader/downloader.go

@@ -212,7 +216,7 @@ func New(mode SyncMode, stateDb ethdb.Database, mux *event.TypeMux, hasHeader he
 // these are zero.
 func (d *Downloader) Progress() ethereum.SyncProgress {
 	// Fetch the pending state count outside of the lock to prevent unforeseen deadlocks
-	pendingStates := uint64(d.queue.PendingNodeData())
+	// pendingStates := uint64(d.queue.PendingNodeData())


Please delete commented line.

Arachnid · 2017-05-12T14:47:37Z

eth/downloader/statesync.go

+		case req := <-d.trackStateReq:
+			activeReqs[req.peer.id] = req
+			req.timer = time.AfterFunc(req.timeout, func() {
+				select {


Can you leave a comment about the reason for this?

karalabe · 2017-05-30T07:21:42Z

eth/downloader/statesync.go

+		case <-s.cancel:
+			return errCancelStateFetch
+		case req := <-s.deliver:
+			// response or timeout


~~This path doesn't get called on timeout, so any node which timed out will forever get stuck.~~

karalabe · 2017-05-30T13:53:12Z

eth/downloader/downloader.go

-	}
-
-	pivot := d.queue.FastSyncPivot()
+// processBlocks takes fetch results from the queue and tries to import them


name mismatch

LGTM, though since I've added some code, would be nice for someone else to review

Scheduling of state node downloads hogged the downloader queue lock when new requests were scheduled. This caused timeouts for other requests. With this change, state sync is fully independent of all other downloads and doesn't involve the queue at all. State sync is started and checked on in processContent. This is slightly awkward because processContent doesn't have a select loop. Instead, the queue is closed by an auxiliary goroutine when state sync fails. We tried several alternatives to this but settled on the current approach because it's the least amount of change overall. Handling of the pivot block has changed slightly: the queue previously prevented import of pivot block receipts before the state of the pivot block was available. In this commit, the receipt will be imported before the state. This causes an annoyance where the pivot block is committed as fast block head even when state downloads fail. Stay tuned for more updates in this area ;)

This change also ensures that pivot block receipts aren't imported before the pivot block itself.

It fails the sync too much.

Fixes TestDeliverHeadersHang*Fast and (hopefully) the weird cancellation behaviour at the end of fast sync.

This commit explicitly tracks duplicate and unexpected state delieveries done against a trie Sync structure, also adding there to import info logs. The commit moves the db batch used to commit trie changes one level deeper so its flushed after every node insertion. This is needed to avoid a lot of duplicate retrievals caused by inconsistencies between Sync internals and database. A better approach is to track not-yet-written states in trie.Sync and flush on commit, but I'm focuing on correctness first now. The commit fixes a regression around pivot block fail count. The counter previously was reset to 1 if and only if a sync cycle progressed (inserted at least 1 entry to the database). The current code reset it already if a node was delivered, which is not stong enough, because unless it ends up written to disk, an attacker can just loop and attack ad infinitum. The commit also fixes a regression around state deliveries and timeouts. The old downloader tracked if a delivery is stale (none of the deliveries were requestedt), in which case it didn't mark the node idle and did not send further requests, since it signals a past timeout. The current code did mark it idle even on stale deliveries, which eventually caused two requests to be in flight at the same time, making the deliveries always stale and mass duplicating retrievals between multiple peers.

This commit fixes the hang seen sometimes while doing the state sync. The cause of the hang was a rare combination of events: request state data from peer, peer drops and reconnects almost immediately. This caused a new download task to be assigned to the peer, overwriting the old one still waiting for a timeout, which in turned leaked the requests out, never to be retried. The fix is to ensure that a task assignment moves any pending one back into the retry queue. The commit also fixes a regression with peer dropping due to stalls. The current code considered a peer stalling if they timed out delivering 1 item. However, the downloader never requests only one, the minimum is 2 (attempt to fine tune estimated latency/bandwidth). The fix is simply to drop if a timeout is detected at 2 items. Apart from the above bugfixes, the commit contains some code polishes I made while debugging the hang.

timtait · 2017-06-20T06:05:38Z

I don't really know enough about the whole process to give an overall 👍 or 👎

But for what it's worth I've had a good read through the last few commits from @karalabe and they look good to me 👍

fjl · 2017-06-21T12:36:19Z

trie/sync.go

@@ -48,6 +52,21 @@ type SyncResult struct {
 	Data []byte      // Data content of the retrieved node
 }

+// SyncMemCache is an in-memory cache of successfully downloaded but not yet
+// persisted data items.
+type SyncMemCache struct {


Please call it syncBatch

fjl added the in progress label May 11, 2017

fjl force-pushed the new-state-sync branch from 0252600 to eb2656f Compare May 11, 2017 10:02

karalabe previously requested changes May 11, 2017

View reviewed changes

karalabe mentioned this pull request May 11, 2017

eth/downloader: don't process trie nodes on the queue thread #14449

Closed

karalabe added this to the 1.6.2 milestone May 24, 2017

remyroy mentioned this pull request May 24, 2017

sync very slow #14433

Closed

fjl force-pushed the new-state-sync branch 3 times, most recently from 4c8d741 to 203158d Compare May 29, 2017 15:36

fjl requested a review from Arachnid May 29, 2017 15:44

Arachnid reviewed May 29, 2017

View reviewed changes

fjl removed the in progress label May 29, 2017

karalabe reviewed May 30, 2017

View reviewed changes

fjl force-pushed the new-state-sync branch from 192f5e0 to a6566e0 Compare May 30, 2017 10:06

karalabe reviewed May 30, 2017

View reviewed changes

fjl modified the milestones: 1.6.3, 1.6.2 May 31, 2017

karalabe modified the milestones: 1.6.4, 1.6.3 Jun 1, 2017

remyroy mentioned this pull request Jun 6, 2017

Geth is taking way too long to sync on a mechanical hdd #14575

Closed

karalabe modified the milestones: 1.6.6, 1.6.4 Jun 16, 2017

remyroy mentioned this pull request Jun 19, 2017

Geth sync performance #14643

Closed

fjl added 5 commits June 19, 2017 13:15

eth/downloader: remove cancelTimeout channel

c7e5270

eth/downloader: retry state requests on timeout

4209354

eth/downloader: improve comment

ac085e0

eth/downloader: mark peers idle when state sync is done

465acb2

fjl and others added 12 commits June 19, 2017 13:15

eth/downloader: move pivot block splitting to processContent

4c6040c

This change also ensures that pivot block receipts aren't imported before the pivot block itself.

eth/downloader: limit state node retries

44b8cb8

eth/downloader: improve state node error handling and retry check

5339c79

eth/downloader: remove maxStateNodeRetries

339bc10

It fails the sync too much.

eth/downloader: remove last use of cancelCh in statesync.go

baa79e9

Fixes TestDeliverHeadersHang*Fast and (hopefully) the weird cancellation behaviour at the end of fast sync.

eth/downloader: fix leak in runStateSync

6588e66

eth/downloader: don't run processFullSyncContent in LightSync mode

5029223

eth/downloader: improve comments

50d0c3f

eth/downloader: fix vet, megacheck

b48521a

eth/downloader: remove unrequested tasks anyway

829a042

karalabe force-pushed the new-state-sync branch from 816b645 to 115768a Compare June 19, 2017 10:21

core, eth, trie: support batched trie sync db writes

2410187

fjl commented Jun 21, 2017

View reviewed changes

trie: rename SyncMemCache to syncMemBatch

8a22650

karalabe merged commit 0042f13 into ethereum:master Jun 22, 2017

wtfiwtz mentioned this pull request Sep 2, 2017

geth --fast stalls before crossing finish line #15001

Closed

This was referenced Jan 5, 2018

Fix/dl stop syncing whilei/go-ethereum#2

Closed

Fix/dl stop syncing ethereumproject/go-ethereum#457

Closed

Fix/dl stop syncing whilei/go-ethereum#3

Merged

whilei mentioned this pull request Feb 16, 2018

make state sync independent ethereumproject/go-ethereum#506

Merged

1 task

wtfiwtz mentioned this pull request Jun 23, 2018

What is the upper bound of "imported new state entries"? #14647

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eth/downloader: separate state sync from queue #14460

eth/downloader: separate state sync from queue #14460

fjl commented May 11, 2017 •

edited

Loading

karalabe May 11, 2017

karalabe May 11, 2017

karalabe May 11, 2017

karalabe May 11, 2017

karalabe May 11, 2017

karalabe May 11, 2017

karalabe May 11, 2017

karalabe May 11, 2017

karalabe May 11, 2017

karalabe May 11, 2017

karalabe May 11, 2017

fjl commented May 29, 2017

Arachnid May 12, 2017

Arachnid May 12, 2017

karalabe May 30, 2017 •

edited

Loading

karalabe May 30, 2017

timtait commented Jun 20, 2017

fjl Jun 21, 2017

eth/downloader: separate state sync from queue #14460

eth/downloader: separate state sync from queue #14460

Conversation

fjl commented May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjl commented May 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karalabe May 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timtait commented Jun 20, 2017

Choose a reason for hiding this comment

fjl commented May 11, 2017 •

edited

Loading

karalabe May 30, 2017 •

edited

Loading