les: implement new les fetcher #20692

rjl493456442 · 2020-02-19T11:26:56Z

This PR refactors the light/fetcher which reuses underlying eth/fetcher.

This PR is based on the #19991, would be nice to merge this one first.

les/fetcher.go

les/utils/expiredvalue.go

ericramos1980 · 2020-05-31T12:35:06Z

[email protected]

rjl493456442 · 2020-06-01T05:57:35Z

@zsfelfoldi I think I address all the comments, please take another look

les/fetcher.go

les/utils/expiredvalue.go

zsfelfoldi · 2020-06-02T21:07:10Z

les/fetcher.go

-		td     *big.Int
-	)
+		case resp := <-f.deliverCh:
+			if req := fetching[resp.reqid]; req != nil {


I realized there is one small thing missing from this fetcher logic that the old one had. If we download a header based on an announcement we should check that the Td and Number are actually the same as in the announcement and drop the peer if it does not match. Maybe we could include them in the request struct and check here upon delivery. The reason is that the monotonic Td announcement constraint protects the client against the server spamming it with announcements but it only makes sense if we actually check it. I am not sure whether it is a realistic attack vector now but adding the check would always ensure that the client won't accept a lot of bad announcements (because the server cannot forge a lot of headers with valid, monotonically increasing Td).

Well, it's a bit dangerous. The server can send some announces on the fork(but it thinks it's canonical one). Especially there are two blocks have the same td(miner can choose one randomly). Then we can't find the same announces

I add the check. If the hash is the same and is announced by the server, then check the number and td. Please check it

zsfelfoldi · 2020-06-06T16:16:04Z

les/fetcher.go

