Remove stale expiries in AdvanceTimeTo #3415
Conversation
| newChainTimeUnix := uint64(newChainTime.Unix()) | ||
| for expiryIterator.Next() { | ||
| expiry := expiryIterator.Value() | ||
| if expiry.Timestamp > newChainTimeUnix { |
There was a problem hiding this comment.
(No action required) Maybe add a comment indicating that expiryIterator is ordered ascending by Timestamp such that the first unexpired Expiry implies that all subsequent Expiries are also unexpired?
| // Promote any pending stakers to current if [StartTime] <= [newChainTime]. | ||
| // | ||
| // Invariant: It is not safe to modify the state while iterating over it, | ||
| // so we use the parentState's iterator rather than the changes iterator. | ||
| // ParentState must not be modified before this iterator is released. | ||
| pendingStakerIterator, err := parentState.GetPendingStakerIterator() | ||
| if err != nil { | ||
| return false, err | ||
| return nil, false, err | ||
| } | ||
| defer pendingStakerIterator.Release() |
There was a problem hiding this comment.
This was perilously close to being an existing race.
The iterator is released in a defer, but the changes are applied on the return line. The defer is executed after the return line is evaluated. Which previously broke the invariant (that is now documented). However, because we only ever passed in a state.Diff into this function and the state.Diff lazily allocates the btree's that are iterated over, the returned iterator wasn't a tree iterator, but just explicitly empty... Which does support writes during iteration.
The expiry tree is allocated during diff initialization, which is why this function showed a race when adding the expiry iterator.
By moving the state application to the parent caller, the defer executes before Apply and there is no chance of a race.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Why this should be merged
In ACP-77,
RegisterValidatorwarp messages use time expiry to prevent replay attacks. This is implemented using an expiry set. This PR introduces the removal of stale expiry entries duringAdvanceTimeTo.How this works
The timestamp in an expiry is when that value is no longer valid to be added to the chain. Ref
Note: This change doesn't need an
IsEtnaActivatedflag becauseExpiriesare only added afterEtnais activated. And if there are no expiries, the code is a noop (as tested)How this was tested