Skip to content

Conversation

@darenliang
Copy link

This PR aims to remove the need to reverse the bytes slice when using math/big. I also made some of the variable/type naming more consistent (Go uses camel-case) and fixed a few typos.

I saw that there was an issue with math/big using primarily big-endian in its package. Oddly, I came across this function https://pkg.go.dev/math/big#Int.SetBits that uses little-endian.

It's possible (in a rather hacky way) to use the unsafe package to type cast the bytes slice into a big.Word slice and use SetBits without the need to reverse a slice.

Here are the given benchmark tests, showing a slight improvement.

With reverse

BenchmarkBalloon
BenchmarkBalloon-4          6135           1736703 ns/op
BenchmarkBalloonM
BenchmarkBalloonM-4         4665           2638889 ns/op

Without reverse

BenchmarkBalloon
BenchmarkBalloon-4          6474           1692983 ns/op
BenchmarkBalloonM
BenchmarkBalloonM-4         4732           2511378 ns/op

I think more can be squeezed out of this hashing implementation, but it'll need more looking into.

Also enforce naming conventions and fix typos
@ecton
Copy link
Member

ecton commented Apr 10, 2022

CLA assistant check
All committers have signed the CLA.

@daxpedda
Copy link
Member

daxpedda commented Jun 9, 2022

I apparently wasn't subscribed to this repo, so I didn't see your PR. I don't really feel qualified to review this, my knowledge of Go is not limited, it's zero. So whenever @nogoegst get's around reviewing the original PR, he can take a look here too.

Thanks for looking into it though!

@darenliang
Copy link
Author

It is all good,

I was learning about balloon hashing and came across your PR. After looking at the password-hashes crate, I'm jealous of Rust devs. I really hope someone can create a Go library with a collection of modern hashing algs.

// Int.SetBits. We can unsafely convert otherBytes directly
// into a big.Word slice to bypass the need to reverse
// otherBytes.
header := *(*reflect.SliceHeader)(unsafe.Pointer(&otherBytes))

Choose a reason for hiding this comment

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

There should not be reflect and unsafe in crypto code. Also, it's hard to read (and to test).

Copy link
Member

Choose a reason for hiding this comment

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

I did manage to convert it without unsafe in the original PR, not sure if it can be improved any further though.

otherBytes := h.Sum(nil)
otherWords := make([]big.Word, h.Size() / (bits.UintSize / 8))


for byte, word := 0, 0; byte < len(otherBytes); byte, word = byte+bits.UintSize/8, word+1 {
	if bits.UintSize == 64 {
		otherWords[word] = big.Word(binary.LittleEndian.Uint64(otherBytes[byte : byte+bits.UintSize/8]))
	} else if bits.UintSize == 32 {
		otherWords[word] = big.Word(binary.LittleEndian.Uint32(otherBytes[byte : byte+bits.UintSize/8]))
	} else {
		panic("balloon: unsupported architecture")
	}
}


otherInt.SetBits(otherWords)

See https://github.com/nogoegst/balloon/pull/2/files#diff-43f7147f39b6ce50c058187412aeffffd6b79bf6c972dc475b1a86a674597901R128-R141.

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.

4 participants