Merge pull request #3949 from greymistcube/refactor/message-encoding

♻️ Changed encoding of `GetBlockHashesMsg` and `BlockHashesMsg`

CHANGES.md

@@ -25,9 +25,14 @@ To be released.
  -  (Libplanet.Net) Changed `BlockHashDownloadState` and `BlockDownloadState`
     to be `Obsolete`.  [[#3943]]
  -  Removed `Fork()` method from `BlockChain`.  [[#3948]]
+ -  Changed the return type for `BlockChain.FindNextHashes()`
+    to `IReadOnlyList<BlockHash>`.  [[#3949]]
 
 ### Backward-incompatible network protocol changes
 
+ -  (Libplanet.Net) Changed the encoding for `GetBlockHashesMsg` and
+    `BlockHashesMsg`.  [[#3949]]
+
 ### Backward-incompatible storage format changes
 
 ### Added APIs
@@ -57,6 +62,7 @@ To be released.
 [#3942]: https://github.com/planetarium/libplanet/pull/3942
 [#3943]: https://github.com/planetarium/libplanet/pull/3943
 [#3948]: https://github.com/planetarium/libplanet/pull/3948
+[#3949]: https://github.com/planetarium/libplanet/pull/3949
 
 
 Version 5.2.2

src/Libplanet.Net/Messages/BlockHashesMsg.cs

@@ -8,18 +8,9 @@ namespace Libplanet.Net.Messages
 {
     internal class BlockHashesMsg : MessageContent
     {
-        public BlockHashesMsg(long? startIndex, IEnumerable<BlockHash> hashes)
+        public BlockHashesMsg(IEnumerable<BlockHash> hashes)
         {
-            StartIndex = startIndex;
             Hashes = hashes.ToList();
-
-            if (StartIndex is null == Hashes.Any())
-            {
-                throw new ArgumentException(
-                    "The startIndex can be null iff hashes are empty.",
-                    nameof(startIndex)
-                );
-            }
         }
 
         public BlockHashesMsg(byte[][] dataFrames)
@@ -28,8 +19,7 @@ public BlockHashesMsg(byte[][] dataFrames)
             var hashes = new List<BlockHash>(hashCount);
             if (hashCount > 0)
             {
-                StartIndex = BitConverter.ToInt64(dataFrames[1], 0);
-                for (int i = 2, end = hashCount + 2; i < end; i++)
+                for (int i = 1, end = hashCount + 1; i < end; i++)
                 {
                     hashes.Add(new BlockHash(dataFrames[i]));
                 }
@@ -38,12 +28,6 @@ public BlockHashesMsg(byte[][] dataFrames)
             Hashes = hashes;
         }
 
-        /// <summary>
-        /// The block index of the first hash in <see cref="Hashes"/>.
-        /// It is <see langword="null"/> iff <see cref="Hashes"/> are empty.
-        /// </summary>
-        public long? StartIndex { get; }
-
         /// <summary>
         /// The continuous block hashes, from the lowest index to the highest index.
         /// </summary>
@@ -58,12 +42,7 @@ public override IEnumerable<byte[]> DataFrames
             {
                 var frames = new List<byte[]>();
                 frames.Add(BitConverter.GetBytes(Hashes.Count()));
-                if (StartIndex is { } offset)
-                {
-                    frames.Add(BitConverter.GetBytes(offset));
-                    frames.AddRange(Hashes.Select(hash => hash.ToByteArray()));
-                }
-
+                frames.AddRange(Hashes.Select(hash => hash.ToByteArray()));
                 return frames;
             }
         }

src/Libplanet.Net/Messages/GetBlockHashesMsg.cs

@@ -1,4 +1,3 @@
-using System;
 using System.Collections.Generic;
 using Libplanet.Blockchain;
 using Libplanet.Types.Blocks;
@@ -14,7 +13,7 @@ public GetBlockHashesMsg(BlockLocator locator)
 
         public GetBlockHashesMsg(byte[][] dataFrames)
         {
-            Locator = new BlockLocator(new BlockHash(dataFrames[1]));
+            Locator = new BlockLocator(new BlockHash(dataFrames[0]));
         }
 
         public BlockLocator Locator { get; }
@@ -26,9 +25,7 @@ public override IEnumerable<byte[]> DataFrames
             get
             {
                 var frames = new List<byte[]>();
-                frames.Add(BitConverter.GetBytes(1));
                 frames.Add(Locator.Hash.ToByteArray());
-                frames.Add(Array.Empty<byte>());
                 return frames;
             }
         }

src/Libplanet.Net/Messages/MessageContent.cs

@@ -31,7 +31,7 @@ public enum MessageType : byte
             /// <summary>
             /// Request to query block hashes.
             /// </summary>
-            GetBlockHashes = 0x04,
+            GetBlockHashes = 0x032,
 
             /// <summary>
             /// Inventory to transfer transactions.
@@ -76,7 +76,7 @@ public enum MessageType : byte
             /// <summary>
             /// Message containing demand block hashes with their index numbers.
             /// </summary>
-            BlockHashes = 0x0e,
+            BlockHashes = 0x33,
 
             /// <summary>
             /// Request current chain status of the peer.

src/Libplanet.Net/Swarm.MessageHandlers.cs

@@ -50,20 +50,15 @@ private Task ProcessMessageHandlerAsync(Message message)
                         "Received a {MessageType} message locator [{LocatorHead}]",
                         nameof(GetBlockHashesMsg),
                         getBlockHashes.Locator.Hash);
-                    BlockChain.FindNextHashes(
+                    IReadOnlyList<BlockHash> hashes = BlockChain.FindNextHashes(
                         getBlockHashes.Locator,
-                        FindNextHashesChunkSize
-                    ).Deconstruct(
-                        out long? offset,
-                        out IReadOnlyList<BlockHash> hashes
-                    );
+                        FindNextHashesChunkSize);
                     _logger.Debug(
-                        "Found {HashCount} hashes after the branchpoint (offset: {Offset}) " +
+                        "Found {HashCount} hashes after the branchpoint " +
                         "with locator [{LocatorHead}]",
                         hashes.Count,
-                        offset,
                         getBlockHashes.Locator.Hash);
-                    var reply = new BlockHashesMsg(offset, hashes);
+                    var reply = new BlockHashesMsg(hashes);
 
                     return Transport.ReplyMessageAsync(reply, message.Identity, default);
                 }

src/Libplanet/Blockchain/BlockChain.cs

@@ -662,11 +662,10 @@ public IImmutableSet<TxId> GetStagedTransactionIds()
         /// <param name="locator">The <see cref="BlockLocator"/> to find the branching point
         /// from.</param>
         /// <param name="count">The Maximum number of <see cref="BlockHash"/>es to return.</param>
-        /// <returns>A tuple of the index of the branch point and <see cref="BlockHash"/>es
-        /// including the branch point <see cref="BlockHash"/>.  If no branch point is found,
-        /// returns a tuple of <see langword="null"/> and an empty array of
-        /// <see cref="BlockHash"/>es.</returns>
-        public Tuple<long?, IReadOnlyList<BlockHash>> FindNextHashes(
+        /// <returns>A list of <see cref="BlockHash"/>es including
+        /// the branch point <see cref="BlockHash"/>.  If no branch point is found,
+        /// returns an empty list of <see cref="BlockHash"/>es.</returns>
+        public IReadOnlyList<BlockHash> FindNextHashes(
             BlockLocator locator,
             int count = 500)
         {
@@ -675,12 +674,12 @@ public IImmutableSet<TxId> GetStagedTransactionIds()
 
             if (!(FindBranchpoint(locator) is { } branchpoint))
             {
-                return new Tuple<long?, IReadOnlyList<BlockHash>>(null, Array.Empty<BlockHash>());
+                return Array.Empty<BlockHash>();
             }
 
             if (!(Store.GetBlockIndex(branchpoint) is { } branchpointIndex))
             {
-                return new Tuple<long?, IReadOnlyList<BlockHash>>(null, Array.Empty<BlockHash>());
+                return Array.Empty<BlockHash>();
             }
 
             var result = new List<BlockHash>();
@@ -705,7 +704,7 @@ public IImmutableSet<TxId> GetStagedTransactionIds()
                     Store.ListChainIds().Count(),
                     stopwatch.ElapsedMilliseconds);
 
-            return new Tuple<long?, IReadOnlyList<BlockHash>>(branchpointIndex, result);
+            return result;
         }
 
         /// <summary>
@@ -1190,34 +1189,19 @@ internal void AppendStateRootHashPreceded(
         /// <see langword="null"/>.</returns>
         internal BlockHash? FindBranchpoint(BlockLocator locator)
         {
-            try
+            if (ContainsBlock(locator.Hash))
             {
-                _rwlock.EnterReadLock();
-
-                _logger.Debug(
-                    "Finding a branchpoint with locator [{LocatorHead}]",
-                    locator.Hash);
-                BlockHash hash = locator.Hash;
-                if (_blocks.ContainsKey(hash)
-                    && _blocks[hash] is Block block
-                    && hash.Equals(Store.IndexBlockHash(Id, block.Index)))
-                {
-                    _logger.Debug(
-                        "Found a branchpoint with locator [{LocatorHead}]: {Hash}",
-                        locator.Hash,
-                        hash);
-                    return hash;
-                }
-
                 _logger.Debug(
-                    "Failed to find a branchpoint locator [{LocatorHead}]",
+                    "Found a branchpoint with locator [{LocatorHead}]: {Hash}",
+                    locator.Hash,
                     locator.Hash);
-                return null;
-            }
-            finally
-            {
-                _rwlock.ExitReadLock();
+                return locator.Hash;
             }
+
+            _logger.Debug(
+                "Failed to find a branchpoint locator [{LocatorHead}]",
+                locator.Hash);
+            return null;
         }
 
         /// <summary>

test/Libplanet.Net.Tests/Messages/BlockHashesTest.cs

@@ -19,23 +19,11 @@ public void Dispose()
             NetMQConfig.Cleanup(false);
         }
 
-        [Fact]
-        public void Constructor()
-        {
-            Assert.Throws<ArgumentException>(() =>
-                new BlockHashesMsg(null, new[] { default(BlockHash) })
-            );
-            Assert.Throws<ArgumentException>(() =>
-                new BlockHashesMsg(123, new BlockHash[0])
-            );
-        }
-
         [Fact]
         public void Decode()
         {
             BlockHash[] blockHashes = GenerateRandomBlockHashes(100L).ToArray();
-            var msg = new BlockHashesMsg(123, blockHashes);
-            Assert.Equal(123, msg.StartIndex);
+            var msg = new BlockHashesMsg(blockHashes);
             Assert.Equal(blockHashes, msg.Hashes);
             var privateKey = new PrivateKey();
             AppProtocolVersion apv = AppProtocolVersion.Sign(privateKey, 3);
@@ -48,7 +36,6 @@ public void Decode()
                 peer,
                 DateTimeOffset.UtcNow);
             BlockHashesMsg restored = (BlockHashesMsg)messageCodec.Decode(encoded, true).Content;
-            Assert.Equal(msg.StartIndex, restored.StartIndex);
             Assert.Equal(msg.Hashes, restored.Hashes);
         }
 

test/Libplanet.Net.Tests/Messages/NetMQMessageCodecTest.cs

@@ -119,7 +119,7 @@ private MessageContent CreateMessage(MessageContent.MessageType type)
                 case MessageContent.MessageType.BlockHeaderMessage:
                     return new BlockHeaderMsg(genesis.Hash, genesis.Header);
                 case MessageContent.MessageType.BlockHashes:
-                    return new BlockHashesMsg(0, new[] { genesis.Hash });
+                    return new BlockHashesMsg(new[] { genesis.Hash });
                 case MessageContent.MessageType.GetChainStatus:
                     return new GetChainStatusMsg();
                 case MessageContent.MessageType.ChainStatus:

test/Libplanet.Tests/Blockchain/BlockChainTest.cs

@@ -433,12 +433,9 @@ public void RenderActionsAfterAppendComplete()
         public void FindNextHashes()
         {
             var key = new PrivateKey();
-            long? offsetIndex;
             IReadOnlyList<BlockHash> hashes;
 
-            _blockChain.FindNextHashes(
-                new BlockLocator(_blockChain.Genesis.Hash))
-                .Deconstruct(out offsetIndex, out hashes);
+            hashes = _blockChain.FindNextHashes(new BlockLocator(_blockChain.Genesis.Hash));
             Assert.Single(hashes);
             Assert.Equal(_blockChain.Genesis.Hash, hashes.First());
             var block0 = _blockChain.Genesis;
@@ -451,19 +448,13 @@ public void FindNextHashes()
                 key, lastCommit: CreateBlockCommit(_blockChain.Tip));
             _blockChain.Append(block3, CreateBlockCommit(block3));
 
-            _blockChain.FindNextHashes(new BlockLocator(block0.Hash))
-                .Deconstruct(out offsetIndex, out hashes);
-            Assert.Equal(0, offsetIndex);
+            hashes = _blockChain.FindNextHashes(new BlockLocator(block0.Hash));
             Assert.Equal(new[] { block0.Hash, block1.Hash, block2.Hash, block3.Hash }, hashes);
 
-            _blockChain.FindNextHashes(new BlockLocator(block1.Hash))
-                .Deconstruct(out offsetIndex, out hashes);
-            Assert.Equal(1, offsetIndex);
+            hashes = _blockChain.FindNextHashes(new BlockLocator(block1.Hash));
             Assert.Equal(new[] { block1.Hash, block2.Hash, block3.Hash }, hashes);
 
-            _blockChain.FindNextHashes(new BlockLocator(block0.Hash), count: 2)
-                .Deconstruct(out offsetIndex, out hashes);
-            Assert.Equal(0, offsetIndex);
+            hashes = _blockChain.FindNextHashes(new BlockLocator(block0.Hash), count: 2);
             Assert.Equal(new[] { block0.Hash, block1.Hash }, hashes);
         }