Skip to content

Commit

Permalink
bor: milestone avoid TryLock (#12422)
Browse files Browse the repository at this point in the history
`TryLock` - tells that mutex was locked - but doesn't tell by whom.
maybe by parent caller, maybe by another goroutine.

So next pattern is bad:
```
if m.finality.TryLock() {
   defer m.finality.Unlock()
}

// mutate
```
because mutex can be locked by another goroutine and our func will
mutate fields without waiting for mutex.

```
==================
WARNING: DATA RACE
Write at 0x00c004e64688 by goroutine 28583:
  github.com/erigontech/erigon/polygon/bor/finality/whitelist.(*milestone).UnlockSprint()
      github.com/erigontech/erigon/polygon/bor/finality/whitelist/milestone.go:175 +0x106
  github.com/erigontech/erigon/polygon/bor/finality.fetchWhitelistMilestone()
      github.com/erigontech/erigon/polygon/bor/finality/whitelist_helpers.go:113 +0xb3b
  github.com/erigontech/erigon/polygon/bor/finality.handleMilestone()
      github.com/erigontech/erigon/polygon/bor/finality/whitelist.go:202 +0xc8
  github.com/erigontech/erigon/polygon/bor/finality.retryHeimdallHandler()
      github.com/erigontech/erigon/polygon/bor/finality/whitelist.go:159 +0x5cb
  github.com/erigontech/erigon/polygon/bor/finality.RetryHeimdallHandler()
      github.com/erigontech/erigon/polygon/bor/finality/whitelist.go:117 +0x5c
  github.com/erigontech/erigon/polygon/bor/finality.startMilestoneWhitelistService()
      github.com/erigontech/erigon/polygon/bor/finality/whitelist.go:93 +0x2a
  github.com/erigontech/erigon/polygon/bor/finality.Whitelist.gowrap2()
      github.com/erigontech/erigon/polygon/bor/finality/whitelist.go:64 +0x17

Previous write at 0x00c004e64688 by goroutine 28584:
  github.com/erigontech/erigon/polygon/bor/finality/whitelist.(*milestone).RemoveMilestoneID()
      github.com/erigontech/erigon/polygon/bor/finality/whitelist/milestone.go:194 +0x164
  github.com/erigontech/erigon/polygon/bor/finality.handleNoAckMilestone()
      github.com/erigontech/erigon/polygon/bor/finality/whitelist.go:237 +0x12c
  github.com/erigontech/erigon/polygon/bor/finality.retryHeimdallHandler()
      github.com/erigontech/erigon/polygon/bor/finality/whitelist.go:159 +0x5cb
  github.com/erigontech/erigon/polygon/bor/finality.RetryHeimdallHandler()
      github.com/erigontech/erigon/polygon/bor/finality/whitelist.go:117 +0x57
  github.com/erigontech/erigon/polygon/bor/finality.startNoAckMilestoneService()
      github.com/erigontech/erigon/polygon/bor/finality/whitelist.go:102 +0x2a
  github.com/erigontech/erigon/polygon/bor/finality.Whitelist.gowrap3()
      github.com/erigontech/erigon/polygon/bor/finality/whitelist.go:65 +0x17
```
  • Loading branch information
AskAlexSharov authored Oct 23, 2024
1 parent 18bf69a commit c5b303e
Showing 1 changed file with 12 additions and 10 deletions.
22 changes: 12 additions & 10 deletions polygon/bor/finality/whitelist/milestone.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ func (m *milestone) Process(block uint64, hash common.Hash) {

whitelistedMilestoneMeter.SetUint64(block)

m.UnlockSprint(block)
m.unlockSprint(block)
}

// LockMutex This function will Lock the mutex at the time of voting
Expand All @@ -145,7 +145,7 @@ func (m *milestone) UnlockMutex(doLock bool, milestoneId string, endBlockNum uin
m.Locked = m.Locked || doLock

if doLock {
m.UnlockSprint(m.LockedMilestoneNumber)
m.unlockSprint(m.LockedMilestoneNumber)
m.Locked = true
m.LockedMilestoneHash = endBlockHash
m.LockedMilestoneNumber = endBlockNum
Expand All @@ -168,8 +168,16 @@ func (m *milestone) UnlockSprint(endBlockNum uint64) {
return
}

if m.finality.TryLock() {
defer m.finality.Unlock()
m.finality.Lock()
defer m.finality.Unlock()

m.unlockSprint(endBlockNum)
}

// UnlockSprint This function will unlock the locked sprint
func (m *milestone) unlockSprint(endBlockNum uint64) {
if endBlockNum < m.LockedMilestoneNumber {
return
}

m.Locked = false
Expand Down Expand Up @@ -233,12 +241,6 @@ func (m *milestone) GetMilestoneIDsList() []string {

// This is remove the milestoneIDs stored in the list.
func (m *milestone) purgeMilestoneIDsList() {
// try is used here as the finality lock is preserved over calls - so the lock state
// is not clearly defined in the local code - this likely needs to be revised
if m.finality.TryLock() {
defer m.finality.Unlock()
}

m.LockedMilestoneIDs = make(map[string]struct{})
}

Expand Down

0 comments on commit c5b303e

Please sign in to comment.