op-node: Don't invoke p2p unsafe sync for old L2 blocks

[trianglesphere]

Description

Limits when the p2p unsafe sync gets invoked to make sure that we don't request old blocks.
There is another mechanism which will limit how often a p2p request is made, but it is not
guaranteed to work.

Tests

Unit tests.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 97a9a1c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[netlify]

✅ Deploy Preview for opstack-docs canceled.

Name	Link
🔨 Latest commit	97a9a1c
🔍 Latest deploy log	https://app.netlify.com/sites/opstack-docs/deploys/645af8bb17d1250008f03002

[ajsutton]

I'm missing some context here - why can't we request these blocks from p2p? From what I understand we shouldn't be using alt-sync at all if we're syncing from L1 (

optimism/op-node/rollup/driver/state.go

Lines 237 to 242 in 0d7859b

	// If the engine is not ready, or if the L2 head is actively changing, then reset the alt-sync:
	// there is no need to request L2 blocks when we are syncing already.
	if head := s.derivation.UnsafeL2Head(); head != lastUnsafeL2 || !s.derivation.EngineReady() {
	lastUnsafeL2 = head
	altSyncTicker.Reset(syncCheckInterval)
	}

should do that). As long as L1 is advancing we should either get new batches or reach the end of the sequence window - either way the L2 head advances and moves us within this range of blocks to fetch again. So we'd be hitting this limit only in the case of an extended L1 outage?
I can see that requesting really old blocks would likely cause a lot of blocks to be held in the quarantine as p2p sync walked backwards along the chain so some limit likely makes sense there, just want to properly understand the situations we'd hit that limit as it likely affects what sizing is right.

[trianglesphere]

I'm missing some context here - why can't we request these blocks from p2p? From what I understand we shouldn't be using alt-sync at all if we're syncing from L1

So there are two uses for alt-sync

1. Alt sync from a L2 op-geth URL. This uses a trusted url to pull blocks from & then insert them locally. It is more intended to doing a sync from scratch
2. P2P unsafe sync. This requests recent unsafe blocks so a node can keep up with the unsafe portion of the chain. The most common case is when a node restarts. It will miss some of the unsafe blocks that got published & won't be able to sync the new unsafe blocks it has received until the safe portion of the chain catches up.

I put the time check inside the P2P sync because the rpc sync should be able to request old blocks. For the P2P sync it should just fetch blocks from L1 unless they are unsafe. The alt sync ticker mostly does this, but I wanted to add a second check for this so we don't accidentally request blocks that we cannot use.

[ajsutton]

Ah yeah that makes sense to me. Looks good.

[mergify]

This PR has been added to the merge queue, and will be merged soon.

[mergify]

This PR is next in line to be merged, and will be merged as soon as checks pass.

[mergify]

This PR is next in line to be merged, and will be merged as soon as checks pass.