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.

Sign up for GitHub

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

Jump to bottom

les: implement new les fetcher #20692

Merged
merged 15 commits into from
Jul 28, 2020
Merged

les: implement new les fetcher #20692

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
81b721a
cmd, consensus, eth, les: implement light fetcher
rjl493456442 May 24, 2020
814302c
les: address comment
rjl493456442 May 29, 2020
e5f49f8
les: address comment
rjl493456442 Jun 1, 2020
181a26b
les: address comments
rjl493456442 Jun 3, 2020
496a7b9
les: check td after delivery
rjl493456442 Jun 3, 2020
7a055b6
les: add linearExpiredValue for error counter
rjl493456442 Jun 3, 2020
c66d8d9
les: fix import
rjl493456442 Jun 3, 2020
265801d
les: fix dead lock
rjl493456442 Jun 5, 2020
88f267c
les: order announces by td
rjl493456442 Jun 8, 2020
c2208ff
les: encapsulate invalid counter
rjl493456442 Jun 8, 2020
cf82e50
les: address comment
rjl493456442 Jun 10, 2020
7f2feac
les: add more checks during the delivery
rjl493456442 Jun 10, 2020
547a140
les: fix log
rjl493456442 Jun 16, 2020
a121f15
eth, les: fix lint
rjl493456442 Jun 30, 2020
965f87a
eth/fetcher: address comment
rjl493456442 Jul 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 25 additions & 28 deletions les/fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@
package les

import (
"math/big"
"math/rand"
"sync"
"time"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/common/prque"
"github.com/ethereum/go-ethereum/consensus"
"github.com/ethereum/go-ethereum/core"
"github.com/ethereum/go-ethereum/core/rawdb"
Expand Down Expand Up @@ -68,46 +68,47 @@ type fetcherPeer struct {
latest *announceData // The latest announcement sent from the peer

// These following two fields can track the latest announces
// from the peer with limited size for caching.
// from the peer with limited size for caching. We hold the
// assumption that all enqueued announces are td-monotonic.
announces map[common.Hash]*announce // Announcement map
announceQueue *prque.Prque // Announcement queue
announcesList []common.Hash // FIFO announces list
}

// addAnno enqueues an new trusted announcement. If the queued announces overflow,
// evict from the oldest.
func (fp *fetcherPeer) addAnno(announce *announce) {
// Short circuit if the announce already exists. In normal case it should
// never happen since only monotonic announce is accepted. But the adversary
func (fp *fetcherPeer) addAnno(anno *announce) {
// Short circuit if the anno already exists. In normal case it should
// never happen since only monotonic anno is accepted. But the adversary
// may feed us fake announces with higher td but same hash. In this case,
// ignore the announce anyway.
hash, number := announce.data.Hash, announce.data.Number
// ignore the anno anyway.
hash := anno.data.Hash
if _, exist := fp.announces[hash]; exist {
return
}
fp.announces[hash] = announce
fp.announceQueue.Push(hash, -int64(number))
fp.announces[hash] = anno
fp.announcesList = append(fp.announcesList, hash)
rjl493456442 marked this conversation as resolved.
Show resolved Hide resolved

// Evict oldest if the announces are oversized.
for fp.announceQueue.Size() > cachedAnnosThreshold {
item, _ := fp.announceQueue.Pop()
delete(fp.announces, item.(common.Hash))
for len(fp.announcesList) > cachedAnnosThreshold {
delete(fp.announces, fp.announcesList[0])
fp.announcesList = fp.announcesList[1:]
}
}

// forwardAnno removes all announces from the map with a number lower than
rjl493456442 marked this conversation as resolved.
Show resolved Hide resolved
// the provided threshold.
func (fp *fetcherPeer) forwardAnno(number uint64) []*announce {
var ret []*announce
for !fp.announceQueue.Empty() {
item, priority := fp.announceQueue.Pop()
if uint64(-priority) > number {
fp.announceQueue.Push(item, priority)
func (fp *fetcherPeer) forwardAnno(td *big.Int) []*announce {
var evicted []*announce
for len(fp.announcesList) > 0 {
anno := fp.announces[fp.announcesList[0]]
if anno.data.Td.Cmp(td) > 0 {
break
}
ret = append(ret, fp.announces[item.(common.Hash)])
delete(fp.announces, item.(common.Hash))
evicted = append(evicted, anno)
rjl493456442 marked this conversation as resolved.
Show resolved Hide resolved
delete(fp.announces, anno.data.Hash)
fp.announcesList = fp.announcesList[1:]
}
return ret
return evicted
}

// lightFetcher implements retrieval of newly announced headers. It reuses
Expand Down Expand Up @@ -197,10 +198,7 @@ func (f *lightFetcher) registerPeer(p *serverPeer) {
f.plock.Lock()
defer f.plock.Unlock()

f.peers[p.ID()] = &fetcherPeer{
announces: make(map[common.Hash]*announce),
announceQueue: prque.New(nil),
}
f.peers[p.ID()] = &fetcherPeer{announces: make(map[common.Hash]*announce)}
}

// unregisterPeer removes the specified peer from the fetcher's peer set
Expand Down Expand Up @@ -382,12 +380,11 @@ func (f *lightFetcher) mainloop() {
continue
}
reset(ev.Block.Header())
number := localHead.Number.Uint64()

// Clean stale announcements from les-servers.
var droplist []enode.ID
f.forEachPeer(func(id enode.ID, p *fetcherPeer) bool {
removed := p.forwardAnno(number)
removed := p.forwardAnno(localTd)
for _, anno := range removed {
if header := f.chain.GetHeaderByHash(anno.data.Hash); header != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

if header.Number.Uint64() != anno.data.Number {
Expand Down