BlockTracker may panic on double close()

[ronnno]

https://github.com/orbs-network/orbs-network-go/blob/fc0cf472dcb249de489bc11be5553bd06f93f073/services/statestorage/block_tracker.go#L35

Two goroutines concurrently calling IncrementHeight() may attempt close the same channel twice causing panic.

We could:

• use a mutex to synchronize calls to the entire method. In this case we no longer atomic.AddUint64()
• add defer func() { recover() }() before the close(prevLatch). this will suppress possible panics.

The first option seems more straight forward and readable. But might also be less efficient. @electricmonk, what do you think?

[electricmonk]

depends on what are the odds of this actually happening. Which goroutine should "normally" call the code that in turn calls IncrementHeight()?

[ronnno]

chances are pretty low of this colliding. Calling service is BlockStorage in Block Creation and Intra-Node Sync flows.

In a single block service scenario it's highly unlikely. If we ever have multiple instances of BlockStorage in a single node this might be more likely.

[ronnno]

better solution:

use atomic.LoadPointer and atomic.SwatPointer as in this example: https://gist.github.com/zviadm/c234426882bfc8acba88f3503edaaa36#file-cond2-go

[ronnno]

decided on the simple (rw)mutex solution. we cannot avoid a mutex and this is the simplest mutex solution.

on the way, addressed another possible race condition in waiting for loop in WaitForBlock(): if block height is incremented between the for loop condition and the select's latch evaluation we might wait an additional block accidentally - or if we're unlucky even reach timeout even tough the desired block has already arrived).