From 237f4059f102cb718d9677ee7426e5a51365c114 Mon Sep 17 00:00:00 2001 From: ajlopez Date: Sun, 2 Jul 2017 12:03:28 -0300 Subject: [PATCH 1/2] Cloning working block in submitBitcoinBlock to avoid mutation over the same object --- .../java/co/rsk/mine/MinerClientImpl.java | 4 ++ .../java/co/rsk/mine/MinerServerImpl.java | 19 +++++++-- .../java/co/rsk/mine/MinerManagerTest.java | 39 +++++++++++++++++++ 3 files changed, 58 insertions(+), 4 deletions(-) diff --git a/rskj-core/src/main/java/co/rsk/mine/MinerClientImpl.java b/rskj-core/src/main/java/co/rsk/mine/MinerClientImpl.java index d4f0655ce0c..cb4efeb3a9b 100644 --- a/rskj-core/src/main/java/co/rsk/mine/MinerClientImpl.java +++ b/rskj-core/src/main/java/co/rsk/mine/MinerClientImpl.java @@ -31,6 +31,7 @@ import javax.annotation.Nonnull; import java.math.BigInteger; +import java.util.Random; import java.util.Timer; import java.util.TimerTask; @@ -42,6 +43,7 @@ @Component("MinerClient") public class MinerClientImpl implements MinerClient { + private static Random random = new Random(); @Autowired private Rsk rsk; @@ -171,6 +173,8 @@ public boolean mineBlock() { */ private boolean findNonce(@Nonnull final co.rsk.bitcoinj.core.BtcBlock bitcoinMergedMiningBlock, @Nonnull final BigInteger target) { + bitcoinMergedMiningBlock.setNonce(random.nextLong()); + while (!stop && !newBestBlockArrivedFromAnotherNode) { // Is our proof of work valid yet? BigInteger blockHashBI = bitcoinMergedMiningBlock.getHash().toBigInteger(); diff --git a/rskj-core/src/main/java/co/rsk/mine/MinerServerImpl.java b/rskj-core/src/main/java/co/rsk/mine/MinerServerImpl.java index d07ad085fee..ee4b8f7e0e3 100644 --- a/rskj-core/src/main/java/co/rsk/mine/MinerServerImpl.java +++ b/rskj-core/src/main/java/co/rsk/mine/MinerServerImpl.java @@ -151,20 +151,26 @@ public void submitBitcoinBlock(String blockHashForMergedMining, co.rsk.bitcoinj. Block newBlock; Sha3Hash key = new Sha3Hash(TypeConverter.removeZeroX(blockHashForMergedMining)); synchronized (LOCK) { - newBlock = blocksWaitingforPoW.get(key); - if (newBlock == null) { + Block workingBlock = blocksWaitingforPoW.get(key); + + if (workingBlock == null) { logger.warn("Cannot publish block, could not find hash " + blockHashForMergedMining + " in the cache"); return; } // just in case, remove all references to this block. - if (latestBlock == newBlock) { + if (latestBlock == workingBlock) { latestBlock = null; latestblockHashWaitingforPoW = null; currentWork = null; } + + // clone the block + newBlock = new Block(workingBlock.getEncoded()); + logger.debug("blocksWaitingforPoW size " + blocksWaitingforPoW.size()); } + logger.info("Received block {} {}", newBlock.getNumber(), Hex.toHexString(newBlock.getHash())); newBlock.setBitcoinMergedMiningHeader(bitcoinMergedMiningBlock.cloneAsHeader().bitcoinSerialize()); @@ -259,7 +265,7 @@ public MinerWork getWork() { * currentWork, regardless of what it is. */ synchronized (LOCK) { - if (currentWork != work || currentWork == null) { + if (currentWork != work || currentWork == null) { return currentWork; } currentWork = new MinerWork(currentWork.getBlockHashForMergedMining(), currentWork.getTarget(), @@ -270,6 +276,11 @@ public MinerWork getWork() { return work; } + @VisibleForTesting + public void setWork(MinerWork work) { + this.currentWork = work; + } + public MinerWork updateGetWork(@Nonnull final Block block, @Nonnull final boolean notify) { Sha3Hash blockMergedMiningHash = new Sha3Hash(block.getHashForMergedMining()); diff --git a/rskj-core/src/test/java/co/rsk/mine/MinerManagerTest.java b/rskj-core/src/test/java/co/rsk/mine/MinerManagerTest.java index cb98aefd9d4..1d6417306a7 100644 --- a/rskj-core/src/test/java/co/rsk/mine/MinerManagerTest.java +++ b/rskj-core/src/test/java/co/rsk/mine/MinerManagerTest.java @@ -32,6 +32,8 @@ import org.junit.Assert; import org.junit.Test; +import java.util.Arrays; +import java.util.List; import java.util.concurrent.Callable; /** @@ -98,6 +100,43 @@ public void refreshWorkRunTwice() { Assert.assertEquals(2, blockchain.getBestBlock().getNumber()); } + @Test + public void mineBlockTwiceReusingTheSameWork() { + World world = new World(); + Blockchain blockchain = world.getBlockChain(); + + Assert.assertEquals(0, blockchain.getBestBlock().getNumber()); + + MinerServerImpl minerServer = getMinerServer(blockchain); + MinerClientImpl minerClient = getMinerClient(minerServer); + + minerServer.buildBlockToMine(blockchain.getBestBlock(), false); + + MinerWork minerWork = minerServer.getWork(); + + Assert.assertNotNull(minerWork); + + Assert.assertTrue(minerClient.mineBlock()); + + Block bestBlock = blockchain.getBestBlock(); + + Assert.assertNotNull(bestBlock); + Assert.assertEquals(1, bestBlock.getNumber()); + + // reuse the same work + Assert.assertNull(minerServer.getWork()); + minerServer.setWork(minerWork); + Assert.assertNotNull(minerServer.getWork()); + + Assert.assertTrue(minerClient.mineBlock()); + + List blocks = blockchain.getBlocksByNumber(1); + + Assert.assertNotNull(blocks); + Assert.assertEquals(2, blocks.size()); + Assert.assertFalse(Arrays.equals(blocks.get(0).getHash(), blocks.get(1).getHash())); + } + @Test public void mineBlockWhileSyncingBlocks() { World world = new World(); From 7ef1e220a61e9169a001397e71c3a16737164ade Mon Sep 17 00:00:00 2001 From: ajlopez Date: Sun, 2 Jul 2017 12:11:01 -0300 Subject: [PATCH 2/2] Avoiding random nonce in MinerClientImpl --- rskj-core/src/main/java/co/rsk/mine/MinerClientImpl.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/rskj-core/src/main/java/co/rsk/mine/MinerClientImpl.java b/rskj-core/src/main/java/co/rsk/mine/MinerClientImpl.java index cb4efeb3a9b..9364257a793 100644 --- a/rskj-core/src/main/java/co/rsk/mine/MinerClientImpl.java +++ b/rskj-core/src/main/java/co/rsk/mine/MinerClientImpl.java @@ -31,7 +31,6 @@ import javax.annotation.Nonnull; import java.math.BigInteger; -import java.util.Random; import java.util.Timer; import java.util.TimerTask; @@ -43,7 +42,7 @@ @Component("MinerClient") public class MinerClientImpl implements MinerClient { - private static Random random = new Random(); + private long nextNonceToUse = 0; @Autowired private Rsk rsk; @@ -173,7 +172,7 @@ public boolean mineBlock() { */ private boolean findNonce(@Nonnull final co.rsk.bitcoinj.core.BtcBlock bitcoinMergedMiningBlock, @Nonnull final BigInteger target) { - bitcoinMergedMiningBlock.setNonce(random.nextLong()); + bitcoinMergedMiningBlock.setNonce(nextNonceToUse++); while (!stop && !newBestBlockArrivedFromAnotherNode) { // Is our proof of work valid yet? @@ -182,7 +181,7 @@ private boolean findNonce(@Nonnull final co.rsk.bitcoinj.core.BtcBlock bitcoinMe return true; } // No, so increment the nonce and try again. - bitcoinMergedMiningBlock.setNonce(bitcoinMergedMiningBlock.getNonce() + 1); + bitcoinMergedMiningBlock.setNonce(nextNonceToUse++); if (bitcoinMergedMiningBlock.getNonce() % 100000 == 0) { logger.debug("Solving block. Nonce: " + bitcoinMergedMiningBlock.getNonce()); }