The StateRootsIterator assumes that all roots in state.state_roots are valid keys in the database.
This assumption is incorrect, because don't store states when there is a skip-slot. There are two places (I can think of) where we produce a state without storing any intermediate skipped states:
BeaconChain::process_block(..): If there are skipped slots between the block being processed and it's parent, per_slot_processing is used to advance the state and those intermediate states are not stored, but the final state is.BeaconChain::catchup_state(..): Here we produce a state that has been advanced to the latest slot (via skip slots) and we don't store any of those intermediate, skipped states. Notably, we don't actually store the produced state either, but it's still an issue because we may expect to iterate through the roots of that state.We also create intermediate states without storing them in BeaconChain::produce_block_on_state(..), however the state we produce is never stored and ephemeral, so I find this valid behavior.
A naive solution to (1) would involve storing each intermediate state as we produce it, however this is incorrect because we don't actually know the block is valid (and it's states should be stored) until later on. As such, we probably need to keep all the intermediate states in a Vec and then store them once we're certain the block is valid.
Regarding (2), this problem should go away if we implement my suggestions in #482.
As an alternative solution, could we make StateRootsIterator reconstruct the skipped states? It could iterate back until it finds a real state and then run forwards again, caching the results for future iterations. Sounds a bit complicated and slow though, and maintaining the invariant that if a state is stored in the database then all its ancestors are too is nicer anyway...
In that case I'm happy with both proposed solutions
As an alternative solution, could we make StateRootsIterator reconstruct the skipped states?
I don't think this works in all cases. For example, if we skip > SLOTS_PER_HISTORICAL_ROOT slots then we won't have any reference to a full-state.
This was fixed in some prior PR