-
Notifications
You must be signed in to change notification settings - Fork 366
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
Gc 304 companion #454
Gc 304 companion #454
Conversation
Introducing GKR API and making tests pass in light of recent MiMC changes |
staticcheck --> seems this branch is not merged with Just to clarify, what is the description of the PR? A mix of GKR api (#425) + fix MiMC interface ? |
Yes exactly. It was originally the mimc fixes but since that broke GKR as well I had to merge in the GKR API stuff as well. |
It's currently up to date with develop. |
|
r := byte(rand.Int()) //#nosec G404 -- This is a false positive | ||
buf.Write([]byte{r}) | ||
} | ||
var leaf fr.Element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test shouldn't depend on bn254.fr.Element
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
// Code generated by gnark DO NOT EDIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it generated ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes because of the big switch case
"math/big" | ||
) | ||
|
||
func String(api frontend.API, str []byte) (frontend.Variable, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't understand the package name and the function name -- nor while it is a fullblown package in the std
? a package in the std
should read as "here is a circuit component that anyone can use"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of this is to cleanly and easily (from the user's POV at least) convert strings to frontend.Variables in a way compatible with what gnark-crypto does. I get that the large switch-case doesn't feel very std-like and that it's not really a "gadget" though.
Does the idea of this function make sense and just not where it's placed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmhh if it is something we want to do often, then it makes sense to expose it in its own package, but to me "hardcoded_strings.String" is not the right semantic. Maybe in a bytes
package, so that it reads bytes.ToVariable(api, b []byte)
? Or, since it's applicable to constants only, a constant
pacakge with constant.FromString()
or constant.FromByte()
.
If we only use it at one place for now, I would make it package private there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think constant.FromString()
is the best option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason I avoided more straightforward names was that we already have standard ways of turning a string
or []byte
into a variable so I wanted to avoid creating confusion between this way of converting and the existing ones. How's constant.HashToVariable
?
std/hash/hash.go
Outdated
@@ -30,3 +30,5 @@ type Hash interface { | |||
// Reset empty the internal state and put the intermediate state to zero. | |||
Reset() | |||
} | |||
|
|||
var BuilderRegistry = make(map[string]func(api frontend.API) (Hash, error)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is that? comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's for the GKR API. In the specification of a circuit, we don't pass an actual hash object, but a string identifier which can later be used to construct a hash object both in the SNARK (for the GKR verifier) and in pure Go (for the GKR prover.) So this would be used to "register" hash functions in a similar way to hints.
would it be possible to split this PR in 2? 🙄
I think 1. is straightforward and would unblock gnark v0.8.0 release, and the second one needs a bit more cosmetic work, GKR api is not needed for v0.8.0. |
Yes I can break it up again. The fixes for the GKR tests didn't have anything to do with the API so that merge turned out to be unnecessary. |
Reflecting the changes in Consensys/gnark-crypto#308 including tests and stats related to MiMC and Fiat-Shamir challenge name hashing, as well as things depending on it (EdDSA, Merkle Trees, GKR test vectors)