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

bug: document clearly that the hash function in std/recursion is not collision-resistant when inputs are of different length #1141

Closed
ivokub opened this issue May 24, 2024 · 0 comments · Fixed by #1198
Assignees
Labels
bug Something isn't working consolidate strengthen an existing feature doc-change-required Indicates an issue or PR that requires doc to be updated

Comments

@ivokub
Copy link
Collaborator

ivokub commented May 24, 2024

Description

The short-hash defined in std/recursion is used as an efficient way to compute the challenges inside the circuit. For compatibility with existing marshalling schemes and field mismatch, we work on the binary decomposition of the inputs (bits in-circuit, bytes natively). However, in this approach if we have inputs of different length, they can represent same element value, e.g.

func TestCollision(t *testing.T) {
	h, err := recursion.NewShort(ecc.BLS12_377.ScalarField(), ecc.BN254.ScalarField())
	if err != nil {
		panic(err)
	}
	println(ecc.BLS12_377.ScalarField().BitLen())
	println(ecc.BN254.ScalarField().BitLen())
	h.Write([]byte{0x00})
	hash := h.Sum(nil)
	fmt.Printf("%x\n", hash) // 34373b65f439c874734ff51ea349327c140cde2e47a933146e6f9f2ad8eb17
	h.Reset()

	h.Write([]byte{0x00})
	h.Write([]byte{0x00})
	fmt.Printf("%x\n", h.Sum(nil)) //34373b65f439c874734ff51ea349327c140cde2e47a933146e6f9f2ad8eb17
}

We do not consider this as an issue currently as the hash function is really meant to be use for in-circuit verifier where the lengths are fixed. We need to add a warning in the constructor function to clearly state that the hash function is not collision resistant in case of different input lengths. Better approach would be if we are able to make the hash function internal and not usable outside of gnark std, but we need to consider that short-hash must be passable to PLONK prover.

@ivokub ivokub added bug Something isn't working doc-change-required Indicates an issue or PR that requires doc to be updated consolidate strengthen an existing feature labels May 24, 2024
@ivokub ivokub self-assigned this May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working consolidate strengthen an existing feature doc-change-required Indicates an issue or PR that requires doc to be updated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant