-
Notifications
You must be signed in to change notification settings - Fork 3.9k
op-acceptance-tests: add test to base gate for unsafe gossip / head progression #18031
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| package chain | ||
|
|
||
| import ( | ||
| "strings" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/ethereum-optimism/optimism/op-devstack/devtest" | ||
| "github.com/ethereum-optimism/optimism/op-devstack/presets" | ||
| "github.com/ethereum-optimism/optimism/op-supervisor/supervisor/types" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // TestUnsafeSync tests that the verifier nodes are able to sync the unsafe head of the chain. | ||
| // It does this by waiting for the sequencer to produce NUM_UNSAFE_BLOCKS unsafe blocks and requiring the verifier nodes to | ||
| // be progress to the final unsafe head number. | ||
| func TestUnsafeSync(gt *testing.T) { | ||
| t := devtest.SerialT(gt) | ||
| sys := presets.NewSingleChainMultiNode(t) | ||
|
|
||
| // Stop batcher so there is no sync from deriving from L1 | ||
| // This won't work on external networks. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I can tell, it calls
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In principle, yes -- but I think that |
||
| sys.L2Batcher.Stop() | ||
|
Comment on lines
+21
to
+23
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this test doesn't make sense to run on external networks, how does the acceptance test framework know to skip it? Is there some helper we should call, or preset we should use for that? @scharissis
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You'd do this with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be done per test or only per package?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe only per package currently.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok in that case I should probably pull this out into it's own package, since I don't want to avoid running the neighbouring tests on external devnets.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What prevents it from being run on an external devnet?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the control of the batcher, see other thread #18031 (comment). I think we could potentially work towards it running on an external network, if we can be sure that it is safe (in case someone points it at a production network) and won't interfere with other tests. |
||
|
|
||
| l2chainID := sys.L2Chain.Escape().ChainID() | ||
| const NUM_UNSAFE_BLOCKS = 10 | ||
| const TIMEOUT = 5 * time.Second // time to wait for all verifier nodes to catch up to the sequencer | ||
|
|
||
| // In order for this test to be valid, we need at least 2 L2 EL nodes. | ||
| t.Require().Greater(len(sys.L2Chain.L2ELNodes()), 1, "expected at least 2 L2 EL nodes") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should already be guaranteed by the preset when
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. I'm tempted to leave this in as a sanity check / protect against regressions though. |
||
|
|
||
| // Find the index of the sequencer node. | ||
| indexOfSequencer := 0 | ||
| for i, el := range sys.L2Chain.L2ELNodes() { | ||
| if strings.Contains(el.ID().String(), "sequencer") { | ||
| indexOfSequencer = i | ||
| break | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Instead of doing that check manually, can't we add a special case in the preset for the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| // Wait for the sequencer to produce at least NUM_UNSAFE_BLOCKS unsafe blocks. | ||
| initialUnsafeHeadNumber := sys.L2Chain.L2ELNodes()[0].ChainSyncStatus(sys.L2Chain.Escape().ChainID(), types.LocalUnsafe).Number | ||
| finalUnsafeHeadNumber := initialUnsafeHeadNumber + NUM_UNSAFE_BLOCKS | ||
| ticker := time.NewTicker(time.Duration(sys.L2Chain.Escape().RollupConfig().BlockTime) * time.Second) | ||
| for range ticker.C { | ||
| sequencerUnsafeHead := sys.L2Chain.L2ELNodes()[indexOfSequencer].ChainSyncStatus(sys.L2Chain.Escape().ChainID(), types.LocalUnsafe) | ||
| t.Logf("Sequencer unsafe head (number %d) is %d/%d blocks ahead of initial unsafe head (number %d)", | ||
| sequencerUnsafeHead.Number, sequencerUnsafeHead.Number-initialUnsafeHeadNumber, NUM_UNSAFE_BLOCKS, initialUnsafeHeadNumber) | ||
| if sequencerUnsafeHead.Number >= finalUnsafeHeadNumber { | ||
| break | ||
| } | ||
| } | ||
joshklop marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // Check verifier nodes progess to the final unsafe head number. | ||
| allVerifierNodesInSync := func() bool { | ||
| for i, el := range sys.L2Chain.L2ELNodes() { | ||
| if i == indexOfSequencer { | ||
| continue | ||
| } | ||
| verifierUnsafeHead := el.ChainSyncStatus(l2chainID, types.LocalUnsafe) | ||
| if verifierUnsafeHead.Number < initialUnsafeHeadNumber+NUM_UNSAFE_BLOCKS { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we check if the hash matches? Not sure if it matters in this case.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have |
||
| return false | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. quite possibly! I can have a play with those utils. |
||
| } | ||
| return true | ||
| } | ||
|
|
||
| require.Eventually(t, allVerifierNodesInSync, TIMEOUT, time.Duration(sys.L2Chain.Escape().RollupConfig().BlockTime)*time.Second, | ||
| "all verifier nodes did not progress to the final unsafe head number within %s", | ||
| TIMEOUT, | ||
| ) | ||
| t.Log("All verifier nodes progressed to the final unsafe head number", finalUnsafeHeadNumber) | ||
|
|
||
| // Final sanity check on the block hashes being consistent. | ||
scharissis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| finalUnsafeBlockHash := sys.L2Chain.L2ELNodes()[indexOfSequencer].BlockRefByNumber(finalUnsafeHeadNumber).Hash | ||
| for i, el := range sys.L2Chain.L2ELNodes() { | ||
| if i == indexOfSequencer { | ||
| continue | ||
| } | ||
| verifierUnsafeBlockHash := el.BlockRefByNumber(finalUnsafeHeadNumber).Hash | ||
| if verifierUnsafeBlockHash != finalUnsafeBlockHash { | ||
| t.Require().Fail("verifier (%s) unsafe block hash (%s) does not match final unsafe block hash (%s)", el.ID(), verifierUnsafeBlockHash, finalUnsafeBlockHash) | ||
| } | ||
| } | ||
| t.Log("All verifier nodes have the final unsafe block hash", finalUnsafeBlockHash) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using this for all base tests? Does it give us extra coverage in existing tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite possibly, yes!
Also, this is how you'd mark this test to be skipped on external devnets: