Skip to content

[Tech Debt] too complicated setup for epoch-related consensus integration tests #8022

@AlexHentschel

Description

@AlexHentschel

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

// 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:

  1. 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.
  2. Combine createRootSnapshot and withNextEpoch. At least consolidates the logic of making this special snapshot.
  3. Maybe we can use EpochBuilder, pull out the latest snapshot, then just sign the Head block?

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions