Skip to content

Commit

Permalink
Fix map access panic (#343)
Browse files Browse the repository at this point in the history
## Describe your changes and provide context
- `CollectIteratorItems` needs to hold an RLock to avoid a concurrent
access panic

## Testing performed to validate your change
- Reproduced through a sei-chain-side test (concurrent instantiates)
  • Loading branch information
stevenlanders authored and udpatil committed Jan 2, 2024
1 parent 59519ed commit 64672e9
Showing 1 changed file with 33 additions and 12 deletions.
45 changes: 33 additions & 12 deletions store/multiversion/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ func (s *Store) SetEstimatedWriteset(index int, incarnation int, writeset WriteS
s.txWritesetKeys[index] = writeSetKeys
}

// GetWritesetKeys implements MultiVersionStore.
// GetAllWritesetKeys implements MultiVersionStore.
func (s *Store) GetAllWritesetKeys() map[int][]string {
s.mtx.RLock()
defer s.mtx.RUnlock()
Expand Down Expand Up @@ -243,10 +243,13 @@ func (s *Store) GetIterateset(index int) Iterateset {

// CollectIteratorItems implements MultiVersionStore. It will return a memDB containing all of the keys present in the multiversion store within the iteration range prior to (exclusive of) the index.
func (s *Store) CollectIteratorItems(index int) *db.MemDB {
s.mtx.RLock()
defer s.mtx.RUnlock()

sortedItems := db.NewMemDB()

// get all writeset keys prior to index
keys := s.GetAllWritesetKeys()
keys := s.txWritesetKeys
for i := 0; i < index; i++ {
indexedWriteset, ok := keys[i]
if !ok {
Expand Down Expand Up @@ -316,21 +319,27 @@ func (s *Store) validateIterator(index int, tracker iterationTracker) bool {
}
}

// TODO: do we want to return bool + []int where bool indicates whether it was valid and then []int indicates only ones for which we need to wait due to estimates? - yes i think so?
func (s *Store) ValidateTransactionState(index int) (bool, []int) {
defer telemetry.MeasureSince(time.Now(), "store", "mvs", "validate")
conflictSet := map[int]struct{}{}
valid := true
func (s *Store) checkIteratorAtIndex(index int) bool {
s.mtx.RLock()
defer s.mtx.RUnlock()

// TODO: can we parallelize for all iterators?
iterateset := s.GetIterateset(index)
valid := true
iterateset := s.txIterateSets[index]
for _, iterationTracker := range iterateset {
iteratorValid := s.validateIterator(index, iterationTracker)
valid = valid && iteratorValid
}
return valid
}

func (s *Store) checkReadsetAtIndex(index int) (bool, []int) {
s.mtx.RLock()
defer s.mtx.RUnlock()

conflictSet := make(map[int]struct{})
readset := s.txReadSets[index]
valid := true

// validate readset
readset := s.GetReadset(index)
// iterate over readset and check if the value is the same as the latest value relateive to txIndex in the multiversion store
for key, value := range readset {
// get the latest value from the multiversion store
Expand All @@ -357,16 +366,28 @@ func (s *Store) ValidateTransactionState(index int) (bool, []int) {
}
}

// convert conflictset into sorted indices
conflictIndices := make([]int, 0, len(conflictSet))
for index := range conflictSet {
conflictIndices = append(conflictIndices, index)
}

sort.Ints(conflictIndices)

return valid, conflictIndices
}

// TODO: do we want to return bool + []int where bool indicates whether it was valid and then []int indicates only ones for which we need to wait due to estimates? - yes i think so?
func (s *Store) ValidateTransactionState(index int) (bool, []int) {
defer telemetry.MeasureSince(time.Now(), "store", "mvs", "validate")

// TODO: can we parallelize for all iterators?
iteratorValid := s.checkIteratorAtIndex(index)

readsetValid, conflictIndices := s.checkReadsetAtIndex(index)

return iteratorValid && readsetValid, conflictIndices
}

func (s *Store) WriteLatestToStore() {
s.mtx.Lock()
defer s.mtx.Unlock()
Expand Down

0 comments on commit 64672e9

Please sign in to comment.