Skip to content

Create new struct hash package#730

Closed
krhubert wants to merge 8 commits intodevfrom
feature/structhash
Closed

Create new struct hash package#730
krhubert wants to merge 8 commits intodevfrom
feature/structhash

Conversation

@krhubert
Copy link
Copy Markdown
Contributor

features:

  • panic on an invalid tag (thanks to it one bug was found in Dependency)
  • can use omitempty filed and extend structs
  • more test coverage
  • support complex type 🏆
  • tags version method and lastversion are not included because core doesn't' use it, but it could be easily extended

@krhubert krhubert force-pushed the feature/structhash branch 3 times, most recently from df09b75 to f4ed582 Compare January 25, 2019 14:19
@krhubert krhubert removed the on hold label Feb 20, 2019
@krhubert krhubert requested review from NicolasMahe and ilgooz and removed request for ilgooz February 20, 2019 05:54
@NicolasMahe
Copy link
Copy Markdown
Member

@mesg-foundation/core please review

Copy link
Copy Markdown
Member

@antho1404 antho1404 left a comment

Choose a reason for hiding this comment

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

I don't think this PR is needed. Few reasons for that:

  • It adds a lot of complexity for the hashing method, we duplicate some logic from another library without having all the history from that (list of pending issues for example). If we really want to have this change I would recommend to fork and fix the library and even better to do a PR on that project.
  • This hash is not critical anymore. We needed a deterministic hash even with different modifications in the structure itself for the services but we moved to the tar sum to generate the hash so this is not critical anymore.
  • There is still the problem with the executions that if we have a hash that can change from one version to another it's a problem but for now executions are only local and temporary so it's not really a big problem and also we can always have our own specific hash method where we hash an array that we create for example.

I strongly feel that this development is too complex to solve an issue that will be solved by #731.

@NicolasMahe NicolasMahe mentioned this pull request Mar 14, 2019

// Sha1 takes a data structure and returns its md5 hash as string.
func Sha3(v interface{}) [64]byte {
return sha3.Sum512(serialize(v))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wrong command and why sha3 is 512 bits?

@NicolasMahe
Copy link
Copy Markdown
Member

This PR is way less important since PR #731 has been merged.
The core still need to hash struct for event and execution but their lifetime is much smaller and thus having a different hash after an upgrade of the core doesn't impact.
It could only have impact with the network with nodes running different version of core. But anyway, the "omitempty" will not resolve the problem.

I suggest to keep the current hash package (https://github.com/cnf/structhash)

@krhubert any suggestion why we should merge this PR?

@NicolasMahe NicolasMahe deleted the feature/structhash branch March 14, 2019 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants