Add migration tests

[litt3]

Closes DAINT-376

• Uses an atomic bool for v2 dispersal, so it can be modified in tests
• Implements some simple migration tests

[samlaf]

First pass on architecture. Haven't looked at tests yet. We can chat offline regarding:

[samlaf]

bug: should this set commits.CertV2?

[litt3]

No- this is how the cert versions are defined

const (
	// EigenDA V1
	CertV0 EigenDACommitmentType = iota
	// EigenDA V2
	CertV1
)

Not crazy about this, I've just been carrying it forward

[samlaf]

oh.... kek. Should we rename? CertV0 makes no sense. We can still keep the iota numbering starting from 0. But at least the name will make sense. @ethenotethan thoughts?

[litt3]

I've no strong feelings for changing vs not. I've been generally trying (maybe failing) to keep diffs as small as possible, which is why I left it thus far

[samlaf]

why is this being removed here? was it just not used anymore? why?

Also right now we have like config/server_config under the config/ dir, but the metrics config is kept under metrics/. Seems a bit nonuniform.

I think I personally prefer keeping configs close to the structs they are used in. What was your motivation for moving configs to the high-level config/ dir?

[litt3]

Moving some configs to the config/ package was a consequence of how flags are being used

Since the flags for a given category are defined in the various packages, I was struggling with circular imports. It's possible there is a better package structure out there, I'm just not sure what it would be

[samlaf]

Ah I see. Prob just need to pop the config and flags in a subpackage of each respective package. Like server/config/config.go and server/flags/flags.go. I think that would resolve it. But also totally fine to leave as is.

[litt3]

Let's leave as-is for now, and keep in mind potential future proxy package structure improvements (maybe as part of merging proxy into core)

[samlaf]

Tests review

[samlaf]

let's try to refactor this function into smaller understandable parts. For example feels like we should be able to extract a "buildL1Block" function, but right now block1 and block2 are being built differently. Probably can refactor and extract out the FinalizationSignal in block2.

Also I personally think the require() should be moved out of this function and into tests. I prefer to see all the require statements when looking at a test. Helps see with a single look what exactly is being tested.

[litt3]

9bfbda9

Made a few tweaks, and left a note to improve further in the future

[ethenotethan]

Definitely out of scope for this PR but would be nice to remove this Boolean longterm in favor of some 'ProtocolVersion' enum - pseudo equivalent to how node software like geth maintains hard fork versioning

[samlaf]

Wouldnt that just require us to set an arbitrary timestamp at which the setDisperseV2 should happen?

[ethenotethan]

Wouldnt that just require us to set an arbitrary timestamp at which the setDisperseV2 should happen?

hmm you could introduce scheduling but feel it'd be unnecessary since the rollup itself is the control plane. Maybe for migrating arbitrum we could expose some admin API that the nodes would call when an ArbSysConfig update happens. This might be over-engineering though but definitely something to consider in the future if we introduce versioning with actual breaking changes where proxy has to have stricter switching between version subsections (e.g, if we change how secondary storage keys are generated). This opens a greater question though if there should be some delineation between the DA byte version and the actual EigenDA protocol version.

Currently we're mapping da byte 1:1 w/ EigenDA protocol but we need to bump this byte in the future irrespective of an EigenDA upgrade!

[ethenotethan]

How are we actually asserting that proxy is dual processing both V1 and V2 if we're only verifying dispersal/retrieval based on a generalized metrics vector. Would there be merit in extending the telemetry to also support a versioning field?

[samlaf]

We should def separate out the prometheus metrics regardless of whether its used in tests or not

[ethenotethan]

I'd argue for guaranteed migration test coverage its absolutely necessary

[litt3]

I agree, the assertions here just aren't strong enough.

I think it makes sense to create a follow-up PR, I created an issue

[ethenotethan]

Nit - does this actually need to be atomic? Iiuc there's isn't a current test case which distributes client load in parallel when switching the version flag

[samlaf]

needs to be atomic to prevent race condition from proxy reading it, while test writing to it. Prob innocuous race condition but still better to be race free

[litt3]

Making it atomic also keeps the door open for exposing the flag to users, if there would ever be a use case. Maybe not likely, but the atomicity cost is negligible at the rates we'll see here.

[ethenotethan]

Does this enact the entire derivation pipeline using the first L1 batch or does it just catch up from a previous batch state checkpoint?

[litt3]

I'm not sure, I just utilized what was already there for this test. Sam and I tried to go through and understand what precisely was happening in this test, but I didn't feel like we really arrived at a deep level of understanding.

[ethenotethan]

Is this testing that a migration can be applied against OP which re-derives from an inbox that contains both V1 and V2 certs or is it just applying the migration to do single step update/derive?

[litt3]

See above, I'm not super confident in my understanding of what these tests are doing.