crypto.sha3: add support for Keccak-256 and Keccak-512

[syobocat]

Closes #23057

_{Huly®: V_0.6-21498}

[spytheman]

@blackshirt, @kimshrier can you please review this PR?

[kimshrier]

It would be good to add keccak-256 and keccak-512 tests to test_200_length_hash() so that there is more than one validation of the algorithm. Also, since the existing tests are against an empty string, that is potentially a degenerate case. Testing against a non-empty string will give us better confidence in the correctness of the implementation.

[blackshirt]

Is it good to this sha3 and the kecak version to adhere hash.Hash interface?

[blackshirt]

There are many duplicates here, likes new_keccak_digest and other specialized module level fns, even its just a thin wrapper for other function. Its seem (for me) there are many ways to do a same thing here, ..
I think @spytheman and others has more thinking in this area's
I'm personally like optional parameters with default to sha3 part. For example from standard math.big.integer_from_bytes accepts optional IntegerConfig as the last parameters. when its not supplied, its assumed signum to be positive 1.

something similar like this (just the raw idea)

@[params]
struct DigestOptions {
     // currently, only single option
     pad u8 = hash_pad // default to sha3
}

fn new_digest_common(absorption_rate int, hash_size int, opt DigestOptions) !&Digest {
        if opt.pad != hash_pad && opt.pad != xof_pad && opt.pad != kecak_pad {
          return error('bad pad')
       }
       // just performs other required check
	d := Digest{
		rate:       absorption_rate
		suffix:     opt.pad
		output_len: hash_size
		s:          State{}
	}
       return &d
}

So, the others can use this single comon constructor, and eliminates non-required similar fns

pub fn new512keccak() !&Digest {
	return new_digest_common(rate_512, size_512, pad: keccak_pad)!
}

[blackshirt]

Maybe you should add the function docs to elaborate the current behaviour of the new_digest, so the user know the created Digest was expected digest.
@spytheman what do you think ?

[spytheman]

I agree.

[blackshirt]

Can you please also adds some normal tests, that do import crypto.sha3 and only use the public API of the module? Especially (buat not only) for new_digest function, because its changed to accept options.
Its usually good to know its not breaks anything and working normally

[blackshirt]

Its doesn't import crypto.sha3 module at the top of your test file...
Just create normal usage test file with required import. For example, see poly1305 usage_test.v

[blackshirt]

I think overall changes is mostly acceptable now, with some notes on unhandled issues (maybe can be addressed on next iteration of pr), ie,

• is this sha3.Digest should adhere to hash.Hash interface? I think its good if yes, because the standard stock of sha1, sha256 and sha512 has already did it.
• Its good to have README file for this sha3 module. Its not yet availables at this time of writing, and for more crucial reasons, this module has support for several variant of hashing instead of standard sha3 thats represented by this module name.

Good jobs guys...
Waiting for other review

[JalonSolov]

Yes, it should adhere to the hash.Hash interface. Conformity is a good thing, for related functions.

These new variants should also be added to the enum Hash mentioned in the main crypto/README.md, as well as more specific docs added somewhere... perhaps the main crypto/README/md file should be expanded with at least a paragraph for each type, instead of a couple of simple examples and the Hash.

[JalonSolov]

Yay - more int <-> u32 issues. @spytheman

[spytheman]

Yay - more int <-> u32 issues. @spytheman

it is fixed in master - the CI runs just happened to interleave

[spytheman]

I think that it is good to be merged - the remaining things are out of scope of this particular PR, and require changes to existing code. They are better to be done separately imho.

[spytheman]

Excellent work @syobocat 🙇🏻‍♂️ .

[blackshirt]

I think that it is good to be merged - the remaining things are out of scope of this particular PR, and require changes to existing code. They are better to bepr done separately imho.

Its good, the other concerns can be done on next pr