From a0c7d0c177b421cdd14e052f720ab46c08ca3f42 Mon Sep 17 00:00:00 2001 From: Manav Darji Date: Mon, 15 Apr 2024 17:17:18 +0530 Subject: [PATCH] eth/downloader: bypass peer validation if remote peer is far away (#1219) * eth/downloader: bypass peer validation if remote peer is far away * eth/downloader: parameterise threshold, fix condition * eth: add tests for bypassing validation * eth/downloader: declare max validation threshold instead of using direct value * address comments * simplify --- eth/downloader/downloader.go | 57 ++++++++++++++++++++----------- eth/downloader/downloader_test.go | 34 ++++++++++++++++-- 2 files changed, 68 insertions(+), 23 deletions(-) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 22af24789a..a342927f7a 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -57,6 +57,8 @@ var ( fsHeaderSafetyNet = 2048 // Number of headers to discard in case a chain violation is detected fsHeaderContCheck = 3 * time.Second // Time interval to check for header continuations during state download fsMinFullBlocks = 64 // Number of blocks to retrieve fully even in snap sync + + maxValidationThreshold = uint64(1024) // Number of block difference from remote peer to start validation ) var ( @@ -147,7 +149,9 @@ type Downloader struct { quitCh chan struct{} // Quit channel to signal termination quitLock sync.Mutex // Lock to prevent double closes + // Validation ethereum.ChainValidator + maxValidationThreshold uint64 // Number of block difference from remote peer to start validation // Testing hooks syncInitHook func(uint64, uint64) // Method to call upon initiating a new sync run @@ -228,19 +232,20 @@ func New(stateDb ethdb.Database, mux *event.TypeMux, chain BlockChain, lightchai } dl := &Downloader{ - stateDB: stateDb, - mux: mux, - queue: newQueue(blockCacheMaxItems, blockCacheInitialItems), - peers: newPeerSet(), - blockchain: chain, - lightchain: lightchain, - dropPeer: dropPeer, - headerProcCh: make(chan *headerTask, 1), - quitCh: make(chan struct{}), - SnapSyncer: snap.NewSyncer(stateDb, chain.TrieDB().Scheme()), - stateSyncStart: make(chan *stateSync), - syncStartBlock: chain.CurrentSnapBlock().Number.Uint64(), - ChainValidator: whitelistService, + stateDB: stateDb, + mux: mux, + queue: newQueue(blockCacheMaxItems, blockCacheInitialItems), + peers: newPeerSet(), + blockchain: chain, + lightchain: lightchain, + dropPeer: dropPeer, + headerProcCh: make(chan *headerTask, 1), + quitCh: make(chan struct{}), + SnapSyncer: snap.NewSyncer(stateDb, chain.TrieDB().Scheme()), + stateSyncStart: make(chan *stateSync), + syncStartBlock: chain.CurrentSnapBlock().Number.Uint64(), + ChainValidator: whitelistService, + maxValidationThreshold: maxValidationThreshold, } // Create the post-merge skeleton syncer and start the process dl.skeleton = newSkeleton(stateDb, dl.peers, dropPeer, newBeaconBackfiller(dl, success)) @@ -893,13 +898,6 @@ func (d *Downloader) getFetchHeadersByNumber(p *peerConnection) func(number uint // In the rare scenario when we ended up on a long reorganisation (i.e. none of // the head links match), we do a binary search to find the common ancestor. func (d *Downloader) findAncestor(p *peerConnection, remoteHeader *types.Header) (uint64, error) { - // Check the validity of peer from which the chain is to be downloaded - if d.ChainValidator != nil { - if _, err := d.IsValidPeer(d.getFetchHeadersByNumber(p)); err != nil { - return 0, err - } - } - // Figure out the valid ancestor range to prevent rewrite attacks var ( floor = int64(-1) @@ -917,6 +915,25 @@ func (d *Downloader) findAncestor(p *peerConnection, remoteHeader *types.Header) localHeight = d.lightchain.CurrentHeader().Number.Uint64() } + // Check the validity of peer from which the chain is to be downloaded + if d.ChainValidator != nil { + _, err := d.IsValidPeer(d.getFetchHeadersByNumber(p)) + if errors.Is(err, whitelist.ErrMismatch) { + return 0, err + } + + if errors.Is(err, whitelist.ErrNoRemote) { + // Don't validate the peer against whitelisted milestones until the different of + // our local height and remote peer's height is less than `maxValidationThreshold` + if localHeight >= remoteHeight-d.maxValidationThreshold { + log.Info("Remote peer didn't respond", "id", p.id, "local", localHeight, "remote", remoteHeight, "err", err) + return 0, err + } + + log.Info("Remote peer didn't respond but is far ahead, skipping validation", "id", p.id, "local", localHeight, "remote", remoteHeight, "err", err) + } + } + p.log.Debug("Looking for common ancestor", "local", localHeight, "remote", remoteHeight) // Recap floor value for binary search diff --git a/eth/downloader/downloader_test.go b/eth/downloader/downloader_test.go index b248c860ec..e1ecd011e3 100644 --- a/eth/downloader/downloader_test.go +++ b/eth/downloader/downloader_test.go @@ -1649,14 +1649,42 @@ func TestFakedSyncProgress67NoRemoteCheckpoint(t *testing.T) { chainA := testChainForkLightA.blocks tester.newPeer("light", protocol, chainA[1:]) + // Set the max validation threshold equal to chain length to enforce validation + tester.downloader.maxValidationThreshold = uint64(len(chainA) - 1) + // Synchronise with the peer and make sure all blocks were retrieved // Should fail in first attempt - if err := tester.sync("light", nil, mode); err != nil { - assert.Equal(t, whitelist.ErrNoRemote, err, "failed synchronisation") - } + err := tester.sync("light", nil, mode) + assert.Equal(t, whitelist.ErrNoRemote, err, "failed synchronisation") // Try syncing again, should succeed if err := tester.sync("light", nil, mode); err != nil { t.Fatal("succeeded attacker synchronisation") } } + +// TestFakedSyncProgress67BypassWhitelistValidation tests if peer validation +// via whitelist is bypassed when remote peer is far away or not +func TestFakedSyncProgress67BypassWhitelistValidation(t *testing.T) { + protocol := uint(eth.ETH67) + mode := FullSync + + tester := newTester(t) + validate := func(count int) (bool, error) { + return false, whitelist.ErrNoRemote + } + + tester.downloader.ChainValidator = newWhitelistFake(validate) + + defer tester.terminate() + + // 1223 length chain + chainA := testChainBase.blocks + tester.newPeer("light", protocol, chainA[1:]) + + // Although the validate function above returns an error (which says that + // remote peer doesn't have that block), sync will go through as the chain + // import length is 1223 which is more than the default threshold of 1024 + err := tester.sync("light", nil, mode) + assert.NoError(t, err, "failed synchronisation") +}