Loadblock returns err on invalid range #7811
Conversation
| // In the event where last valid block slot is less than current state slot, | ||
| // we can just process start state up to input slot. | ||
| if lastValidSlot >= startState.Slot() { | ||
| return processSlotsStateGen(ctx, startState, slot) | ||
| } |
There was a problem hiding this comment.
Is this comment correct? I can't make the connection between the text and the if statement.
…rysm into return-err-invalid-range
…rysm into return-err-invalid-range
| @@ -219,12 +219,24 @@ func (s *State) loadStateBySlot(ctx context.Context, slot uint64) (*state.Beacon | |||
| return nil, err | |||
| } | |||
|
|
|||
| // In the event where start state is greater or equal to requested slot, | |||
| // we should just return the start state. | |||
| if startState.Slot() >= slot { | |||
There was a problem hiding this comment.
wouldn't that be an error condition ?
prysm/beacon-chain/state/stategen/replay.go
Line 234 in 76b4ede
According to this method it should always be <= slot
There was a problem hiding this comment.
Agreed and this is not necessary. Removed
| // Gather the last saved block root and the slot number. | ||
| lastValidRoot, lastValidSlot, err := s.lastSavedBlock(ctx, slot) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "could not get last valid block for hot state using slot") | ||
| } | ||
|
|
||
| // In the event where current state slot is greater or equal to last valid block slot, | ||
| // we should just process state up to input slot. | ||
| if startState.Slot() >= lastValidSlot { |
There was a problem hiding this comment.
This also sounds like an impossible condition, no ? How would the state.Slot be larger than the block's slot. If there is
no block with that particular slot, its not possible to generate the state with that particular slot.
There was a problem hiding this comment.
agreed, but it's still better to be defensive here due to bugs from downstream functions. I added a comment to mention this is an impossible condition
There was a problem hiding this comment.
In that case shouldn't we fail over here and return an error ? Having something like this might mask a bigger issue.
There was a problem hiding this comment.
not necessarily, it just means the last available block slot is less than the start state slot. This serves as a short cut and also makes a lot of tests happy.
Example:
- request state with slot 200
- last valid state in DB given slot index is at slot 100
- last valid block in DB given slot index is <= slot 100
- in this case, we can short cut and advance state from slot 100 to 200 without running rest of the logic
What type of PR is this?
What does this PR do? Why is it needed?
Supersedes #7781
This PR fixes
LoadBlocksto return err in the event of input start slot > end slot. It also updates the test to conform to this standard. The previous written tests have cut corners and violated this assumptionsWhich issues(s) does this PR fix?
Fixes #7620
Other notes for review