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

Feat/add decompose mimc #265

Merged
merged 10 commits into from
Jan 5, 2023
Merged

Feat/add decompose mimc #265

merged 10 commits into from
Jan 5, 2023

Conversation

ThomasPiellard
Copy link
Contributor

@ThomasPiellard ThomasPiellard commented Nov 15, 2022

Fixes mimc collision problem.

Now the buffer in the mimc function (which is a slice of bytes) is interpreted as a bigInt written in basis r, the modulus of the snark field, so that there is a one-to-one correspondence between []byte and sequence (a_i)_i of elements of Fr, preventing collisions.

Before this PR, the buffer was cut in pieces of size fr.Bytes long, which could result in collisions: if x is the data to hash and r the modulus, then if x+r fits on fr.Bytes bytes then mimc(x)=mimc(x+r) since the reduction mod r of x and x+r is the same.

Thanks to @zhiqiangxu for pointing the issue.

Copy link
Collaborator

@gbotrel gbotrel left a comment

Choose a reason for hiding this comment

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

🔴 --> the performance impact of this is too high. In various context, we may want to compute large (or many) hashes, and just adding the following benchmark confirms my fear:

func BenchmarkMiMC(b *testing.B) {
	const n = 1 << 18
	rand.Seed(time.Now().UnixNano())
	var input [n]byte
	_, err := rand.Read(input[:])
	if err != nil {
		b.Fatal(err)
	}

	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		h := NewMiMC()
		h.Write(input[:])
		_ = h.Sum(nil)
	}
}

Results:

name     old time/op    new time/op    delta
MiMC-10    62.4ms ± 1%  5908.0ms ± 0%  +9368.58%  (p=0.100 n=3+3)

name     old alloc/op   new alloc/op   delta
MiMC-10    24.2MB ± 0%  2280.8MB ± 0%  +9326.85%  (p=0.100 n=3+3)

name     old allocs/op  new allocs/op  delta
MiMC-10      752k ± 0%      775k ± 0%     +2.98%  (p=0.100 n=3+3)

@gbotrel
Copy link
Collaborator

gbotrel commented Dec 1, 2022

@ThomasPiellard any progress on this one?

@gbotrel
Copy link
Collaborator

gbotrel commented Dec 16, 2022

can make that 2-3x faster by replacing the 2 large big.int division by a single one;

tmp.SetBigInt(rawBigInt) // tmp <- rawBitInt [r]
res = append(res, tmp)
rawBigInt.Div(rawBigInt, modulo)

can be

rawBigInt.DivMod(rawBigInt, modulo, t)
tmp.SetBigInt(t)

Then the decompose function returns (and allocate) a list of field elements, but this is not needed in the checksum function. The checksum should do the decomposition and call encrypt on a temporary element.

Even with this the perf is terrible -- unless we find a smarter way to decompose the huge big int (maybe starting from the right bytes with blocksize and propagating a carry could work?), maybe we should consider the option of taking 1 less byte per block to avoid the issue?

@@ -89,7 +89,6 @@ func (d *digest) Write(p []byte) (n int, err error) {
// The XOR operation is replaced by field addition, data is in Montgomery form
func (d *digest) checksum() fr.Element {

var buffer [BlockSize]byte
var x fr.Element
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ThomasPiellard this variable x is always zero; yet it appears in the loop below as d.h.Add(&r, &d.h).Add(&d.h, &x) is this a mistake?

@gbotrel gbotrel merged commit 2576e10 into develop Jan 5, 2023
@gbotrel gbotrel deleted the feat/add_decompose_mimc branch January 5, 2023 18:04
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.

Possible MiMC bug Buffer in mimc should reason on p-digits, not block size
3 participants