Problem:
Solution:
OUTPUT_SIZE to be constexpr so that it can be used in
array sizes.rkey which can now
be simplified because it is a std::array instead of a C-array.
Problem:
- Magic numbers are used to determine sizes, but are error prone.
Solution:
- Change `OUTPUT_SIZE` to be `constexpr` so that it can be used in
array sizes.
- Use arrays so the size is carried with the variable instead of
external computation.
- Cleanup initialization and further setting of `rkey` which can now
be simplified because it is a `std::array` instead of a C-array.
Any style changes should probably be kept consistent across the following files :-)
src/crypto/hmac_sha256.h
src/crypto/hmac_sha512.h
src/crypto/ripemd160.h
src/crypto/sha1.h
src/crypto/sha256.h
src/crypto/sha512.h
src/hash.h
Please read: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring
I don't like the magic numbers this PR tries to change, though I less like changing some of the magic values. Also the code is less readable afterwards (IMO).
It's like with code formatting. Lets just accept how it is and concentrate on new code.
I intend to move this to a more compile-time and compiler-optimizer friendly generic implementation of all of the crypto algos. This is just step 1. I think most of this code can be eliminated through usage of templates.
Since I am new, I am trying to adhere to the CONTRIBUTING.md guidelines. @jonasschnelli, can you point to which ones you want me to focus on more directly?
I'm not convinced about the long term goal of eliminating code through templates. Cryptographic code like this is incredibly stable, so the usual reasons for doing refactoring like this (easier maintenance) don't really apply - it's likely possible to not ever touch it.
However, as such all the changes here look fine to me. Not sure the auto variables are necessary, but moving to a more C++ish style doesn't sound bad.
Concept ACK
24 | + CSHA256().Write(key, keylen).Finalize(rkey.data()); 25 | + std::fill(rkey.begin() + OUTPUT_SIZE, rkey.end(), 0); 26 | } 27 | 28 | - for (int n = 0; n < 64; n++) 29 | + for (auto n = 0u; n < rkey.size(); n++)
I'd prefer not to use 'auto' here, keeping integer signedness explicit makes it easier to reason about.
Concept ACK modulo @sipa's feedback and assuming that any changes performed are applied consistently across:
src/crypto/hmac_sha256.h
src/crypto/hmac_sha512.h
src/crypto/ripemd160.h
src/crypto/sha1.h
src/crypto/sha256.h
src/crypto/sha512.h
src/hash.h
I agree with @sipa. This code (and the other algorithm implementations) are critically important, already very stable, and could probably exist "as is" for a very long time.
Refactoring for the sake of refactoring or "clean up" is basically all risk, with seemingly very little potential reward.
What there is the potential for is introducing subtle/obscure bugs. Amplified by the fact that the group of reviewers for this kind of the work is quite small. @ldm5180 If you're planning on large scale refactoring of "all of the crypto algos", it'd be great if you could open an issue and outline what some of your ideas and goals are. i.e Performance, making the code more readable, reusable etc? Otherwise, continual changes to this code are likely to sit in limbo.
I will re-write this an reopen when ready.
