[crypto] Refactor HMAC_SHA to eliminate code duplication #11760

Problem:

• HMAC_SHA256 and HMAC_SHA512 almost the same. Differences are:
  • OUTPUT_SIZE of the hash
  • magic number constants which can be derived from OUTPUT_SIZE
• Code duplication is error prone and causes maintenance issues.

Solution:

• Eliminate magic numbers by making OUTPUT_SIZE a compile-time constant and deriving the other constants from it.
• Compile-time constants permit container type creation, type-based bounds checking, and range-based iteration instead of run-time branching.
• Elimination of run-time eval and magic numbers allows templatization to reduce code duplication.
• Old type names and header files are preserved via using declarations to limit the scope of the change.

Note:

• This is a rethink which takes into account the comments from #11735

I prefer not touching this code. Crypto code like this is hardly ever changed, so the maintenance argument doesn't really hold. Changing this pretty much just adds risk, and review load.

I agree with what @sipa said here: #11735 (comment)

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.

So NACK from me, sorry.

Thanks you that you're trying to contribute, but refactors make very bad initial contributions. If you want to contribute something useful it's better to find an open issue and fix a bug, or add a requested feature.

@laanwj Code always has to be maintained. This is what good testing is for. It allows refactoring with the knowledge that the old behavior is still intact. If there was untested behavior, then we don't really know anyways.

To address the concern raised by @sipa of eliminating code, this commit does eliminate about 50 lines of purely duplicated code.

@ldm5180 You misunderstood me. I was arguing that eliminating code is not useful for this specific case.

@ldm5180 Regardless of this ends up being merged or not (the crypto code has some special "stability requirements") - very nice cleanup! Great to have you on board and looking forward to seeing future contributions from you! :-)

@ldm5180 Saw that you've done some interesting fuzzing of Boost libraries! The Bitcoin fuzzer could be improved a lot so that might be an area where you could make very valuable contributions if you're interested! The problem with the current fuzzer is that it is very shallow – we need the fuzzer to reach deeper into the code base to uncover bugs beyond deserialization issues. Sounds interesting? If so take a look at test_bitcoin_fuzzy.cpp and perhaps @guidovranken's code discussed in #11045 :-)

@practicalswift I will take a look.

The general mood seems to indicate that this might be controversial.

Closing for now.