From 91ed02e8b3f9bce24519254f1a835becc49eb396 Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Wed, 30 Mar 2022 13:23:14 +0800 Subject: [PATCH 1/5] eth/downloader: retrieve pivot header from local chain if necessary --- eth/downloader/downloader.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index ebd414105f42..1f60133abfe4 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -158,6 +158,9 @@ type LightChain interface { // GetHeaderByHash retrieves a header from the local chain. GetHeaderByHash(common.Hash) *types.Header + // GetHeaderByNumber retrieves a block header from the local chain by number. + GetHeaderByNumber(number uint64) *types.Header + // CurrentHeader retrieves the head header from the local chain. CurrentHeader() *types.Header @@ -477,15 +480,22 @@ func (d *Downloader) syncWithPeer(p *peerConnection, hash common.Hash, td, ttd * return err } if latest.Number.Uint64() > uint64(fsMinFullBlocks) { - pivot = d.skeleton.Header(latest.Number.Uint64() - uint64(fsMinFullBlocks)) + number := latest.Number.Uint64() - uint64(fsMinFullBlocks) + pivot = d.skeleton.Header(number) + if pivot == nil { + // Retrieve the pivot point from the local chain if it's + // not in the range of skeleton. + pivot = d.lightchain.GetHeaderByNumber(number) + } } } // If no pivot block was returned, the head is below the min full block // threshold (i.e. new chain). In that case we won't really snap sync // anyway, but still need a valid pivot block to avoid some code hitting // nil panics on access. + var noSnap bool if mode == SnapSync && pivot == nil { - pivot = d.blockchain.CurrentBlock().Header() + pivot, noSnap = d.blockchain.CurrentBlock().Header(), true } height := latest.Number.Uint64() @@ -516,7 +526,10 @@ func (d *Downloader) syncWithPeer(p *peerConnection, hash common.Hash, td, ttd * origin = 0 } else { pivotNumber := pivot.Number.Uint64() - if pivotNumber <= origin { + + // Only cap the sync origin number by pivot point when + // state sync is required. + if pivotNumber <= origin && !noSnap { origin = pivotNumber - 1 } // Write out the pivot into the database so a rollback beyond it will From 8f8cba6d228c81d12775c6bc4ef9e15138f0e448 Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Thu, 31 Mar 2022 11:53:07 +0800 Subject: [PATCH 2/5] eth/downloader: improve readability --- eth/downloader/downloader.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 1f60133abfe4..31b35c45d886 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -493,9 +493,9 @@ func (d *Downloader) syncWithPeer(p *peerConnection, hash common.Hash, td, ttd * // threshold (i.e. new chain). In that case we won't really snap sync // anyway, but still need a valid pivot block to avoid some code hitting // nil panics on access. - var noSnap bool + var syncState = true // means whether state sync is required in this cycle if mode == SnapSync && pivot == nil { - pivot, noSnap = d.blockchain.CurrentBlock().Header(), true + pivot, syncState = d.blockchain.CurrentBlock().Header(), false } height := latest.Number.Uint64() @@ -527,9 +527,8 @@ func (d *Downloader) syncWithPeer(p *peerConnection, hash common.Hash, td, ttd * } else { pivotNumber := pivot.Number.Uint64() - // Only cap the sync origin number by pivot point when - // state sync is required. - if pivotNumber <= origin && !noSnap { + // Cap the sync origin by pivot header when state sync is required. + if pivotNumber <= origin && syncState { origin = pivotNumber - 1 } // Write out the pivot into the database so a rollback beyond it will From b1b569061454082c2a2f1106e542f9088de74cdb Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Fri, 1 Apr 2022 11:55:40 +0800 Subject: [PATCH 3/5] eth/downloader: update fix --- eth/downloader/beaconsync.go | 6 +++++- eth/downloader/downloader.go | 23 ++++++++++++++--------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/eth/downloader/beaconsync.go b/eth/downloader/beaconsync.go index d8ea58c239fc..e2b13e99188e 100644 --- a/eth/downloader/beaconsync.go +++ b/eth/downloader/beaconsync.go @@ -265,7 +265,11 @@ func (d *Downloader) fetchBeaconHeaders(from uint64) error { hashes = make([]common.Hash, 0, maxHeadersProcess) ) for i := 0; i < maxHeadersProcess && from <= head.Number.Uint64(); i++ { - headers = append(headers, d.skeleton.Header(from)) + header := d.skeleton.Header(from) + if header == nil { + header = d.lightchain.GetHeaderByNumber(from) + } + headers = append(headers, header) hashes = append(hashes, headers[i].Hash()) from++ } diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 31b35c45d886..5ff5f32c4eeb 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -78,6 +78,7 @@ var ( errCanceled = errors.New("syncing canceled (requested)") errTooOld = errors.New("peer's protocol version too old") errNoAncestorFound = errors.New("no common ancestor found") + errNoPivotHeader = errors.New("pivot header is not found") ErrMergeTransition = errors.New("legacy sync reached the merge") ) @@ -481,21 +482,27 @@ func (d *Downloader) syncWithPeer(p *peerConnection, hash common.Hash, td, ttd * } if latest.Number.Uint64() > uint64(fsMinFullBlocks) { number := latest.Number.Uint64() - uint64(fsMinFullBlocks) - pivot = d.skeleton.Header(number) - if pivot == nil { - // Retrieve the pivot point from the local chain if it's - // not in the range of skeleton. + + // Retrieve the pivot header from the skeleton chain segment but + // fallback to local chain if it's not found in skeleton space. + if pivot = d.skeleton.Header(number); pivot == nil { pivot = d.lightchain.GetHeaderByNumber(number) } + // Print an error log and return directly in case the pivot header + // is still not found. It means the skeleton chain is not linked + // correctly with local chain. + if pivot == nil { + log.Error("Pivot header is not found", "number", number) + return errNoPivotHeader + } } } // If no pivot block was returned, the head is below the min full block // threshold (i.e. new chain). In that case we won't really snap sync // anyway, but still need a valid pivot block to avoid some code hitting // nil panics on access. - var syncState = true // means whether state sync is required in this cycle if mode == SnapSync && pivot == nil { - pivot, syncState = d.blockchain.CurrentBlock().Header(), false + pivot = d.blockchain.CurrentBlock().Header() } height := latest.Number.Uint64() @@ -526,9 +533,7 @@ func (d *Downloader) syncWithPeer(p *peerConnection, hash common.Hash, td, ttd * origin = 0 } else { pivotNumber := pivot.Number.Uint64() - - // Cap the sync origin by pivot header when state sync is required. - if pivotNumber <= origin && syncState { + if pivotNumber <= origin { origin = pivotNumber - 1 } // Write out the pivot into the database so a rollback beyond it will From 8d651cde40459cfe9f1a47f9e38e54a07335945e Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Fri, 1 Apr 2022 15:11:57 +0800 Subject: [PATCH 4/5] eth/downloader: add beacon sync tests --- eth/downloader/downloader_test.go | 73 +++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/eth/downloader/downloader_test.go b/eth/downloader/downloader_test.go index 6989252c11ac..d51696654b0b 100644 --- a/eth/downloader/downloader_test.go +++ b/eth/downloader/downloader_test.go @@ -79,6 +79,31 @@ func newTester() *downloadTester { return tester } +// newTester creates a new downloader test mocker. +func newTesterWithNotification(success func()) *downloadTester { + freezer, err := ioutil.TempDir("", "") + if err != nil { + panic(err) + } + db, err := rawdb.NewDatabaseWithFreezer(rawdb.NewMemoryDatabase(), freezer, "", false) + if err != nil { + panic(err) + } + core.GenesisBlockForTesting(db, testAddress, big.NewInt(1000000000000000)) + + chain, err := core.NewBlockChain(db, nil, params.TestChainConfig, ethash.NewFaker(), vm.Config{}, nil, nil) + if err != nil { + panic(err) + } + tester := &downloadTester{ + freezer: freezer, + chain: chain, + peers: make(map[string]*downloadTesterPeer), + } + tester.downloader = New(0, db, new(event.TypeMux), tester.chain, nil, tester.dropPeer, success) + return tester +} + // terminate aborts any operations on the embedded downloader and releases all // held resources. func (dl *downloadTester) terminate() { @@ -1368,3 +1393,51 @@ func testCheckpointEnforcement(t *testing.T, protocol uint, mode SyncMode) { assertOwnChain(t, tester, len(chain.blocks)) } } + +// Tests that peers below a pre-configured checkpoint block are prevented from +// being fast-synced from, avoiding potential cheap eclipse attacks. +func TestBeaconSync66Full(t *testing.T) { testBeaconSync(t, eth.ETH66, FullSync) } +func TestBeaconSync66Snap(t *testing.T) { testBeaconSync(t, eth.ETH66, SnapSync) } + +func testBeaconSync(t *testing.T, protocol uint, mode SyncMode) { + //log.Root().SetHandler(log.LvlFilterHandler(log.LvlInfo, log.StreamHandler(os.Stderr, log.TerminalFormat(true)))) + + var cases = []struct { + name string // The name of testing scenario + local int // The length of local chain(canonical chain assumed), 0 means genesis is the head + }{ + {name: "Beacon sync since genesis", local: 0}, + {name: "Beacon sync with short local chain", local: 1}, + {name: "Beacon sync with long local chain", local: blockCacheMaxItems - 15 - fsMinFullBlocks/2}, + {name: "Beacon sync with full local chain", local: blockCacheMaxItems - 15 - 1}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + success := make(chan struct{}) + tester := newTesterWithNotification(func() { + close(success) + }) + defer tester.terminate() + + chain := testChainBase.shorten(blockCacheMaxItems - 15) + tester.newPeer("peer", protocol, chain.blocks[1:]) + + // Build the local chain segment if it's required + if c.local > 0 { + tester.chain.InsertChain(chain.blocks[1 : c.local+1]) + } + if err := tester.downloader.BeaconSync(mode, chain.blocks[len(chain.blocks)-1].Header()); err != nil { + t.Fatalf("Failed to beacon sync chain %v %v", c.name, err) + } + select { + case <-success: + // Ok, downloader fully cancelled after sync cycle + if bs := int(tester.chain.CurrentBlock().NumberU64()) + 1; bs != len(chain.blocks) { + t.Fatalf("synchronised blocks mismatch: have %v, want %v", bs, len(chain.blocks)) + } + case <-time.NewTimer(time.Second * 3).C: + t.Fatalf("Failed to sync chain in three seconds") + } + }) + } +} From b59d235e1c94568c60a5210059fddcd69a96b448 Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Fri, 1 Apr 2022 15:18:06 +0800 Subject: [PATCH 5/5] eth/downloader: remove duplicated code --- eth/downloader/downloader_test.go | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-) diff --git a/eth/downloader/downloader_test.go b/eth/downloader/downloader_test.go index d51696654b0b..d11315dc1324 100644 --- a/eth/downloader/downloader_test.go +++ b/eth/downloader/downloader_test.go @@ -56,27 +56,7 @@ type downloadTester struct { // newTester creates a new downloader test mocker. func newTester() *downloadTester { - freezer, err := ioutil.TempDir("", "") - if err != nil { - panic(err) - } - db, err := rawdb.NewDatabaseWithFreezer(rawdb.NewMemoryDatabase(), freezer, "", false) - if err != nil { - panic(err) - } - core.GenesisBlockForTesting(db, testAddress, big.NewInt(1000000000000000)) - - chain, err := core.NewBlockChain(db, nil, params.TestChainConfig, ethash.NewFaker(), vm.Config{}, nil, nil) - if err != nil { - panic(err) - } - tester := &downloadTester{ - freezer: freezer, - chain: chain, - peers: make(map[string]*downloadTesterPeer), - } - tester.downloader = New(0, db, new(event.TypeMux), tester.chain, nil, tester.dropPeer, nil) - return tester + return newTesterWithNotification(nil) } // newTester creates a new downloader test mocker.