Add sha256_buffer to PoolByteArray

[chiguireitor]

This PR includes sha256_buffer to PoolByteArray (sha256_string should be refactored to sha256_text to keep consistency with string methods)

[chiguireitor]

We're using this in our GDScript crypto library, and would like to mainline it so advanced uses can be done with Godot's native script.

[akien-mga]

CC @Faless - does it make sense to add this or does #30239 provide the sha256 sum as a PoolByteArray too?

[chiguireitor]

That's for files from what i gather (and tested on latest compiled godot), and it returns the sha256 as an hex string.

For cryptocurrency manipulation stuff you need unmangled bytes to be processed by sha256, so a string isn't fit as it trims at any 0x00 char.

Also, code is much cleaner using just PBAs for handling this and finally converting everything to your needed format when finalized.

[chiguireitor]

Just to be clear, i'm porting a BigNumber (arbitrary precision arithmethic) library to GDScript, Elliptic Curve cryptography, RipeMD160, base58 and several Bitcoin functions (message signing, tx building, etc). Without proper sha256 manipulation on unmangled PBAs the Bitcoin stuff is almost impossible (doublesha256 + ripemd160 for address building, etc) due to "String" mangling the characters with 0x00 as value. Using a clean PBA with sha256 solves all this.

[Faless]

This will be addressed soon via #29871 which allows to perform different hashes (MD5, SHA1, SHA256) PoolBytesArray via a single API (HashingContext class).

[chiguireitor]

This will be addressed soon via #29871 which allows to perform different hashes (MD5, SHA1, SHA256) PoolBytesArray via a single API (HashingContext class).

Ok, that works too, and to boot has the random bytes call, which we had to solve via "random mouse movement of the user".

However, if the sha256_text call is making the cut to the PBA code on 3.2 (as it's already on master), it wouldn't hurt anyone to include this PR too, as it completes the functionality and causes less confusion to the uninitiated.

[Faless]

I would honestly prefer deprecating the functions on strings/poolbytesarray regarding hashing and move everything to the new HashContext class removing most of the code from variantcall.
But this is probably a matter for discussion. Maybe do it in 4.0 instead?

[chiguireitor]

As this is a minor revision, feature completeness is more important than correctness imho (also, users would expect little to no breaking changed from 3.1 to 3.2).

I'm not a main dev here, but i would feel more comfortable with breaking changes making the cut on 4.0 than 3.2. Maybe add a deprecation notice in the documentation

[Faless]

I'm not a main dev here, but i would feel more comfortable with breaking changes making the cut on 4.0 than 3.2. Maybe add a deprecation notice in the documentation

Yes, ofc, deprecation will maintain the current funcitons, show warnings in editor/docs, and then in 4.0 deprecated functions will be dropped.
This is why adding a function that is already deprecated seems confusing.

[chiguireitor]

Indeed, it's weird, but i gather that some people will get confused if they happen to need it and just see the string version but not the pba version.

And i gather 4.0 could be a while off, so again it's just my opinion, but you want users seeing a more complete feature set (although with deprecation notices) than something that looks kinda half done because there's a coming update (and this updates addresses that issue with almost 0 fluff).

Btw, i would rather prefer using your PR, it's more like what one does in nodejs and python, would be nice if it exposed the native ripemd hash in the same way (although afaik, that's not provided by mbedtls).

[akien-mga]

The plan is to include #29871 in 3.2, so it's maybe better that we don't add new APIs next to it that would be meant to be removed in 4.0.

If we decide not to merge this PR (#31187) and we do merge #29871 soon, maybe we should also revert #24269 which was merged during 3.2 development too (so reverting it now doesn't really break compat, only for users of the master branch)?

[chiguireitor]

Makes sense. I'm ok with consistency here, so if there's a better alternative that has the same end result, much better for everyone.

However, leaving PBA with half of the functionality out would be very confusing at best, so the approach of reverting in favor of the new functionality is much cleaner imho.

[akien-mga]

I opened #31560 to revert sha256_string. I guess we can also close this PR now, but thanks for the proposal and the discussion :)

[Henry-E]

So, Is it possible to use base58 encoding in godot? And how can I call it. Thanks!

[chiguireitor]

Afaik, it's not in. However, adding a C++ library for that is kinda easy if you're fluent in C++ ofc.

[Henry-E]

ah ok, so I could just make a custom module. I will look into that!