eth/downloader: implement beacon sync

[karalabe]

No description provided.

[MariusVanDerWijden]

@karalabe Could you add this commit on top? MariusVanDerWijden@63a8982

[MariusVanDerWijden]

I've also added this diff to enable the sync (don't know if we want to do it in this pr or a follow-up)

diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go
index 1c3d65a1c..cd1d98a01 100644
--- a/eth/catalyst/api.go
+++ b/eth/catalyst/api.go
@@ -260,13 +260,14 @@ func (api *ConsensusAPI) ExecutePayloadV1(params ExecutableDataV1) (ExecutePaylo
                }
                return ExecutePayloadResponse{Status: VALID.Status, LatestValidHash: block.Hash()}, nil
        }
+       if b := api.eth.BlockChain().GetBlockByHash(params.BlockHash); b != nil && b.NumberU64() == block.NumberU64() {
+               log.Info("Ignoring already processed block", "number", params.Number, "hash", params.BlockHash)
+               return ExecutePayloadResponse{Status: VALID.Status, LatestValidHash: block.Hash()}, nil
+       }
        if !api.eth.BlockChain().HasBlock(block.ParentHash(), block.NumberU64()-1) {
-               /*
-                       TODO (MariusVanDerWijden) reenable once sync is merged
-                       if err := api.eth.Downloader().BeaconSync(api.eth.SyncMode(), block.Header()); err != nil {
-                               return SYNCING, err
-                       }
-               */
+               if err := api.eth.Downloader().BeaconSync(api.eth.SyncMode(), block.Header()); err != nil {
+                       return ExecutePayloadResponse{Status: SYNCING.Status, LatestValidHash: common.Hash{}}, err
+               }
                // TODO (MariusVanDerWijden) we should return nil here not empty hash
                return ExecutePayloadResponse{Status: SYNCING.Status, LatestValidHash: common.Hash{}}, nil
        }
        ```

[MariusVanDerWijden]

make the linter happy:

diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go
index 8f966229e..5e79e9a88 100644
--- a/eth/catalyst/api.go
+++ b/eth/catalyst/api.go
@@ -147,7 +147,7 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update beacon.ForkchoiceStateV1, pa
        }
        // If payload generation was requested, create a new block to be potentially
        // sealed by the beacon client. The payload will be requested later, and we
-       // might replace it arbitrarilly many times in between.
+       // might replace it arbitrarily many times in between.
        if payloadAttributes != nil {
                log.Info("Creating new payload for sealing")
                start := time.Now()
@@ -183,7 +183,7 @@ func (api *ConsensusAPI) ExecutePayloadV1(params beacon.ExecutableDataV1) (beaco
        if err != nil {
                return api.invalid(), err
        }
-       // If we alreayd have the block locally, ignore the entire execution and just
+       // If we already have the block locally, ignore the entire execution and just
        // return a fake success.
        if block := api.eth.BlockChain().GetBlockByHash(params.BlockHash); block != nil {
                log.Warn("Ignoring already known beacon payload", "number", params.Number, "hash", params.BlockHash, "age", common.PrettyAge(time.Unix(int64(block.Time()), 0)))
@@ -191,7 +191,7 @@ func (api *ConsensusAPI) ExecutePayloadV1(params beacon.ExecutableDataV1) (beaco
        }
        // If the parent is missing, we - in theory - could trigger a sync, but that
        // would also entail a reorg. That is problematic if multiple sibling blocks
-       // are being fed to us, and even moreso, if some semi-distant uncle shortens
+       // are being fed to us, and even more so, if some semi-distant uncle shortens
        // our live chain. As such, payload execution will not permit reorgs and thus
        // will not trigger a sync cycle. That is fine though, if we get a fork choice
        // update after legit payload executions.
diff --git a/eth/downloader/beaconsync.go b/eth/downloader/beaconsync.go
index 5afd7292b..2a2e0d6b0 100644
--- a/eth/downloader/beaconsync.go
+++ b/eth/downloader/beaconsync.go
@@ -275,7 +275,7 @@ func (d *Downloader) fetchBeaconHeaders(from uint64) error {
                        }
                }
                // State sync still going, wait a bit for new headers and retry
-               log.Trace("Pivot not yet comitted, waiting...")
+               log.Trace("Pivot not yet committed, waiting...")
                select {
                case <-time.After(fsHeaderContCheck):
                case <-d.cancelCh:
diff --git a/eth/downloader/skeleton.go b/eth/downloader/skeleton.go
index 67fe39d05..376bca898 100644
--- a/eth/downloader/skeleton.go
+++ b/eth/downloader/skeleton.go
@@ -238,7 +238,7 @@ func (s *skeleton) startup() {
 
        // Wait for startup or teardown. This wait might loop a few times if a beacon
        // client requests sync head extensions, but not forced reorgs (i.e. they are
-       // giving us new payloads without setting a starting head intially).
+       // giving us new payloads without setting a starting head initially).
        for {
                select {
                case errc := <-s.terminate:
diff --git a/eth/downloader/skeleton_test.go b/eth/downloader/skeleton_test.go
index 0aeabd87b..2d802c27d 100644
--- a/eth/downloader/skeleton_test.go
+++ b/eth/downloader/skeleton_test.go
@@ -71,7 +71,7 @@ func (hf *hookedBackfiller) resume() {
 }
 
 // skeletonTestPeer is a mock peer that can only serve header requests from a
-// pre-perated header chain (which may be arbitrarilly wrong for testing).
+// pre-perated header chain (which may be arbitrarily wrong for testing).
 //
 // Requesting anything else from these peers will hard panic. Note, do *not*
 // implement any other methods. We actually want to make sure that the skeleton
@@ -511,7 +511,7 @@ func TestSkeletonSyncRetrievals(t *testing.T) {
 
                head     *types.Header       // New head header to announce to reorg to
                peers    []*skeletonTestPeer // Initial peer set to start the sync with
-               midstate []*subchain         // Expected sync state after inital cycle
+               midstate []*subchain         // Expected sync state after initial cycle
                midserve uint64              // Expected number of header retrievals after initial cycle
                middrop  uint64              // Expectd number of peers dropped after initial cycle

[holiman]

Suggested change

	td = new(big.Int).Add(td, result.Header.Difficulty)
	td.Add(td, result.Header.Difficulty)

?

[karalabe]

I'm not a fan of that form as it's easy to misuse. This code isn't particularly critical, so I'm happier with a slightly more cleanlyness.

[holiman]

Suggested change

	td = new(big.Int).Add(td, rejected[0].Header.Difficulty)
	td.Add(td, rejected[0].Header.Difficulty)

?

[karalabe]

Maybe use something that doesn't clash with the meta entries.

[karalabe]

Myeah, changing this would cause all merge testnet nodes to blow up since they'd need a database upgrade (i.e. resync). I don't really see it as valuable as how much a PITA it is to change.

[holiman]

if the last chunkHeader is the final PoW-block, then len(rejected) will be 0, but we still would have wanted to enter here, no?

[karalabe]

My gut feeling is that we don't want to, since everything can be successfully imported. The error should be triggered only if you attempt to import something after the merge block. It should get triggered when the next batch of something is attempted to be imported.

[holiman]

The number reported here is the first rejected, so,
block-n, transition-block, invalid-block-1,..., the invalid-block-1 is what it will output. Seems not the right thing to output?

[karalabe]

Actually, that's exactly what I wanted to output. The block that's imported is not that relevant since we have import logs too as well as it's part of the chain so can be retrieved. What I'm interested to see is the details of the block that is rejected so I can look it up or investigate.

[holiman]

But -- if all our peers are well-behaving, they won't send us PoS blocks over the network, will they? Just want to avoid the scenario where we wait for the fist non-pow header to abort our cycle and "officially enter beacon mode", but our peers aren't delivering it because it's PoS.

[holiman]

As it's implemented here, the timeout is based on ttl := s.peers.rates.TargetTimeout(). So this might feasibly happen:

• One super-fast peer with good connection obtains a ttl of 5ms.
• 10 laggy peers obtain ttls of 500ms.
• The fast peer hits a slow-down, and takes 100ms for a delivery.
• We drop it the fast peer, and only keep the slow ones.

Haven't checked if the numbers I made up make any sense, but you probably understand the idea.

[karalabe]

The message rate tracker tries to track the median RTT across all peers. The timeout is the median RTT multiplied by some confidence number.

In your specific scenario, if you have one super fast peer and many laggy ones, the TTL will be the laggy value since that's the median. Even if your fast peer returns a bit slow once, it should still be fine.

[holiman]

Some nits, generally LGTM

[holiman]

But -- if all our peers are well-behaving, they won't send us PoS blocks over the network, will they? Just want to avoid the scenario where we wait for the fist non-pow header to abort our cycle and "officially enter beacon mode", but our peers aren't delivering it because it's PoS.

[holiman]

Suggested change

	if err == downloader.ErrMergeTransition && time.Since(cs.warned) > 10*time.Second {
	if errors.Is(err,downloader.ErrMergeTransition) && time.Since(cs.warned) > 10*time.Second {

[karalabe]

GitHub doesn't allow me to answer on this, weird:

But -- if all our peers are well-behaving, they won't send us PoS blocks over the network, will they? Just want to avoid the scenario where we wait for the fist non-pow header to abort our cycle and "officially enter beacon mode", but our peers aren't delivering it because it's PoS.

That's not the case. Here we're in the downloader and we're actively requesting data. The peers will deliver what we request. We still need to detect that what we've requested passed the merge threshold (we don't know in advance due to the TTD mechanism).