From 7abaf28b5e807cb583275b5820fc3a61bbc7f5bb Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Wed, 13 Apr 2022 14:37:02 +0800 Subject: [PATCH 1/5] eth/downlaoder: resolve local header by hash for beacon sync --- eth/downloader/beaconsync.go | 13 +++++++++++-- eth/downloader/downloader.go | 27 +++++++++++++++++++++++---- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/eth/downloader/beaconsync.go b/eth/downloader/beaconsync.go index e2b13e99188e..21f133ce1bb4 100644 --- a/eth/downloader/beaconsync.go +++ b/eth/downloader/beaconsync.go @@ -254,7 +254,7 @@ func (d *Downloader) findBeaconAncestor() (uint64, error) { // fetchBeaconHeaders feeds skeleton headers to the downloader queue for scheduling // until sync errors or is finished. func (d *Downloader) fetchBeaconHeaders(from uint64) error { - head, _, err := d.skeleton.Bounds() + head, tail, err := d.skeleton.Bounds() if err != nil { return err } @@ -266,8 +266,17 @@ func (d *Downloader) fetchBeaconHeaders(from uint64) error { ) for i := 0; i < maxHeadersProcess && from <= head.Number.Uint64(); i++ { header := d.skeleton.Header(from) + + // The header is not found in skeleton space, try to resolve the header + // reversely via skeleton tail. Note we can't retrieve the header by + // number directly, since there is no guarantee it's the header we want. + if header == nil { + header = d.getAncestorHeader(from, tail) + } + // The header is still missing, the beacon sync is corrupted and bail out + // the error here. if header == nil { - header = d.lightchain.GetHeaderByNumber(from) + return fmt.Errorf("missing beacon header %d %x", tail.Number.Uint64()-1, tail.ParentHash) } headers = append(headers, header) hashes = append(hashes, headers[i].Hash()) diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 5ff5f32c4eeb..f1d510124883 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -159,9 +159,6 @@ 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 @@ -486,7 +483,8 @@ func (d *Downloader) syncWithPeer(p *peerConnection, hash common.Hash, td, ttd * // 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) + _, tail, _ := d.skeleton.Bounds() // error is already checked + pivot = d.getAncestorHeader(number, tail) } // Print an error log and return directly in case the pivot header // is still not found. It means the skeleton chain is not linked @@ -1772,3 +1770,24 @@ func (d *Downloader) DeliverSnapPacket(peer *snap.Peer, packet snap.Packet) erro return fmt.Errorf("unexpected snap packet type: %T", packet) } } + +// getAncestorHeader retrieves the header with given number. Different from obtaining +// the canonical header through the number directly, a verified tail header is used +// here as the base point and obtain the header eventually by resolving the parent. +func (d *Downloader) getAncestorHeader(number uint64, last *types.Header) *types.Header { + var current = last + for { + parent := d.lightchain.GetHeaderByHash(current.ParentHash) + if parent == nil { + break // The chain is not continuous + } + if parent.Number.Uint64() < number { + break // It shouldn't happen though + } + if parent.Number.Uint64() == number { + return parent + } + current = parent + } + return nil +} From c98ff83d2d05cb26dd36e85cb0f5b83520f39bd2 Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Wed, 13 Apr 2022 14:54:37 +0800 Subject: [PATCH 2/5] eth/downloader: fix error message --- eth/downloader/beaconsync.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eth/downloader/beaconsync.go b/eth/downloader/beaconsync.go index 21f133ce1bb4..c55e7be5128c 100644 --- a/eth/downloader/beaconsync.go +++ b/eth/downloader/beaconsync.go @@ -276,7 +276,7 @@ func (d *Downloader) fetchBeaconHeaders(from uint64) error { // The header is still missing, the beacon sync is corrupted and bail out // the error here. if header == nil { - return fmt.Errorf("missing beacon header %d %x", tail.Number.Uint64()-1, tail.ParentHash) + return fmt.Errorf("missing beacon header %d", from) } headers = append(headers, header) hashes = append(hashes, headers[i].Hash()) From 9986865ca8481cc28dc4f3b1aa5ee5674d0db751 Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Thu, 14 Apr 2022 13:54:18 +0800 Subject: [PATCH 3/5] eth/downloader: cap the reverse header resolving --- eth/downloader/beaconsync.go | 23 +++++++++++++++----- eth/downloader/downloader.go | 35 ++++++++++++++++++------------- eth/downloader/downloader_test.go | 6 +++--- 3 files changed, 42 insertions(+), 22 deletions(-) diff --git a/eth/downloader/beaconsync.go b/eth/downloader/beaconsync.go index c55e7be5128c..a242a4600f17 100644 --- a/eth/downloader/beaconsync.go +++ b/eth/downloader/beaconsync.go @@ -258,6 +258,18 @@ func (d *Downloader) fetchBeaconHeaders(from uint64) error { if err != nil { return err } + // A part of headers are not in the skeleton space, try to resolve + // them from the local chain. Note the range should be very short + // and it should only happen when there are less than 64 post-merge + // blocks in the network. + var localHeaders []*types.Header + if from < tail.Number.Uint64() { + count := tail.Number.Uint64() - from + if count > uint64(fsMinFullBlocks) { + return fmt.Errorf("invalid origin (%d) of beacon sync (%d)", from, tail.Number) + } + localHeaders = d.readHeaderRange(tail, int(count)) + } for { // Retrieve a batch of headers and feed it to the header processor var ( @@ -267,11 +279,12 @@ func (d *Downloader) fetchBeaconHeaders(from uint64) error { for i := 0; i < maxHeadersProcess && from <= head.Number.Uint64(); i++ { header := d.skeleton.Header(from) - // The header is not found in skeleton space, try to resolve the header - // reversely via skeleton tail. Note we can't retrieve the header by - // number directly, since there is no guarantee it's the header we want. - if header == nil { - header = d.getAncestorHeader(from, tail) + // The header is not found in skeleton space, try to find it in local chain. + if header == nil && from < tail.Number.Uint64() { + dist := tail.Number.Uint64() - from + if len(localHeaders) >= int(dist) { + header = localHeaders[dist-1] + } } // The header is still missing, the beacon sync is corrupted and bail out // the error here. diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index f1d510124883..6bad8e1fea3b 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -483,8 +483,14 @@ func (d *Downloader) syncWithPeer(p *peerConnection, hash common.Hash, td, ttd * // 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 { - _, tail, _ := d.skeleton.Bounds() // error is already checked - pivot = d.getAncestorHeader(number, tail) + _, oldest, _ := d.skeleton.Bounds() // error is already checked + if number < oldest.Number.Uint64() { + count := int(oldest.Number.Uint64() - number) // it's capped by fsMinFullBlocks + headers := d.readHeaderRange(oldest, count) + if len(headers) == count { + pivot = headers[len(headers)-1] + } + } } // Print an error log and return directly in case the pivot header // is still not found. It means the skeleton chain is not linked @@ -1771,23 +1777,24 @@ func (d *Downloader) DeliverSnapPacket(peer *snap.Peer, packet snap.Packet) erro } } -// getAncestorHeader retrieves the header with given number. Different from obtaining -// the canonical header through the number directly, a verified tail header is used -// here as the base point and obtain the header eventually by resolving the parent. -func (d *Downloader) getAncestorHeader(number uint64, last *types.Header) *types.Header { - var current = last +// readHeaderRange returns a list of headers, using the given last header as the base, +// and going backwards towards genesis. This method assumes that the caller already has +// placed a reasonable cap on count. +func (d *Downloader) readHeaderRange(last *types.Header, count int) []*types.Header { + var ( + current = last + headers []*types.Header + ) for { parent := d.lightchain.GetHeaderByHash(current.ParentHash) if parent == nil { - break // The chain is not continuous + break // The chain is not continuous, or the chain is exhausted } - if parent.Number.Uint64() < number { - break // It shouldn't happen though - } - if parent.Number.Uint64() == number { - return parent + headers = append(headers, parent) + if len(headers) >= count { + break } current = parent } - return nil + return headers } diff --git a/eth/downloader/downloader_test.go b/eth/downloader/downloader_test.go index 78380c2e8da0..0baa26d51af9 100644 --- a/eth/downloader/downloader_test.go +++ b/eth/downloader/downloader_test.go @@ -1385,10 +1385,10 @@ func testBeaconSync(t *testing.T, protocol uint, mode SyncMode) { 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 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}, + //{name: "Beacon sync with full local chain", local: blockCacheMaxItems - 15 - 1}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { From 4e5e43692447aeb84123d5927458143eaf6d7425 Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Thu, 14 Apr 2022 13:58:52 +0800 Subject: [PATCH 4/5] eth/downloader: re-enable tests --- eth/downloader/downloader_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/eth/downloader/downloader_test.go b/eth/downloader/downloader_test.go index 0baa26d51af9..78380c2e8da0 100644 --- a/eth/downloader/downloader_test.go +++ b/eth/downloader/downloader_test.go @@ -1385,10 +1385,10 @@ func testBeaconSync(t *testing.T, protocol uint, mode SyncMode) { 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 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}, + {name: "Beacon sync with full local chain", local: blockCacheMaxItems - 15 - 1}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { From 8ac432b95c5f85f039e3a3d4c86ddb471b3ba5d0 Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Thu, 14 Apr 2022 14:06:53 +0800 Subject: [PATCH 5/5] eth/downloader: add warning logs --- eth/downloader/beaconsync.go | 1 + eth/downloader/downloader.go | 1 + 2 files changed, 2 insertions(+) diff --git a/eth/downloader/beaconsync.go b/eth/downloader/beaconsync.go index a242a4600f17..d616e32c9642 100644 --- a/eth/downloader/beaconsync.go +++ b/eth/downloader/beaconsync.go @@ -269,6 +269,7 @@ func (d *Downloader) fetchBeaconHeaders(from uint64) error { return fmt.Errorf("invalid origin (%d) of beacon sync (%d)", from, tail.Number) } localHeaders = d.readHeaderRange(tail, int(count)) + log.Warn("Retrieved beacon headers from local", "from", from, "count", count) } for { // Retrieve a batch of headers and feed it to the header processor diff --git a/eth/downloader/downloader.go b/eth/downloader/downloader.go index 6bad8e1fea3b..0e7c48d8b739 100644 --- a/eth/downloader/downloader.go +++ b/eth/downloader/downloader.go @@ -489,6 +489,7 @@ func (d *Downloader) syncWithPeer(p *peerConnection, hash common.Hash, td, ttd * headers := d.readHeaderRange(oldest, count) if len(headers) == count { pivot = headers[len(headers)-1] + log.Warn("Retrieved pivot header from local", "number", pivot.Number, "hash", pivot.Hash(), "latest", latest.Number, "oldest", oldest.Number) } } }