From e7f8cb2ed11474a407be0cf8112a92791758cb10 Mon Sep 17 00:00:00 2001 From: dmitry-worker Date: Thu, 5 Nov 2020 16:01:36 +0300 Subject: [PATCH 1/2] Fixed concurrency issue to ensure that only one miner is created. --- .../consensus/ethash/EthashConsensus.scala | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/src/main/scala/io/iohk/ethereum/consensus/ethash/EthashConsensus.scala b/src/main/scala/io/iohk/ethereum/consensus/ethash/EthashConsensus.scala index d643bcfdac..fa7bf990a8 100644 --- a/src/main/scala/io/iohk/ethereum/consensus/ethash/EthashConsensus.scala +++ b/src/main/scala/io/iohk/ethereum/consensus/ethash/EthashConsensus.scala @@ -2,7 +2,6 @@ package io.iohk.ethereum package consensus package ethash -import java.util.concurrent.atomic.AtomicReference import akka.actor.ActorRef import akka.util.Timeout import io.iohk.ethereum.consensus.Protocol._ @@ -48,34 +47,33 @@ class EthashConsensus private ( blockchainConfig = blockchainConfig ) - private[this] val atomicMiner = new AtomicReference[Option[ActorRef]](None) + @volatile private[this] var minerRef: Option[ActorRef] = None private implicit val timeout: Timeout = 5.seconds - override def sendMiner(msg: MinerProtocol): Unit = { - atomicMiner - .get() - .foreach(_ ! msg) - } + override def sendMiner(msg: MinerProtocol): Unit = + minerRef.foreach(_ ! msg) override def askMiner(msg: MinerProtocol): Task[MinerResponse] = { - atomicMiner - .get() + minerRef .map(_.askFor[MinerResponse](msg)) .getOrElse(Task.now(MinerNotExist)) } + private[this] val mutex = new Object + private[this] def startMiningProcess(node: Node): Unit = { - atomicMiner.get() match { - case None => - val miner = config.generic.protocol match { - case Ethash | RestrictedEthash => EthashMiner(node) - case MockedPow => MockedMiner(node) + if (minerRef.isEmpty) { + mutex.synchronized { + if (minerRef.isEmpty) { + val miner = config.generic.protocol match { + case Ethash | RestrictedEthash => EthashMiner(node) + case MockedPow => MockedMiner(node) + } + minerRef = Some(miner) + sendMiner(MinerProtocol.StartMining) } - atomicMiner.set(Some(miner)) - sendMiner(MinerProtocol.StartMining) - - case _ => + } } } From a81e65f724cc5e5c95d108ab0df16fa043235395 Mon Sep 17 00:00:00 2001 From: Dominik Zajkowski Date: Thu, 28 Jan 2021 09:58:17 +0100 Subject: [PATCH 2/2] Add a comment explaining the proposed change --- .../io/iohk/ethereum/consensus/ethash/EthashConsensus.scala | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/main/scala/io/iohk/ethereum/consensus/ethash/EthashConsensus.scala b/src/main/scala/io/iohk/ethereum/consensus/ethash/EthashConsensus.scala index fa7bf990a8..1af3019a98 100644 --- a/src/main/scala/io/iohk/ethereum/consensus/ethash/EthashConsensus.scala +++ b/src/main/scala/io/iohk/ethereum/consensus/ethash/EthashConsensus.scala @@ -62,6 +62,12 @@ class EthashConsensus private ( private[this] val mutex = new Object + /* + * guarantees one miner instance + * this should not use a atomic* construct as it has side-effects + * + * TODO further refactors should focus on extracting two types - one with a miner, one without - based on the config + */ private[this] def startMiningProcess(node: Node): Unit = { if (minerRef.isEmpty) { mutex.synchronized {