-
Notifications
You must be signed in to change notification settings - Fork 197
Description
Goal: Clean up the test setup for epoch-related consensus integration tests (in particular withNextEpoch
).
Importance: low priority (test is currently working, but requires disproportional repeated work each time we touch the test)
Context:
This issue is created to capture the resulting todos from this PR comment/discussion, copied below:
affected function
flow-go/consensus/integration/epoch_test.go
Lines 189 to 195 in b092086
// withNextEpoch adds a valid next epoch with the given identities to the input // snapshot. Also sets the length of the first (current) epoch to curEpochViews. // // We make the first (current) epoch start in committed phase so we can transition // to the next epoch upon reaching the appropriate view without any further changes // to the protocol state. func withNextEpoch(
[Yurii] I have fixed this function a bunch of times already and the more I see changes to it the more confusing it gets :)
Not a point for this PR but I would say we should rethink how those tests are setup. Maybe we should create a root snapshot with next epoch immediately because what we do here is kind of hacky(in a way that we can't obtain such snapshot from root block, there must be at least some blocks to incorporate service events).If you agree we could create a low-priority issue to simplify test setup. Maybe instead of modifying root snapshot we just create new one similarly how we create root snapshot. Idk, need to think.
[Jordan] I agree this test setup has become pretty gnarly 😓
I think the problem stems from a few things:
- this test is constructing a strange root snapshot which contains service events for multiple epochs (in some sense, an invalid root snapshot)
- the rest of our logic and testing code does not have a way to handle this kind of root snapshot
- this test has a stronger requirement on the test data than other tests (must have valid QCs)
Usually when we want to instantiate a test case with a snapshot from a non-root epoch or phase, we use
EpochBuilder
, but we can't here because we need the built blocks to have valid QCs.Briefly noting some thoughts on approaches that might help clean this up:
- Modify the general
Snapshot
fixture creation to be more flexible and allow root snapshots with more epoch info. I don't think we should do this, because in almost all cases the resulting snapshot would be considered invalid by our actual business logic.- Combine
createRootSnapshot
andwithNextEpoch
. At least consolidates the logic of making this special snapshot.- Maybe we can use
EpochBuilder
, pull out the latest snapshot, then just sign theHead
block?