eth/protocols/snap: snap sync testing

[holiman]

This is a work in progress - the tests now just hang, which isn't ideal. However, the tests do enable some fairly deep testing of the snap protocol, and the reason I rebased it again and am working on it, is to see if it can be used to trigger #22172.

Also, in general, I think it would be good to have these kinds of fairly high-level protocol tests.

[holiman]

I think this shows two errors, or, maybe let's call them "discussion points".

1. If cancel is invoked, the Sync does not exit with an error, but just exits with nil. Which seems strange, since the sync didn't complete successfully.
2. If a remote part is delivering storage tries in very small increments, the requestor eventually stalls. There are still storage tasks, however, the snippet below causes them to not be retrieved, for some reason:

		// Skip tasks that are already retrieving (or done with) all small states
		if len(task.SubTasks) == 0 && len(task.stateTasks) == 0 {
			continue
		}

3. The current implementation, when delivering storage keys, treats the limit a bit oddly. The server part aborts after a key has been added which goes above the limit. If a server instead respects the limit, the recipient complains that there are more elements available.

[holiman]

I pushed another fix, now there's only one remaining stall, in TestSyncWithStorageAndOneCappedPeer, where one peer delivers storage items, but only delivers it in max 500 bytes at a time.

[holiman]

@karalabe in 01e4846 , I added revertals to storage requests when they fail due to bad proofs. The testcases in this PR hit both those clauses individually, hence why they were added.

However, the same scenario applies for all types of requests, code, trie etc. Should we add revertals for all other cases when a response is invalid? And if so, maybe we should do it in a more generic fashion, instead of adding them one by one?

[holiman]

Another question.
In most places in sync.go, we update the bloom filter at the same time we write batch data to the database.

We do it in

• processStorageResponse
• processBytecodeResponse
• forwardAccountTask

We do not do it in

• processTrienodeHealResponse
• processBytecodeHealResponse

[karalabe]

This is wrong, pls revert. We need the request id (in id), not the peer's id.

[holiman]

Last remaining blocker:

$ go test . -run TriePanic
TRACE[01-20|15:53:31.702] Fetching range of accounts               id=nice-a reqid=4037200794235010051 root="c2661f…b0463a" origin="000000…000000" limit="0fffff…ffffff" bytes=512.00KiB
TRACE[01-20|15:53:31.702] Delivering range of accounts             id=nice-a reqid=4037200794235010051 hashes=1 accounts=1 proofs=1 bytes=210.00B
panic: interface conversion: trie.node is nil, not *trie.fullNode

goroutine 20 [running]:
github.com/ethereum/go-ethereum/trie.unsetInternal(0xa5c2a0, 0xc00007ed70, 0xc00002b9a0, 0x41, 0x41, 0xc00002b9f0, 0x41, 0x41, 0x20, 0xa5c1a0)
	/home/user/go/src/github.com/ethereum/go-ethereum/trie/proof.go:302 +0xd65
github.com/ethereum/go-ethereum/trie.VerifyRangeProof(0x6657bd231f66c2, 0x723c83101df021d9, 0xea1a61864a975b5e, 0x3a46b00c81ac76fa, 0xc00014a630, 0x20, 0x20, 0xc000352400, 0x20, 0x20, ...)
	/home/user/go/src/github.com/ethereum/go-ethereum/trie/proof.go:563 +0x7e5
github.com/ethereum/go-ethereum/eth/protocols/snap.(*Syncer).OnAccounts(0xc000147b00, 0xa62960, 0xc00016a3f0, 0x380704bb7b4d7c03, 0xc0003522a0, 0x1, 0x1, 0xc00034c140, 0x1, 0x1, ...)
	/home/user/go/src/github.com/ethereum/go-ethereum/eth/protocols/snap/sync.go:2033 +0x876
github.com/ethereum/go-ethereum/eth/protocols/snap.defaultAccountRequestHandler(0xc00016a3f0, 0x380704bb7b4d7c03, 0x6657bd231f66c2, 0x723c83101df021d9, 0xea1a61864a975b5e, 0x3a46b00c81ac76fa, 0x0, 0x0, 0x0, 0x0, ...)
	/home/user/go/src/github.com/ethereum/go-ethereum/eth/protocols/snap/sync_test.go:220 +0x11b
created by github.com/ethereum/go-ethereum/eth/protocols/snap.(*testPeer).RequestAccountRange
	/home/user/go/src/github.com/ethereum/go-ethereum/eth/protocols/snap/sync_test.go:162 +0x365

.. For now. And the question about the bloom filter above

[holiman]

@rjl493456442 do you have any good ideas on what the correct fix for the prover would be? The case is basically a very very small trie, consisting of a shortnode as root, iiuc.

[MariusVanDerWijden]

This might be a dumb comment, but everywhere else we need to hold a lock for accessing s especially s.snapped. The only instance I can find where loadSyncStatus() is called is in Line 528, but we don't hold the lock there anymore

[holiman]

Yeah it's a bit funky. loadSyncStatus touches a lot of internals in an unsafe way, and right after, the code accesses s.tasks.

	s.loadSyncStatus()
	if len(s.tasks) == 0 && s.healer.scheduler.Pending() == 0 {

So probably that lock-holding should be extended to cover that portion too.

I don't think it's a real issue, however, because I don't think we enter there in a concurrent way.

[karalabe]

AFAIK loadSyncStatus is only called when you create the thing ,so there's no concurrency at that point.

[holiman]

Rebased on top of @rjl493456442 's panic fix

[karalabe]

SGTM, although would be nice to look into how we could speed it up, .Currently it's enormously slow, even with parallel execution.