Merge pull request #655 from earlbread/fix-chain-fork

Fix a bug where the chain is forked when a branch point doesn't exist
planetarium · Nov 8, 2019 · 328102d · 328102d
2 parents ecfba19 + df559d2
commit 328102d
Show file tree

Hide file tree

Showing 5 changed files with 127 additions and 9 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -127,6 +127,8 @@ To be released.
     fails.  [[#644]]
  -  Fixed bug that whole processes could halt when received an invalid
     type of message.  [[#628], [#641]]
+ -  Fixed a bug that received blocks could not be processed if a branch point
+    is a stale block.  [[#655]]
 
 [#209]: https://github.com/planetarium/libplanet/issues/209
 [#405]: https://github.com/planetarium/libplanet/issues/405
@@ -179,6 +181,7 @@ To be released.
 [#645]: https://github.com/planetarium/libplanet/pull/645
 [#647]: https://github.com/planetarium/libplanet/pull/647
 [#654]: https://github.com/planetarium/libplanet/pull/654
+[#655]: https://github.com/planetarium/libplanet/pull/655
 
 
 Version 0.6.0

diff --git a/Libplanet.Tests/Blockchain/BlockChainTest.cs b/Libplanet.Tests/Blockchain/BlockChainTest.cs
@@ -576,6 +576,26 @@ public async void Fork()
             Assert.Equal(2, forked.Count);
         }
 
+        [Fact]
+        public async Task ForkWithBlockNotExistInChain()
+        {
+            var genesis = await _blockChain.MineBlock(_fx.Address1);
+
+            for (var i = 0; i < 2; i++)
+            {
+                await _blockChain.MineBlock(_fx.Address1);
+            }
+
+            var newBlock = TestUtils.MineNext(genesis, difficulty: 1024);
+
+            Assert.Throws<ArgumentException>(() =>
+                _blockChain.Fork(newBlock.Hash));
+
+            _blockChain.Store.PutBlock(newBlock);
+            Assert.Throws<ArgumentException>(() =>
+                _blockChain.Fork(newBlock.Hash));
+        }
+
         [Fact]
         public void ForkChainWithIncompleteBlockStates()
         {

diff --git a/Libplanet.Tests/Net/SwarmTest.cs b/Libplanet.Tests/Net/SwarmTest.cs
@@ -15,7 +15,6 @@
 using Libplanet.Crypto;
 using Libplanet.Net;
 using Libplanet.Net.Messages;
-using Libplanet.Net.Protocols;
 using Libplanet.Tests.Blockchain;
 using Libplanet.Tests.Common.Action;
 using Libplanet.Tests.Store;
@@ -2317,6 +2316,79 @@ public async Task HandleReorgInSynchronizing()
             }
         }
 
+        [Fact(Timeout = Timeout)]
+        public async Task CreateNewChainWhenBranchPointNotExist()
+        {
+            // If the bucket stored peers are the same, the block may not propagate,
+            // so specify private keys to make the buckets different.
+            var keyA = ByteUtil.ParseHex(
+                "8568eb6f287afedece2c7b918471183db0451e1a61535bb0381cfdf95b85df20");
+            var keyB = ByteUtil.ParseHex(
+                "c34f7498befcc39a14f03b37833f6c7bb78310f1243616524eda70e078b8313c");
+            var keyC = ByteUtil.ParseHex(
+                "941bc2edfab840d79914d80fe3b30840628ac37a5d812d7f922b5d2405a223d3");
+
+            var minerSwarmA = new Swarm<DumbAction>(
+                _blockchains[0],
+                new PrivateKey(keyA),
+                1,
+                host: IPAddress.Loopback.ToString());
+            var minerSwarmB = new Swarm<DumbAction>(
+                _blockchains[1],
+                new PrivateKey(keyB),
+                1,
+                host: IPAddress.Loopback.ToString());
+            var receiverSwarm = new Swarm<DumbAction>(
+                _blockchains[2],
+                new PrivateKey(keyC),
+                1,
+                host: IPAddress.Loopback.ToString());
+
+            BlockChain<DumbAction> minerChainA = _blockchains[0];
+            BlockChain<DumbAction> minerChainB = _blockchains[1];
+            BlockChain<DumbAction> receiverChain = _blockchains[2];
+
+            try
+            {
+                await StartAsync(minerSwarmA);
+                await StartAsync(minerSwarmB);
+                await StartAsync(receiverSwarm);
+
+                await BootstrapAsync(minerSwarmA, receiverSwarm.AsPeer);
+                await BootstrapAsync(minerSwarmB, receiverSwarm.AsPeer);
+
+                // Broadcast SwarmA's first block.
+                var b1 = await minerChainA.MineBlock(_fx1.Address1);
+                await minerChainB.MineBlock(_fx1.Address1);
+                minerSwarmA.BroadcastBlocks(new[] { b1 });
+                await receiverSwarm.BlockReceived.WaitAsync();
+                Assert.Equal(receiverChain.Tip, minerChainA.Tip);
+
+                // Broadcast SwarmB's second block.
+                await minerChainA.MineBlock(_fx1.Address1);
+                var b2 = await minerChainB.MineBlock(_fx1.Address1);
+                minerSwarmB.BroadcastBlocks(new[] { b2 });
+                await receiverSwarm.BlockReceived.WaitAsync();
+                Assert.Equal(receiverChain.Tip, minerChainB.Tip);
+
+                // Broadcast SwarmA's third block.
+                var b3 = await minerChainA.MineBlock(_fx1.Address1);
+                await minerChainB.MineBlock(_fx1.Address1);
+                minerSwarmA.BroadcastBlocks(new[] { b3 });
+                await receiverSwarm.BlockReceived.WaitAsync();
+                Assert.Equal(receiverChain.Tip, minerChainA.Tip);
+            }
+            finally
+            {
+                await minerSwarmA.StopAsync();
+                await minerSwarmB.StopAsync();
+                await receiverSwarm.StopAsync();
+                minerSwarmA.Dispose();
+                minerSwarmB.Dispose();
+                receiverSwarm.Dispose();
+            }
+        }
+
         private static async Task<(Address, Block<DumbAction>[])>
             MakeFixtureBlocksForPreloadAsyncCancellationTest()
         {

diff --git a/Libplanet/Blockchain/BlockChain.cs b/Libplanet/Blockchain/BlockChain.cs
@@ -961,6 +961,22 @@ internal IEnumerable<HashDigest<SHA256>> FindNextHashes(
 
         internal BlockChain<T> Fork(HashDigest<SHA256> point)
         {
+            if (!Contains(point))
+            {
+                throw new ArgumentException(
+                    $"The block [{point}] doesn't exist.",
+                    nameof(point));
+            }
+
+            Block<T> pointBlock = this[point];
+
+            if (!point.Equals(this[pointBlock.Index].Hash))
+            {
+                throw new ArgumentException(
+                    $"The block [{point}] doesn't exist in the chain index.",
+                    nameof(point));
+            }
+
             var forked = new BlockChain<T>(Policy, Store, Guid.NewGuid());
             Guid forkedId = forked.Id;
             _logger.Debug(
@@ -974,7 +990,6 @@ internal BlockChain<T> Fork(HashDigest<SHA256> point)
                 _rwlock.EnterReadLock();
 
                 Store.ForkBlockIndexes(Id, forkedId, point);
-                Block<T> pointBlock = _blocks[point];
 
                 var signersToStrip = new Dictionary<Address, int>();
 
@@ -1074,12 +1089,19 @@ internal BlockLocator GetBlockLocator(int threshold = 10)
         // we need to add a synchronization mechanism to handle this correctly.
         internal void Swap(BlockChain<T> other, bool render)
         {
+            if (other?.Tip is null)
+            {
+                throw new ArgumentException(
+                        $"The chain to be swapped is invalid. Id: {other?.Id}, Tip: {other?.Tip}",
+                        nameof(other));
+            }
+
             _logger.Debug(
-                "Swaping block chain. (from: {fromChainId}) (to: {toChainId})", Id, other.Id);
+                "Swapping block chain. (from: {fromChainId}) (to: {toChainId})", Id, other.Id);
 
             // Finds the branch point.
             Block<T> topmostCommon = null;
-            if (render && !(Tip is null || other.Tip is null))
+            if (render && !(Tip is null))
             {
                 long shorterHeight =
                     Math.Min(Count, other.Count) - 1;

diff --git a/Libplanet/Net/Swarm.cs b/Libplanet/Net/Swarm.cs
@@ -1714,14 +1714,15 @@ await GetBlockHashesAsync(peer, locator, stop, cancellationToken)
                         _logger.Debug("It doesn't need to fork.");
                     }
 
-                    // FIXME BlockChain<T>.Contains() can be very
-                    // expensive.
-                    // we can omit this clause if assume every chain shares
+                    // We can omit this clause if assume every chain shares
                     // same genesis block...
-                    else if (!workspace.Contains(branchPoint))
+                    else if (!workspace.Contains(branchPoint)
+                             || (workspace[branchPoint].Index is long branchPointIndex
+                                 && !workspace[branchPointIndex].Hash.Equals(branchPoint)))
                     {
                         // Create a whole new chain because the branch point doesn't exist on
                         // the current chain.
+                        _logger.Debug("Create new chain...");
                         workspace = new BlockChain<T>(
                             workspace.Policy,
                             workspace.Store,
@@ -1803,7 +1804,7 @@ await GetBlocksAsync(peer, hashesAsArray)
             finally
             {
                 IStore store = blockChain.Store;
-                foreach (var id in scope.Where(guid => guid != workspace.Id))
+                foreach (var id in scope.Where(guid => guid != workspace?.Id))
                 {
                     store.DeleteChainId(id);
                 }