+			f.forEachPeer(func(id enode.ID, p *fetcherPeer) bool {
+				removed := p.forwardAnno(number)
+				for _, anno := range removed {
+					if header := f.chain.GetHeaderByHash(anno.data.Hash); header != nil {


At first I thought that this check is going to be simpler and we just need to add the check after delivering a header to the block fetcher by f.fetcher.FilterHeaders. Unfortunately that function returns before actually inserting the header to the chain so we still don't have the Td calculated. I guess this is why you are checking after evicting the announcements from the queue. This might work too but then I think you should also do this check in the other case when you are evicting the announcement because the queue is full (this is actually the more likely case when an attacker is spamming with bad announcements). Then we can be sure that causing the client to request useless headers by fake announcements is always punished. Either the hash is non-existent (in which case the retrieval will fail) or the hash exists but the number/Td does not match (in which case this check is always going to catch it when it gets out of the queue one way or the other).

Think about it. We have two cases to evict announces: (a) local chain has inserted some headers, to evict stale or useless announces (b) the announce queue is full.

In the latter one, we can't do any meaningful check since they are all "future" announces.

This is not necessarily true if the server is lying about the Td (which is the primary attack vector). If the headers exist but it is a worthless sidechain (or a fork like ETC) and the attacking server just keeps feeding announcements of it with fake high Tds then it is never evicted by forwardAnno since the Td appearing in the FIFO list is high (higher than the real chain). It will be evicted by addAnno where there is no check.

Actually it's still ok. Image the client keeps feeding super high td all the time, it will finally trigger a syncing. And the downloader will detect this announcement is invalid and drop this peer.

And also it's very hard to differentiate "fake announcement with high td" and "valid announcement because local client is out of sync". The main difference is for the latter one, we can finally sync to this point, but for the former, we will never reach the announcement point(it can be dropped by the downloader)

les/fetcher.go

les/server_handler.go

zsfelfoldi

Just a few details left, see comments.

les/fetcher.go

zsfelfoldi · 2020-06-09T20:30:23Z

les/fetcher.go

+			f.forEachPeer(func(id enode.ID, p *fetcherPeer) bool {
+				removed := p.forwardAnno(number)
+				for _, anno := range removed {
+					if header := f.chain.GetHeaderByHash(anno.data.Hash); header != nil {


This is not necessarily true if the server is lying about the Td (which is the primary attack vector). If the headers exist but it is a worthless sidechain (or a fork like ETC) and the attacking server just keeps feeding announcements of it with fake high Tds then it is never evicted by forwardAnno since the Td appearing in the FIFO list is high (higher than the real chain). It will be evicted by addAnno where there is no check.

les/fetcher.go

holiman

LGTM as far as the 'normal' import goes, haven't looked too deeply at the les-logic

karalabe · 2020-07-28T09:54:50Z

eth/fetcher/block_fetcher.go

@@ -330,11 +360,15 @@ func (f *BlockFetcher) loop() {
 				break
 			}
 			// Otherwise if fresh and still unknown, try and import
-			if number+maxUncleDist < height || f.getBlock(hash) != nil {
+			if number+maxUncleDist < height || f.light && f.getHeader(hash) != nil || !f.light && f.getBlock(hash) != nil {


Please add parentheses around the x && y groups. I know Go's convention is that && has a higher precedence than ||, but I really don't want to do mental gymnastics to figure out what comes before what else :)

karalabe · 2020-07-28T09:56:02Z

eth/fetcher/block_fetcher.go

 					// Pick a random peer to retrieve from, reset all others
 					announce := announces[rand.Intn(len(announces))]
 					f.forgetHash(hash)

 					// If the block still didn't arrive, queue for fetching
-					if f.getBlock(hash) == nil {
+					if f.light && f.getHeader(hash) == nil || !f.light && f.getBlock(hash) == nil {


Please add parentheses.

karalabe · 2020-07-28T10:02:47Z

eth/fetcher/block_fetcher.go

+			log.Debug("Unknown parent of propagated header", "peer", peer, "number", header.Number, "hash", hash, "parent", header.ParentHash)
+			return
+		}
+		// Quickly validate the header and propagate the block if it passes


This section is a bit weird, we don't have "quick propagation". Perhaps let's change it to a simple if

// Validate the header and if something went wrong, drop the peer if err := f.verifyHeader(header);; err != nil && err != consensus.ErrFutureBlock { log.Debug("Propagated header verification failed", "peer", peer, "number", header.Number, "hash", hash, "err", err) f.dropPeer(peer) return }

karalabe · 2020-07-28T14:38:10Z

Linter is red, some build error in the tests.

rjl493456442 · 2020-07-28T14:43:09Z

@karalabe the lint is super weird. It reports /home/travis/gopath/src/github.com/ethereum/go-ethereum/les/fetcher_test.go:69:80: too few arguments in call to newClientServerEnv. However I can run all the tests locally.

karalabe · 2020-07-28T14:58:40Z

Dafuq. It works for me locally too, even the linter is all green locally.

karalabe · 2020-07-28T15:02:18Z

Perhaps it's a merge issue on travis? Lemme just merge the thing and see if it blows up on the squashed commit.

* cmd, consensus, eth, les: implement light fetcher * les: address comment * les: address comment * les: address comments * les: check td after delivery * les: add linearExpiredValue for error counter * les: fix import * les: fix dead lock * les: order announces by td * les: encapsulate invalid counter * les: address comment * les: add more checks during the delivery * les: fix log * eth, les: fix lint * eth/fetcher: address comment

rjl493456442 requested review from holiman, karalabe and zsfelfoldi as code owners February 19, 2020 11:26

adamschmideg mentioned this pull request Feb 20, 2020

les: a potential circular wait of *lightFetcher.requestTrigger #20675

Closed

rjl493456442 mentioned this pull request Feb 25, 2020

geth in light mode hanging with 100% CPU #20464

Closed

rjl493456442 force-pushed the les-fetcher branch from 7e079fd to fb89cf2 Compare February 26, 2020 13:52

adamschmideg added the status:triage label Mar 23, 2020

rjl493456442 mentioned this pull request Mar 24, 2020

les: add checkpoint challenge for LES #20125

Closed

adamschmideg assigned zsfelfoldi and karalabe Mar 24, 2020

adamschmideg removed the status:triage label Mar 24, 2020

rjl493456442 mentioned this pull request Apr 14, 2020

les: add checkpoint challenge for light client #20807

Closed

rjl493456442 force-pushed the les-fetcher branch from 527d2e2 to 55d82bd Compare May 22, 2020 06:59

rjl493456442 requested review from fjl and gballet as code owners May 22, 2020 06:59

rjl493456442 force-pushed the les-fetcher branch from 55d82bd to df678e8 Compare May 24, 2020 12:16

zsfelfoldi requested changes May 29, 2020

View reviewed changes

rjl493456442 force-pushed the les-fetcher branch from df678e8 to 99dc0e2 Compare May 29, 2020 08:32

cmd, consensus, eth, les: implement light fetcher

81b721a

rjl493456442 force-pushed the les-fetcher branch from 99dc0e2 to 81b721a Compare May 29, 2020 08:34

les: address comment

814302c

rjl493456442 force-pushed the les-fetcher branch from 36f565c to 814302c Compare June 1, 2020 05:39

les: address comment

e5f49f8

zsfelfoldi reviewed Jun 2, 2020

View reviewed changes

rjl493456442 added 3 commits June 3, 2020 10:07

les: address comments

181a26b

les: check td after delivery

496a7b9

les: add linearExpiredValue for error counter

7a055b6

rjl493456442 force-pushed the les-fetcher branch from e3b1a12 to 9a3f2f4 Compare June 3, 2020 04:59

les: fix import

c66d8d9

rjl493456442 force-pushed the les-fetcher branch from 9a3f2f4 to c66d8d9 Compare June 3, 2020 05:01

les: fix dead lock

265801d

zsfelfoldi reviewed Jun 6, 2020

View reviewed changes

rjl493456442 added 2 commits June 8, 2020 11:38

les: order announces by td

88f267c

les: encapsulate invalid counter

c2208ff

zsfelfoldi reviewed Jun 9, 2020

View reviewed changes

rjl493456442 added 2 commits June 10, 2020 12:02

les: address comment

cf82e50

les: add more checks during the delivery

7f2feac

zsfelfoldi reviewed Jun 16, 2020

View reviewed changes

les/fetcher.go Outdated Show resolved Hide resolved

zsfelfoldi approved these changes Jun 16, 2020

View reviewed changes

les: fix log

547a140

holiman approved these changes Jun 17, 2020

View reviewed changes

eth, les: fix lint

a121f15

adamschmideg added the status:triage label Jul 21, 2020

adamschmideg unassigned zsfelfoldi Jul 28, 2020

adamschmideg removed the status:triage label Jul 28, 2020

karalabe reviewed Jul 28, 2020

View reviewed changes

eth/fetcher: address comment

965f87a

karalabe added this to the 1.9.19 milestone Jul 28, 2020

karalabe merged commit 28c5a8a into ethereum:master Jul 28, 2020

This was referenced Feb 24, 2021

[Upgrade] Go-Ethereum release v1.9.19 Consensys/quorum#1143

Closed

[Upgrade] Go-Ethereum release v1.9.19 Consensys/quorum#1146

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

les: implement new les fetcher #20692

les: implement new les fetcher #20692

rjl493456442 commented Feb 19, 2020 •

edited

Loading

ericramos1980 commented May 31, 2020

rjl493456442 commented Jun 1, 2020

zsfelfoldi Jun 2, 2020

rjl493456442 Jun 3, 2020

rjl493456442 Jun 3, 2020

zsfelfoldi Jun 6, 2020

rjl493456442 Jun 8, 2020

zsfelfoldi Jun 9, 2020

rjl493456442 Jun 10, 2020

rjl493456442 Jun 10, 2020

zsfelfoldi left a comment

zsfelfoldi Jun 9, 2020

holiman left a comment

karalabe Jul 28, 2020

karalabe Jul 28, 2020

karalabe Jul 28, 2020

karalabe commented Jul 28, 2020

rjl493456442 commented Jul 28, 2020

karalabe commented Jul 28, 2020

karalabe commented Jul 28, 2020

les: implement new les fetcher #20692

les: implement new les fetcher #20692

Conversation

rjl493456442 commented Feb 19, 2020 • edited Loading

ericramos1980 commented May 31, 2020

rjl493456442 commented Jun 1, 2020

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

zsfelfoldi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman left a 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

karalabe commented Jul 28, 2020

rjl493456442 commented Jul 28, 2020

karalabe commented Jul 28, 2020

karalabe commented Jul 28, 2020

rjl493456442 commented Feb 19, 2020 •

edited

Loading