-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
add BLAKE3 hash #13194
base: master
Are you sure you want to change the base?
add BLAKE3 hash #13194
Conversation
BLAKE3 is a very fast cryptograpically secure hash. It is the latest iteration of the BLAKE hash, which was a SHA3 finalist (but lost to Keccak in the final round, for being too similar to SHA2). Check this speed chart: https://raw.githubusercontent.com/BLAKE3-team/BLAKE3/master/media/speed.svg and this bench.php run where BLAKE3 is much faster than every secure hash, and beats several non-cryptographic hashes: $ sapi/cli/php ext/hash/bench.php crc32b 0.001195 crc32c 0.001202 crc32 0.001202 xxh3 0.001234 xxh128 0.001289 xxh64 0.001475 xxh32 0.002235 murmur3f 0.002459 murmur3c 0.003681 murmur3a 0.004289 adler32 0.007718 blake3 0.007752 md4 0.013109 fnv132 0.015075 fnv164 0.015109 fnv1a64 0.015116 fnv1a32 0.015251 joaat 0.018858 md5 0.019797 sha1 0.020472 tiger160,3 0.021290 tiger192,3 0.021363 tiger128,3 0.021366 tiger128,4 0.027518 tiger160,4 0.027743 tiger192,4 0.027870 ripemd128 0.029190 ripemd256 0.029378 sha3-224 0.029787 sha3-256 0.031518 haval256,3 0.038328 haval224,3 0.038479 haval128,3 0.038483 sha3-384 0.038559 haval192,3 0.038564 haval160,3 0.039068 sha512/256 0.039302 sha512 0.039307 sha512/224 0.039472 sha384 0.039508 ripemd160 0.042287 ripemd320 0.043036 sha3-512 0.054044 haval192,4 0.055699 haval224,4 0.055902 haval160,4 0.055925 haval256,4 0.055948 haval128,4 0.056165 sha256 0.057846 sha224 0.058139 haval128,5 0.070442 haval224,5 0.070503 haval256,5 0.070569 haval192,5 0.070576 haval160,5 0.071109 whirlpool 0.086256 gost 0.200251 gost-crypto 0.200709 snefru256 0.449650 snefru 0.451111 md2 1.237880
That s quite an addition ... worth discussing. |
.. don't know what I'm supposed to do with it, but compiler complains about it missing.
f9f7291
to
bf8a527
Compare
This is indeed a lot of code. I think this should be discussed on the list, and possibly voted on. |
fix seems to be non-trivial, can look into it later.
asked on php-internals mailing list: https://marc.info/?l=php-internals&m=170568974400302&w=2 |
ext/hash/php_hash_blake3.h
Outdated
#define PHP_BLAKE3_CTX blake3_hasher | ||
// help: is V correct? | ||
//#define PHP_BLAKE3_SPEC "b8b8qb64bbbbb1760" | ||
#define PHP_BLAKE3_SPEC "L8L8Qa64CCCCL8Ca1760" |
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.
can someone double-check this? because i couldn't wrap my head around it, and this is just my best guess based on
php-src/ext/hash/blake3/upstream_blake3/c/blake3.h
Lines 42 to 61 in 882466b
typedef struct { | |
uint32_t cv[8]; | |
uint64_t chunk_counter; | |
uint8_t buf[BLAKE3_BLOCK_LEN]; | |
uint8_t buf_len; | |
uint8_t blocks_compressed; | |
uint8_t flags; | |
} blake3_chunk_state; | |
typedef struct { | |
uint32_t key[8]; | |
blake3_chunk_state chunk; | |
uint8_t cv_stack_len; | |
// The stack size is MAX_DEPTH + 1 because we do lazy merging. For example, | |
// with 7 chunks, we have 3 entries in the stack. Adding an 8th chunk | |
// requires a 4th entry, rather than merging everything down to 1, because we | |
// don't know whether more input is coming. This is different from how the | |
// reference implementation does things. | |
uint8_t cv_stack[(BLAKE3_MAX_DEPTH + 1) * BLAKE3_OUT_LEN]; | |
} blake3_hasher; |
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.
How did you come up with this? From what I see in
Lines 211 to 230 in 6d4598e
/* Serialize a hash context according to a `spec` string. | |
Spec contents: | |
b[COUNT] -- serialize COUNT bytes | |
s[COUNT] -- serialize COUNT 16-bit integers | |
l[COUNT] -- serialize COUNT 32-bit integers | |
q[COUNT] -- serialize COUNT 64-bit integers | |
i[COUNT] -- serialize COUNT `int`s | |
B[COUNT] -- skip COUNT bytes | |
S[COUNT], L[COUNT], etc. -- uppercase versions skip instead of read | |
. (must be last character) -- assert that the hash context has exactly | |
this size | |
Example: "llllllb64l16." is the spec for an MD5 context: 6 32-bit | |
integers, followed by 64 bytes, then 16 32-bit integers, and that's | |
exactly the size of the context. | |
The serialization result is an array. Each integer is serialized as a | |
32-bit integer, except that a run of 2 or more bytes is encoded as a | |
string, and each 64-bit integer is serialized as two 32-bit integers, least | |
significant bits first. This allows 32-bit and 64-bit architectures to | |
interchange serialized HashContexts. */ |
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.
Look at line 150, uppercase Q still exists:
Line 150 in 6d4598e
} else if (*spec == 'q' || *spec == 'Q') { |
Pretty sure uppercase C also existed with the meaning unsigned char
or uint8_t
when it was written, but it seems to have been removed! I'll try to re-write it again.. thanks!
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.
Still can't get it quite right, do you see anything off here?
#define PHP_BLAKE3_SPEC /* uint32_t key[8]; */"l8" \
/* uint32_t cv[8]; */ "l8" \
/* uint64_t chunk_counter; */ "q1" \
/* uint8_t buf[BLAKE3_BLOCK_LEN]; */ "b64" \
/* uint8_t buf_len; */ "b1" \
/* uint8_t blocks_compressed */ "b1" \
/* uint8_t flags; */ "b1" \
/* uint8_t cv_stack_len; */ "b1" \
/* uint8_t cv_stack[(BLAKE3_MAX_DEPTH + 1) * BLAKE3_OUT_LEN]; */ "b1760" \
"."
It still hits
Lines 1493 to 1495 in 6d4598e
serialize_failure: | |
zend_throw_exception_ex(NULL, 0, "HashContext for algorithm \"%s\" cannot be serialized", hash->ops->algo); | |
RETURN_THROWS(); |
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 found a possible solution: it seems the compiler insert 5 alignment-padding-bytes after uint8_t buf_len;
,
and an additional 7 padding bytes after uint8_t cv_stack[(BLAKE3_MAX_DEPTH + 1) * BLAKE3_OUT_LEN];
, if the spec is changed to
#define PHP_BLAKE3_SPEC /* uint32_t key[8]; */"l8" \
/* uint32_t cv[8]; */ "l8" \
/* uint64_t chunk_counter; */ "q" \
/* uint8_t buf[BLAKE3_BLOCK_LEN]; */ "b64" \
/* uint8_t buf_len; */ "b" \
/* skip 5 bytes of alignment padding in chunk */ "B5" \
/* uint8_t blocks_compressed */ "b" \
/* uint8_t flags; */ "b" \
/* uint8_t cv_stack_len; */ "b" \
/* uint8_t cv_stack[(BLAKE3_MAX_DEPTH + 1) * BLAKE3_OUT_LEN]; */ "b1760" \
/* skip 7 trailing alignment bytes */ "B7" \
"."
serialization actually works on my system!
But this seems like the kind of thing different compilers, and even the same compiler with different optimization settings, may choose to do differently. For example, I wouldn't trust gcc -Ofast
(optimize-for-speed) and gcc -Os
(optimize-for-size) to do the exact same alignments paddings in the exact same location 🤔 Nor would I trust the compiler to do the exact same alignment padding across 32bit and 64bit builds..
I might be wrong but.. idk!
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.
you could maybe add one uint8_t reserved field and then on 64bit reserved2 field of uint32_t type which should hopefully make it defined on all platforms.
somehow didn't see that message until after pushing 8762e32 🤔 but I like that better. I'll run some tests locally
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 5 bytes is correct only on 64bit but it should not be correct on 32bit. Also the compiler is free to choose how to do padding so your choice of the padding place is basically relaying on undefined behaviour and it could theoretical differ between compilers. As I mentioned, the only way how to be sure is to define fields as compiler must not re-order them.
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.
From what I see, there's also option to define your own serialization function but not sure if it would make it any simpler.
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.
Well it could be cleaner because it wouldn't rely on struct memory layout...
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 5 bytes is correct only on 64bit but it should not be correct on 32bit
well 263b2f6 works on 32bit (tested locally on a debian 12 32bit docker image, and now confirmed on the 32bit github CI runner)
From what I see, there's also option to define your own serialization function but not sure if it would make it any simpler.
hmm could you link to an example? I haven't seen it. A custom serializer sounds like the safest bet across compilers/architectures/configurations
post on the mailing list worth re-posting here: wrote some benchmarks, but the TL;DR is: array(6) {
["O2-portable-march"]=>
array(2) {
["microseconds_for_16_kib"]=>
int(29295)
["mb_per_second"]=>
float(533.3674688513398)
}
["O2-portable"]=>
array(2) {
["microseconds_for_16_kib"]=>
int(13876)
["mb_per_second"]=>
float(1126.0449697319111)
}
["O2-sse2"]=>
array(2) {
["microseconds_for_16_kib"]=>
int(4969)
["mb_per_second"]=>
float(3144.4958744214127)
}
["O2-sse41"]=>
array(2) {
["microseconds_for_16_kib"]=>
int(4688)
["mb_per_second"]=>
float(3332.977815699659)
}
["O2-avx2"]=>
array(2) {
["microseconds_for_16_kib"]=>
int(2384)
["mb_per_second"]=>
float(6554.1107382550335)
}
["O2-avx512"]=>
array(2) {
["microseconds_for_16_kib"]=>
int(1753)
["mb_per_second"]=>
float(8913.291500285226)
}
} Edit: -O2 portable: 596MB/s Again, even with -march=native, the compiler cannot make the portable |
Co-authored-by: Peter Kokot <[email protected]>
Co-authored-by: Peter Kokot <[email protected]>
Co-authored-by: Peter Kokot <[email protected]>
Having this would be good |
More than 2 years since the first attempt to get Blake3 into PHP, why is it so hard? |
the main problem this round was that I just lost steam. Got occupied with other stuff and forgot about this. You reminded me though! |
BLAKE3 is very nice, we need that algorithm in php |
Seems like this will never be merged, such a shame as a lot of blockchain are starting to use blake3 as standard |
Fwiw this should be rebased against the newest blake3 release, iirc (on phone now can't check, but iirc) this pr currently contains 3 patches to blake3 with a dedicated patch file, but all 3 patches have been accepted upstream and is part of the newest Blake3 official release |
Scratch that, BLAKE3-team/BLAKE3#382 is still not resolved upstream, the other 2 are resolved upstream tho. Edit: also had to add BLAKE3-team/BLAKE3#443 |
caused ``` /Users/runner/work/php-src/php-src/ext/hash/blake3/upstream_blake3/c/blake3_dispatch.c:237:26: error: unused variable 'features' [-Werror,-Wunused-variable] const enum cpu_feature features = get_cpu_features(); ``` on 32bit x86-32 linux builds.
I'm requesting re-review. |
-1 from me I think it is a terrible idea to bundle more crypto stuff in PHP Is there an RFC for this ? |
@remicollet imo we should support BLAKE3 for the same reason we support xxHash (added 8.1.0): speed. BLAKE3 offers SHA3-256-like security at much higher speed than SHA3-256. Quoting Exactly how we support BLAKE3 is not that important, if OpenSSL/Sodium starts offering BLAKE3, I wouldn't mind requiring OpenSSL/Sodium for BLAKE3 support. But they don't support BLAKE3 (yet?). As for RFC, made a draft a year ago, but now I can't even find it. I'll make a new one. |
So effort should go there, not here. |
RFC draft: https://wiki.php.net/rfc/blake3 |
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.
Except the missing serialization, it seem ok to me. It would be also good to add a bit more tests to test some predefined vectors to be sure all is good.
I think it's good already for the RFC so you can announce it.
In terms of the maintanence, the hash extension bundles already quite a few of algos so this is just another one. It's taken from upstream like some other algorithms so I don't think it matters that much the amount of code. The upstream seems to be quite well maintained.
ext/hash/php_hash_blake3.h
Outdated
#define PHP_BLAKE3_CTX blake3_hasher | ||
// help: is V correct? | ||
//#define PHP_BLAKE3_SPEC "b8b8qb64bbbbb1760" | ||
#define PHP_BLAKE3_SPEC "L8L8Qa64CCCCL8Ca1760" |
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.
How did you come up with this? From what I see in
Lines 211 to 230 in 6d4598e
/* Serialize a hash context according to a `spec` string. | |
Spec contents: | |
b[COUNT] -- serialize COUNT bytes | |
s[COUNT] -- serialize COUNT 16-bit integers | |
l[COUNT] -- serialize COUNT 32-bit integers | |
q[COUNT] -- serialize COUNT 64-bit integers | |
i[COUNT] -- serialize COUNT `int`s | |
B[COUNT] -- skip COUNT bytes | |
S[COUNT], L[COUNT], etc. -- uppercase versions skip instead of read | |
. (must be last character) -- assert that the hash context has exactly | |
this size | |
Example: "llllllb64l16." is the spec for an MD5 context: 6 32-bit | |
integers, followed by 64 bytes, then 16 32-bit integers, and that's | |
exactly the size of the context. | |
The serialization result is an array. Each integer is serialized as a | |
32-bit integer, except that a run of 2 or more bytes is encoded as a | |
string, and each 64-bit integer is serialized as two 32-bit integers, least | |
significant bits first. This allows 32-bit and 64-bit architectures to | |
interchange serialized HashContexts. */ |
return in_array($algo, [ | ||
"xxh3", | ||
"xxh128", | ||
"blake3", // todo: blake3 can be seralized but it's not implemented yet |
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.
Obviously this should be changed before merging...
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 suspect PHP_BLAKE3_SPEC is wrong, and that it breaks serialization, but I haven't been able to figure it out yet 🤔
@@ -73,6 +106,8 @@ PHP_INSTALL_HEADERS([ext/hash], m4_normalize([ | |||
php_hash_ripemd.h | |||
php_hash_sha.h | |||
php_hash_sha3.h | |||
php_hash_blake3.h | |||
blake3/upstream_blake3/c/blake3.h |
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.
Why is there that extra upstream_blake3 directory. Cannot this go directly to blake3?
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 directory separate code written for the php-src integration from the substantial amount of code cloned directly from https://github.com/BLAKE3-team/BLAKE3/
Cannot this go directly to blake3?
It can, but I'd prefer if it didn't.
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 don't really understand why it needs extra subdirectory. The blake3 directly contains only upstream_blake3 directory which really doesn't make any sense to me. And you also have c directory which I can see is from upstream. So it should really be just blake3/c .
Co-authored-by: Jakub Zelenka <[email protected]>
Co-authored-by: Jakub Zelenka <[email protected]>
ERROR 2003 (HY000): Can't connect to MySQL server on 'localhost:3306' (10061) https://github.com/php/php-src/actions/runs/12949014301/job/36118857251?pr=13194
Right now there is a problem with https://dev.mysql.com/ causing Windows CI's to fail, specifically https://dev.mysql.com/get/Downloads/MySQL-8.0/mysql-8.0.31-winx64.zip returns HTTP 403 Forbidden. It's not this PR's fault 🤔 edit: made a dedicated issue for it: #17561 |
there's padding issues, idk how portable this is... The CI's should be interesting.
I don't have a 32bit system to test on locally, so testing on CI for now..
BLAKE3 is a very fast cryptographically secure hash. It is the latest iteration of the BLAKE hash, which was a SHA3 finalist (but lost to Keccak in the final round, for being too similar to SHA2).
RFC: https://wiki.php.net/rfc/blake3
Check this speed chart:
and this
/ext/hash/bench.php
run where BLAKE3 is much faster than every other secure hash, and beats several non-cryptographic hashes (AMD Ryzen 9 7950x):Quoting Google Bard: