Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collect different nodes between two state root hashes #1023

Merged
merged 22 commits into from
Oct 12, 2020

Conversation

moreal
Copy link
Contributor

@moreal moreal commented Sep 29, 2020

In this pull request, it introduced a new planet CLI feature to compare two trees via root hash.

@@ -3,7 +3,7 @@

namespace Libplanet.Store.Trie.Nodes
{
internal interface INode
public interface INode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an XML comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted INode's access modifier into internal again.

@@ -4,7 +4,7 @@

namespace Libplanet.Store.Trie
{
internal interface ITrie
public interface ITrie
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an XML comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it in db620fa.

@@ -12,7 +12,7 @@ namespace Libplanet.Store.Trie
{
// TODO: implement 'logs' for debugging.
[Equals]
internal class MerkleTrie : ITrie
public class MerkleTrie : ITrie
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an XML comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it in db620fa.


namespace Libplanet.Store.Trie
{
public static class MerkleTrieExtensions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an XML comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it in db620fa.

@moreal moreal force-pushed the feature/trie-diff branch 4 times, most recently from ac8fe6b to 3068723 Compare September 29, 2020 10:22
@codecov
Copy link

codecov bot commented Sep 29, 2020

Codecov Report

Merging #1023 into main will increase coverage by 0.18%.
The diff coverage is 94.65%.

@@            Coverage Diff             @@
##             main    #1023      +/-   ##
==========================================
+ Coverage   88.24%   88.43%   +0.18%     
==========================================
  Files         331      334       +3     
  Lines       30245    30341      +96     
==========================================
+ Hits        26691    26831     +140     
+ Misses       1942     1902      -40     
+ Partials     1612     1608       -4     
Impacted Files Coverage Δ
Libplanet/Store/Trie/MerkleTrieExtensions.cs 81.25% <81.25%> (ø)
Libplanet/Store/Trie/MerkleTrie.cs 91.82% <96.87%> (+2.15%) ⬆️
...lanet.Tests/Store/Trie/MerkleTrieExtensionsTest.cs 100.00% <100.00%> (ø)
Libplanet.Tests/Store/Trie/MerkleTrieTest.cs 100.00% <100.00%> (ø)
Libplanet/Net/Protocols/KademliaProtocol.cs 79.36% <100.00%> (-0.47%) ⬇️
Libplanet/Net/Protocols/RoutingTable.cs 92.36% <100.00%> (+0.05%) ⬆️
... and 5 more

@moreal moreal marked this pull request as ready for review October 5, 2020 10:31
@@ -4,7 +4,11 @@

namespace Libplanet.Store.Trie
{
internal interface ITrie
/// <summary>
/// A interface for <see href="https://en.wikipedia.org/wiki/Merkle_tree">Merkle Tree</see>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// A interface for <see href="https://en.wikipedia.org/wiki/Merkle_tree">Merkle Tree</see>.
/// An interface for <see href="https://en.wikipedia.org/wiki/Merkle_tree">Merkle Tree</see>.

Comment on lines 39 to 40
/// <param name="rootHash">The root hash of <see cref="MerkleTrie"/>.
/// <seealso cref="ITrie.Hash"/>.</param>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK <seealso> cannot be appeared inside <param>.

Suggested change
/// <param name="rootHash">The root hash of <see cref="MerkleTrie"/>.
/// <seealso cref="ITrie.Hash"/>.</param>
/// <param name="rootHash">The root <see cref="ITrie.Hash"/> of
/// <see cref="MerkleTrie"/>.</param>

Comment on lines 41 to 44
/// <param name="secure">The flag to determine to use <see cref="MerkleTrie"/> in secure
/// mode. If it is true, <see cref="MerkleTrie"/> will stores the value with the hashed
/// result from the given key as the key. It will hash with
/// <see cref="Hashcash.Hash"/>.</param>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// <param name="secure">The flag to determine to use <see cref="MerkleTrie"/> in secure
/// mode. If it is true, <see cref="MerkleTrie"/> will stores the value with the hashed
/// result from the given key as the key. It will hash with
/// <see cref="Hashcash.Hash"/>.</param>
/// <param name="secure">The flag to determine whether to use <see cref="MerkleTrie"/> in
/// secure mode. If it is turned on, <see cref="MerkleTrie"/> internally stores hashed keys
/// instead of bare keys. <see cref="Hashcash.Hash"/> is used to hash them.</param>

/// </summary>
/// <param name="origin">A trie to compare.</param>
/// <param name="other">An other trie to compare.</param>
/// <returns>A <see cref="IGrouping{TKey,TElement}"/>s,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// <returns>A <see cref="IGrouping{TKey,TElement}"/>s,
/// <returns><see cref="IGrouping{TKey,TElement}"/>s,

{
public class Mpt
{
[Command(Description = "Compare two ")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compare two… what?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to fill it, sorry 😢 Thank you 🙏

Comment on lines 61 to 80
switch (keyValueStoreType)
{
case "rocksdb":
return new RocksDBKeyValueStore(keyValueStorePath);
case "default":
return new RocksDBKeyValueStore(keyValueStorePath);
default:
throw new CommandExitedException(
$"The given {nameof(keyValueStoreType)}," +
$" {keyValueStorePath} is not supported.",
-1);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than using switch statement, how about maintaining a read-only static IImmutableDictionary<string, Func<string, IKeyValueStore>>? It would be useful to show all available options in error/help messages.

Plus, the error message would better if it is like:

There is no such type for -t/--kv-store-type: {kvStoreType}.  Available options are: rocksdb, default.

Comment on lines 23 to 24
[Option]
string keyValueStoreType,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option name is too long. It should have a short name (e.g., -t) as well.

Also how about renaming it to kvStoreType?

Comment on lines 17 to 26
[Argument(Name = "KEY-VALUE-STORE-PATH")]
string keyValueStorePath,
[Argument(Name = "STATE-ROOT-HASH")]
string stateRootHashHex,
[Argument(Name = "OTHER-STATE-ROOT-HASH")]
string otherStateRootHashHex,
[Option]
string keyValueStoreType,
[Option]
bool secure = false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give these Arguments and Options their own Description?

[Command(Description = "Compare two ")]
public void Diff(
[Argument(Name = "KEY-VALUE-STORE-PATH")]
string keyValueStorePath,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about renaming it to kvStorePath?

@@ -5,6 +5,7 @@ namespace Libplanet.Tools
{
[HasSubCommands(typeof(Apv), Description = "App protocol version utilities.")]
[HasSubCommands(typeof(Key), Description = "Manage private keys.")]
[HasSubCommands(typeof(Mpt), Description = "Merkle Trie utilities.")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a trivia but: even though the sub-command mpt has a P in its name, the description doesn't mention Patricia.

@moreal moreal requested a review from dahlia October 6, 2020 02:16
@moreal moreal force-pushed the feature/trie-diff branch 2 times, most recently from ce847a0 to 5b636d3 Compare October 6, 2020 03:23
@moreal moreal added storage Related to storage (Libplanet.Store) tools Related to CLI tools (Libplanet.Tools) labels Oct 6, 2020
@moreal
Copy link
Contributor Author

moreal commented Oct 6, 2020

@dahlia I applied for your reviews. Please review these again. Also, I am not sure the usage of Argument and Option was well. In other words, it is about what parameter should be Argument and what parameter should be Option. Could you leave your opinion about it? 🙏

riemannulus
riemannulus previously approved these changes Oct 6, 2020
longfin
longfin previously approved these changes Oct 6, 2020
Copy link
Contributor

@dahlia dahlia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I am not sure the usage of Argument and Option was well. In other words, it is about what parameter should be Argument and what parameter should be Option. Could you leave your opinion about it? 🙏

A rule of thumb is: required values should be arguments and optional values should be options. However, this rule would not be that helpful for this situation.

The reason why the current design of planet mpt diff command seems complicated is that it takes too many values and they are all required. What I want to suggest are:

  • Merge path and store type into one. URI-like string would work (IMHO), e.g., rocksdb+file:///home/user/somewhere. Falling back scheme-less URI (i.e., local path) to default+file:// might be nice to have.

  • Manage stores in the same manner to how git remote are managed. E.g.:

    $ planet mpt add somewhere rocksdb+file:///home/user/somewhere
    $ planet mpt diff somewhere root-a root-b
  • As like git pull or git push do, planet mpt diff should be able to take a bare URI besides a registered store name, e.g.:

    $ planet mpt diff rocksdb+file:///home/user/somewhere root-a root-b

Comment on lines 61 to 60
<PackageReference
Include="Microsoft.DotNet.Analyzers.Compatibility"
Version="0.2.12-alpha">
<PackageReference Include="Microsoft.DotNet.Analyzers.Compatibility" Version="0.2.12-alpha">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently unnecessary…

<PropertyGroup Condition=" '$(PublishSingleFile)' == 'true' And
'$(RuntimeIdentifier.Substring(0, 6))' == 'linux-' ">
<PropertyGroup Condition=" '$(PublishSingleFile)' == 'true' And&#xA; '$(RuntimeIdentifier.Substring(0, 6))' == 'linux-' ">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently unnecessary…

Comment on lines 18 to 20
ImmutableSortedDictionary<string, Func<string, IKeyValueStore>>.Empty
.Add("default", kvStorePath => new DefaultKeyValueStore(kvStorePath))
.Add("rocksdb", kvStorePath => new RocksDBKeyValueStore(kvStorePath));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using a dictionary literal and immediately calling its .ToImmutableSortedDictionary() method? That would be more readable.

't',
Description = "The type of the key-value store stored" +
" at the given KEY-VALUE-STORE-PATH argument. " +
"It should be among in 'default' or 'rocksdb'.")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it's code duplication (the same keys are appeared twice in this file), it might be better to have a separated command to list all available choices, and mention the command here… but I am not sure.

Anyway, I wrote a feature request which address this kind of problem to Cocona: mayuki/Cocona#27.

/// nodes.</param>
/// <param name="rootHash">The root <see cref="ITrie.Hash"/> of
/// <see cref="MerkleTrie"/>.</param>
/// <param name="secure">The flag to determine to use <see cref="MerkleTrie"/> in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// <param name="secure">The flag to determine to use <see cref="MerkleTrie"/> in
/// <param name="secure">Whether to use <see cref="MerkleTrie"/> in

public static class MerkleTrieExtensions
{
/// <summary>
/// Compare two given tries and get different value nodes (leaf node).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.NET docs apparently prefer third-person singular verbs. Here's an unofficial guide for docs style:

Consider using third person singular verb to start the summary of members.

Suggested change
/// Compare two given tries and get different value nodes (leaf node).
/// Compares two given tries and gets different value nodes (leaf node).

Also, it separates the steps of usage like `git-remote`.

Co-Authored-By: Hong Minhee <hong.minhee@gmail.com>
@moreal
Copy link
Contributor Author

moreal commented Oct 7, 2020

@dahlia I tried to implement your suggestions. After it, it became cool to use. 🙏

> planet mpt diff "9c-beta-7-rc9-2" "e682848e718ffab0135335d69c56b26e61d6ac6aacca61d13f87b826a211ef28" "b28b1971110e12
4667f3682233d4901766fdb48dc7c9e05da184cc7713552f8c"

There are some points to fix in CLI (e.g, Checks there is already the alias name, Refactoring, etc...) yet. But you can review other points about code quality and pattern like 'Dependency Injection Pattern was organized well' if you have free of your time.

I would like to thank you again for your good suggestions.

Comment on lines 142 to 144
var exceptionMessage = "The scheme must be explicit and must be formatted with " +
"two methods, '<kv-store-type>' or " +
"'<kv-store-type>+<uri-scheme>'.";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this message would be enough:

A key-value store URI must have a scheme, e.g., default://, rocksdb+file://.

splitScheme[0], ignoreCase: true, out KVStoreType kvStoreType))
{
throw new NotSupportedException(
$"There is no supported kv-store-type for '{splitScheme[0]}'");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this:

No key-value store backend supports the such scheme: {splitScheme[0]}.

&& !SchemeType.TryParse(splitScheme[1], ignoreCase: true, out schemeType))
{
throw new NotSupportedException(
$"There is no supported scheme type for '{splitScheme[1]}'");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About a term, one thing I want to suggest is: how about using the term backend instead of type?

How about this:

No key-value store backend supports the such scheme: {splitScheme[1]}.

throw new KeyNotFoundException($"The given key, '{key}' does not exist.");
}

return _fileSystem.ReadAllText(KeyPath(key));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than implementing key-value configuration using multiple files (where filenames are keys), IMHO it's more handy to maintain a single JSON/YAML/TOML file.

It would be okay with any format at this moment, because we can change the format before releasing v0.10.0. So the easiest one to implement it. Or leave a FIXME comment and file an issue addressing this and let's move fast.

Copy link
Contributor Author

@moreal moreal Oct 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to find ways to stay it now interface and to maintain with JSON. But it was hard to use strict type because they must be Dictionary<string, object> type. So I introduced a new ToolConfiguration struct class and changed the IConfigurationService interface. Now the interface provides methods only Load and Store. Another instruction can proceed like handling a struct variable, with strict type 😀

Now, it is maintained on JSON.

$"There is no supported scheme type for '{splitScheme[1]}'");
}

return (kvStoreType, schemeType, uri.AbsolutePath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need KVStoreType thing and LoadKeyValueStore() method? Can't we make this ParseKVStoreURI() method to directory return IKeyValueStore instead of KVStoreType?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood your comment as that let us merge a function to parse and another function to load kv-store into one. Then the method ParseKVStoreURI() will be return a single value typed IKeyValueStore not tuple, right?

Copy link
Contributor

@dahlia dahlia Oct 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I mean.

@moreal
Copy link
Contributor Author

moreal commented Oct 8, 2020

@dahlia I applied your suggestions. You can review these again! 🙏

Copy link
Contributor

@dahlia dahlia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been long… and I believe it's my last review. Also you should add changelogs about the CLI tools (there's the section in the CHANGES.md file), e.g.:

 -   Added a new sub-command `planet mpt`.
 -   Introduced a configuration file.  It's placed in:
      -  Linux/macOS: *<var>$XDG_CONFIG_HOME</var>/planetarium/cli.json*
      -  Windows: *<var>%AppData%</var>\planetarium\cli.json*

private static readonly string FileConfigurationServiceRoot = Path.Combine(
Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData),
"planetarium",
"planet");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this (which is parallel with @planetarium/cli on npm)?

Suggested change
"planet");
"cli.json");

moreal and others added 3 commits October 8, 2020 22:13
Co-Authored-By: Hong Minhee <hong.minhee@gmail.com>
Co-Authored-By: Hong Minhee <hong.minhee@gmail.com>
dahlia
dahlia previously approved these changes Oct 8, 2020
Copy link
Contributor

@dahlia dahlia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

@moreal
Copy link
Contributor Author

moreal commented Oct 12, 2020

Could you review this pull request? @planetarium/libplanet

earlbread
earlbread previously approved these changes Oct 12, 2020
for (int i = 0; i < bytes.Length / 2; ++i)
{
byte high = bytes[i * 2], low = bytes[i * 2 + 1];
if (high < 0 || high >= 16)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't high >= 16 enough because high and low couldn't be lower than 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I realized byte of C# is unsigned but I forgot. I will amend 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I amended it in 126f439. 🙏

yield return hashNode.HashDigest;
INode? node = GetNode(hashNode.HashDigest);
(INode node, ImmutableArray<byte> path) = queue.Dequeue();
yield return (node, path);
switch (node)
{
case FullNode fullNode:
IEnumerable<HashNode> childHashNodes = fullNode.Children.OfType<HashNode>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be used.

Copy link
Contributor Author

@moreal moreal Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I amended it in 5fdd86b. 🙏

moreal and others added 2 commits October 12, 2020 11:03
Co-Authored-By: Seunghun Lee <waydi1@gmail.com>
The `byte` type of C#, is same with `unsigned byte` in other languages.
So it is useless comparison to check it is less than 0.

Co-Authored-By: Seunghun Lee <waydi1@gmail.com>
@moreal moreal dismissed stale reviews from earlbread and dahlia via 126f439 October 12, 2020 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
storage Related to storage (Libplanet.Store) tools Related to CLI tools (Libplanet.Tools)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants