Skip to content

Unnecessary rewind of the executing engine in DecisionRewind case #71

@lucasmt

Description

@lucasmt

When progressInterop() returns a DecisionRewind, this indicates that the state of the Supernode needs to be rewound due to an invalid L1 head. In this case, applyRewindPlan() is called to perform this rewind. In addition to rewinding the VerifiedDB, LogsDB and DenyList, applyRewindPlan() also calls RewindEngine() on the ChainContainer:

if plan.ResetAllChainsTo != nil {
if err := chain.RewindEngine(i.ctx, *plan.ResetAllChainsTo, eth.BlockRef{}); err != nil {
i.log.Error("failed to reset chain engine on rewind", "chain", chainID, "err", err)
recordErr(fmt.Errorf("chain %s: reset chain engine on rewind: %w", chainID, err))
}
}

This call rewinds the executing engine via the EngineController to the previous timestamp. However, this is not actually necessary in this case, since the goal is to rewind the state of the Supernode to align with the current state of the chains, not the other way around. In fact, the DecisionRewind case indicates that the chains have suffered or are suffering a reorg, and it might be preferable to just keep re-trying the cross-validation loop until they have settled into a consistent state (and the Supernode has been rewound far back enough to be consistent with the current chain state).

Additionally, rewinding the engine in the DecisionRewind case brings up the following issues:

  • If applyRewindPlan() rewinds the engine but still fails because of an error when rewinding the LogsDB, the rewind will remain as a pending transition and re-attempted on the next iteration. In this case, the engine will be rewound again to the same block, repeating the procedure which involves two forkchoice updates (one to a synthetic block, and another back to the actual target block). Additionally, if there is the possibility that the chain has advanced beyond that block, or suffered a reorg to a different fork, it will be brought back unnecessarily.
  • As can be seen above, RewindEngine() is not actually called when plan.ResetAllChainsTo == nil. This happens in the case when the rewind would completely clear the VerifiedDB and LogsDB (and rewind back to before the activation timestamp), and is because if plan.ResetAllChainsTo == nil we lack information of what timestamp to rewind the engine back to. However, this is inconsistent: if we want to rewind the engine on DecisionRewind, it would be expected to also happen in this case. Avoiding rewinding the engine entirely would avoid this inconsistency.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions