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

Have a deterministic Map struct that can be go-wired #553

Closed
sunnya97 opened this issue Mar 3, 2018 · 15 comments
Closed

Have a deterministic Map struct that can be go-wired #553

sunnya97 opened this issue Mar 3, 2018 · 15 comments

Comments

@sunnya97
Copy link
Member

sunnya97 commented Mar 3, 2018

No description provided.

@zmanian
Copy link
Member

zmanian commented Mar 5, 2018

Here are the options based on previous research.

  1. Fork this code
    https://golang.org/src/runtime/hashmap.go and just create a constructor that sets hash0 via an initialization argument.
    Downsides: breaks interoperability with Golang builtins but it otherwise the same api as normal golang maps and uses the audited code that everyone uses.

  2. This is a pretty sane looking deterministic hashmap but no one really uses it . https://github.com/cornelk/hashmap

  3. As far as I know golang maps are only unsafe in consensus if control flow depends on iteration order. These things are pretty easy to detect. We could probably write a lint against this pattern more easily than 1 or 2

@cwgoes
Copy link
Contributor

cwgoes commented Apr 12, 2018

I think this is worthwhile - even if we could write Gaia without it, I can imagine lots of SDK applications that would want to use a map - better that we provide a common option, test it ourselves, and provide clear documentation on how to use it safely (deterministically).

@ebuchman
Copy link
Member

As far as I know golang maps are only unsafe in consensus if control flow depends on iteration order. These things are pretty easy to detect. We could probably write a lint against this pattern more easily than 1 or 2

I like the idea of getting a linter into SDK app build workflow - we can flag other sources of non-determinism too, like time, rand, float

@mappum
Copy link
Contributor

mappum commented Apr 21, 2018

One thing to watch out for with deterministic maps is that attackers may be able to make operations extra expensive by choosing colliding hashmap hashes for many values, so that the bucket gets really long and you get worst-case performance. (This was a problem in Node.js for a while, they fixed it by making the hash function different across processes).

I think we'd have to fix this by doing something like consuming gas based on the number of values in the bucket.

@ebuchman
Copy link
Member

Yeh good point. Linting obviously isn't enough.

What about just using the KVStore interface for this ? We would just need a way to marshal/unmarshal KVStore ...

@sunnya97
Copy link
Member Author

sunnya97 commented Apr 21, 2018

I think maybe having a Map interface would be good, and then we can have like different implementations. TreeMap, HashMap, ListMap, etc?

Also regarding a TreeMap, if amino gives us length of the bytes, could we serialize the TreeMap efficiently, such that we can read in log(n) without deserializing the entire tree?

@zmanian
Copy link
Member

zmanian commented Apr 21, 2018

@mappum This is super insightful.

@ebuchman ebuchman added the API label Apr 23, 2018
@ebuchman
Copy link
Member

I think maybe having a Map interface would be good, and then we can have like different implementations. TreeMap, HashMap, ListMap, etc?

Let's do this.

Also regarding a TreeMap, if amino gives us length of the bytes, could we serialize the TreeMap efficiently, such that we can read in log(n) without deserializing the entire tree?

That would be great - will have to investigate

@jackzampolin
Copy link
Member

I think we changed encoding formats since go-wire so I'm going to assume this issue has been addressed. Closing.

@cwgoes
Copy link
Contributor

cwgoes commented Oct 12, 2018

I think we changed encoding formats since go-wire so I'm going to assume this issue has been addressed. Closing.

Alas, that was merely a name change - this is still a problem. Definitely post-launch though.

@cwgoes cwgoes reopened this Oct 12, 2018
@jackzampolin jackzampolin removed this from the 1.0 Code Freeze milestone Jan 28, 2019
@jackzampolin
Copy link
Member

Is this still something we should do?

@alexanderbez
Copy link
Contributor

With the move to directly using proto (or a simple wrapper of one), I don't see the need for this.

@zmanian
Copy link
Member

zmanian commented May 28, 2019

The way protobuf works is that the spec doesn't specific deterministic maps but most implementation sort keys before serialization.

@alexanderbez
Copy link
Contributor

Seems reasonable to me. This is generally something we hold as an invariant in the SDK anyway.

@jackzampolin
Copy link
Member

So going to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants