fix(op-challenger): Absolute Prestate Provider Refactor by refcell · Pull Request #8453 · ethereum-optimism/optimism

refcell · 2023-12-05T21:28:32Z

Description

Refactors the absolute prestate logic out of the trace provider to hoist its construction to the registry.

Constructing the new PrestateProvider in the registry creator functions is more lightweight than
instantiating the trace provider and maintains separation of concerns, leaving the trace provider
construction in the trace package register functions.

This PR also splits genesis output root validation and absolute prestate commitment hash validation
logic in separate functions in a new source file with full test coverage.

refcell · 2023-12-05T21:28:44Z

Current dependencies on/for this PR:

develop
- PR feat(op-challenger): Output Bisection Game Genesis Output Root Fetching #8394
  - PR fix(op-challenger): Absolute Prestate Provider Refactor #8453 👈

This stack of pull requests is managed by Graphite.

semgrep-app · 2023-12-06T17:03:01Z

Semgrep found 1 todos_require_linear finding:

op-node/rollup/derive/calldata_source.go: L53

Please create a GitHub ticket for this TODO.

_{Ignore this finding from todos_require_linear.}

providers. fix(op-challenger): genesis output root validation for top of output bisection games genesis output root test

ajsutton

I think the basic approach of splitting out a prestate provider is good, but there's some confusion about what the prestate is that we'll need to fix up.

My suggestion would be to do this in a few small steps rather than stacking on the other PR. So the first thing would be to just pull the AbsolutePrestateCommitment function into a PrestateProvider interface and pull the existing functions from each TraceProvider implementation into a separate struct - like you've done with:

type CannonTraceProvider struct {
	CannonPrestateProvider

We can merge that as a simple refactoring to separate two concerns.

Then I think I'd look at introducing a ValidatePrestate method to the GamePlayer interface. There's some interesting design challenges to make the right components available there but I think it's where we need to get to so that validation is nicely encapsulated rather than having to be in register.go.

Assuming we can get there, it would then be easy to trigger the validation in coordinator when it creates the player.

ajsutton · 2023-12-06T20:22:16Z

op-challenger/game/fault/trace/outputs/prestate.go

+	}, nil
+}
+
+func NewPrestateProviderFromInputs(logger log.Logger, rollupClient OutputRollupClient, prestateBlock uint64) *OutputPrestateProvider {


This is unused so can probably be removed. And the logger param is unused if there's a good reason for keeping the method around (maybe it should be used in the test?)

ajsutton · 2023-12-06T20:28:19Z

op-challenger/game/fault/trace/outputs/prestate.go

+}
+
+func (o *OutputPrestateProvider) GenesisOutputRoot(ctx context.Context) (hash common.Hash, err error) {
+	return o.outputAtBlock(ctx, 0)


I don't think we want to check the genesis block at all. When the contracts say "genesis" they mean the absolute pre-state block which is the bedrock block on OP Mainnet.

ajsutton · 2023-12-06T20:30:10Z

op-challenger/game/fault/trace/outputs/prestate.go

+}
+
+func (o *OutputPrestateProvider) AbsolutePreStateCommitment(ctx context.Context) (hash common.Hash, err error) {
+	return o.outputAtBlock(ctx, o.prestateBlock)


This should be returning the prestate of the bottom half of the game (the prestate for the cannon or alphabet VM).

ajsutton · 2023-12-06T20:45:40Z

op-challenger/game/fault/validate.go

+func ValidateGenesisOutputRoot(ctx context.Context, provider types.PrestateProvider, loader HashLoader) error {
+	providerGenesisOutputRoot, err := provider.GenesisOutputRoot(ctx)
+	if err != nil {
+		return fmt.Errorf("failed to get the trace provider's genesis output root: %w", err)
+	}
+	onchainGenesisOutputRoot, err := loader(ctx)
+	if err != nil {
+		return fmt.Errorf("failed to get the onchain genesis output root: %w", err)
+	}
+	if !bytes.Equal(providerGenesisOutputRoot[:], onchainGenesisOutputRoot[:]) {
+		return fmt.Errorf("provider's genesis output root does not match onchain genesis output root: Provider: %s | Chain %s", providerGenesisOutputRoot.Hex(), onchainGenesisOutputRoot.Hex())
+	}
+	return nil
+}
+
+// ValidateAbsolutePrestate validates the absolute prestate of the fault game.
+func ValidateAbsolutePrestate(ctx context.Context, provider types.PrestateProvider, loader HashLoader) error {
+	providerPrestateHash, err := provider.AbsolutePreStateCommitment(ctx)
+	if err != nil {


I think these are fundamentally the same function. They're calling a different method on the provider, but that could just be passed in as a function instead of passing the interface but the comparison logic is all the same and would probably be good to deduplicate.

ajsutton · 2023-12-06T20:53:16Z

op-challenger/game/fault/trace/outputs/prestate_test.go

+	}, &rollupClient
+}
+
+func TestAbsolutePreStateCommitment(t *testing.T) {


We're missing tests for GenesisOutputRoot.

ajsutton · 2023-12-06T20:55:31Z

op-challenger/game/fault/types/types.go

+type PrestateProvider interface {
+	// AbsolutePreStateCommitment is the commitment of the pre-image value of the trace that transitions to the trace value at index 0
+	AbsolutePreStateCommitment(ctx context.Context) (hash common.Hash, err error)
+
+	// GenesisOutputRoot is the output root of the provider at genesis.
+	GenesisOutputRoot(ctx context.Context) (hash common.Hash, err error)
+}


I think this should just have AbsolutePreStateCommitment as its only method. For the output root bisection the absolute prestate commitment is the prestate block output root (which will be either genesis block or bedrock activation block). For the VM it's the pre-state hash.

So for a split game there are two trace providers (output roots and VM) and each has an absolute prestate that we need to verify.

ajsutton · 2023-12-06T21:02:06Z

op-challenger/game/fault/trace/outputs/output_alphabet.go

 ) (*trace.Accessor, error) {
 	bottomDepth := gameDepth - splitDepth
-	outputProvider, err := NewTraceProvider(ctx, logger, cfg.RollupRpc, splitDepth, prestateBlock, poststateBlock)
+	rollupClient, err := dial.DialRollupClientWithTimeout(ctx, dial.DefaultDialTimeout, logger, cfg.RollupRpc)


oof, we're not closing this properly and its the same in cannon. We'll need to fix that - logged https://github.com/ethereum-optimism/client-pod/issues/321
We can live with this code as is for now given it's already done that way for output_cannon anyway.

refcell · 2023-12-06T21:29:23Z

I think the basic approach of splitting out a prestate provider is good, but there's some confusion about what the prestate is that we'll need to fix up.

I don't think there is confusion about what the prestate is? For output bisection we need to verify both the absolute prestate commitment hash for the execution trace provider and the genesis output root for the top output provider. Looping in @clabby to verify but this isn't "confusion" :/

refcell self-assigned this Dec 5, 2023

refcell mentioned this pull request Dec 5, 2023

feat(op-challenger): Output Bisection Game Genesis Output Root Fetching #8394

Merged

refcell force-pushed the refcell/absolute-prestate-validation branch from 2696744 to 079a695 Compare December 6, 2023 00:35

refcell force-pushed the refcell/prestate-provider branch from df0ea57 to 1928de1 Compare December 6, 2023 00:35

feat(op-challenger): Validate absolute prestate for split trace

2ac6817

providers. fix(op-challenger): genesis output root validation for top of output bisection games genesis output root test

refcell force-pushed the refcell/absolute-prestate-validation branch from 079a695 to 2ac6817 Compare December 6, 2023 17:10

refcell requested a review from ajsutton December 6, 2023 17:11

fix(op-challenger): absolute prestate provider refactor

bb5a8b9

refcell force-pushed the refcell/prestate-provider branch from 3331ccf to bb5a8b9 Compare December 6, 2023 17:12

refcell marked this pull request as ready for review December 6, 2023 17:12

refcell requested a review from a team as a code owner December 6, 2023 17:12

refcell requested review from Inphi and clabby December 6, 2023 17:15

fix(op-e2e): prestate provider creation

772d397

ajsutton reviewed Dec 6, 2023

View reviewed changes

refcell force-pushed the refcell/absolute-prestate-validation branch 2 times, most recently from 197aba1 to 74a4849 Compare December 7, 2023 03:04

Base automatically changed from refcell/absolute-prestate-validation to develop December 7, 2023 03:40

refcell closed this Dec 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(op-challenger): Absolute Prestate Provider Refactor#8453

fix(op-challenger): Absolute Prestate Provider Refactor#8453
refcell wants to merge 3 commits intodevelopfrom
refcell/prestate-provider

refcell commented Dec 5, 2023 •

edited

Loading

Uh oh!

refcell commented Dec 5, 2023

Uh oh!

semgrep-app bot commented Dec 6, 2023

Uh oh!

ajsutton left a comment

Uh oh!

ajsutton Dec 6, 2023

Uh oh!

ajsutton Dec 6, 2023

Uh oh!

refcell Dec 6, 2023

Uh oh!

ajsutton Dec 6, 2023

Uh oh!

ajsutton Dec 6, 2023

Uh oh!

ajsutton Dec 6, 2023

Uh oh!

ajsutton Dec 6, 2023

Uh oh!

ajsutton Dec 6, 2023

Uh oh!

refcell commented Dec 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

refcell commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refcell commented Dec 5, 2023

Uh oh!

semgrep-app bot commented Dec 6, 2023

Uh oh!

ajsutton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refcell commented Dec 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

refcell commented Dec 5, 2023 •

edited

Loading