Fixed concurrency issue that allowed multiple miners instantiation.

[dmitry-worker]

Description

Concurrent EthashConsensus.startMiningProcess() method invocation can lead to several miner actors.
There's no thread mutual exclusion and atomicity between AtomicReference.get() and AtomicReference.set() calls.

Proposed Solution

synchronized or ReentrantReadWrite is recommended to ensure that only one miner is created.
AtomicReference is redundant, since no CAS-related methods are used.
Solution assumes that no thread synchronization is needed when miner already exists.

[mmrozek]

Good catch, but I think the simpler solution is to use atomicMiner.getAndUpdate. WDYT?

[dmitry-worker]

Good catch, but I think the simpler solution is to use atomicMiner.getAndUpdate. WDYT?
Thanks!

I was actually about to implement one of CompareAndSet-based methods, like getAndUpdate.
But one thing stops me doing that: the side effect that initializes a new Actor in the ActorSystem

            case Ethash => EthashMiner(node)
            case MockedPow => MockedMiner(node)

^ Those lines create a new actor in a system as a side effect.

Although getAndUpdate will guarantee that minerRef equals our newly created Actor at some point -
it cannot guarantee that no other miners are instantiated as well when the same method is called.
So, only mutual exclusion helped me to solve this :(

[lemastero]

locks/mutex/semaphores are low level, declarative programming approach to concurrencly. They are hard to maintain and super hard to write correctly.

Would it be possible to use sth like STM here. Maybe Ref from cats effect: https://typelevel.org/cats-effect/concurrency/ref.html

[lemastero]

Is it possible to write a test that will show the problem/solution?