Remove midstate from the GeneralHash and HashEngine traits#3009
Remove midstate from the GeneralHash and HashEngine traits#3009apoelstra merged 1 commit intorust-bitcoin:masterfrom
GeneralHash and HashEngine traits#3009Conversation
@apoelstra please confirm claim is correct. |
11c56ad to
8f491a8
Compare
|
Concept ACK, will wait for @apoelstra to confirm that removing midstate from everything that's not SHA256 is desirable. |
|
It's absolutely not "just for creating tagged hashes". But I haven't seen midstates of non-sha256 functions used anywhere in practice, so I think we should delete them for now until somebody complains, and then we can put them back. |
It'd be nice to know the other use cases better as they can inform us on API decisions. |
|
One is in Bitcoin mining -- you want to store the progress of your hash computation so that you can iterate on different continuations rather than restarting every time (this is "ASICboost"). Another is "fast merkle roots" as implemented in Elements consensus logic. (I think this might appear in Bitcoin Core as well, but it's not in consensus anywhere.) |
apoelstra
left a comment
There was a problem hiding this comment.
ACK 8f491a81481d44d6f9679fcc75981f1cf0af1ad7
Thanks for the explanation, so this should be "is only useful in the bitcoin ecosystem for sha256 hashes and shouldn't be part of the |
|
Yeah. I think a clearer description is maybe "midstates are not generic objects; they don't have universal cryptographic properties and if you are using them you should be using a specific midstate type. Therefore it shouldn't be part of |
8f491a8 to
1c0a97e
Compare
Midstates are not generic objects; they don't have universal cryptographic properties and if you are using them you should be using a specific midstate type. Therefore it shouldn't be part of `GeneralHash` or `HashEngine`. Furthermore, in practice it seems like `sha2` midstates are the only ones that anybody uses, at least in bitcoin. Remove the midstate stuff from the `GeneralHash` and `HashEngine` traits. Keep the `midstate` functionality as inherent functions if it is used internally. Keep the functionality on `sha256` as inherent public functions.
1c0a97e to
8dcecfc
Compare
GeneralHash and HashEngine traits
|
PR description, title, and commit log updated to use new text. No code changes. |
| type MidState = Midstate; | ||
|
|
||
| /// Outputs the midstate of the hash engine. This function should not be | ||
| /// used directly unless you really know what you're doing. |
There was a problem hiding this comment.
Expand and format the doc, please?
There was a problem hiding this comment.
This PR is a move-and-remove-only change. Would it be ok to do this in #3010 instead where we've had more discussion on this point?
There was a problem hiding this comment.
Oh, I've confused myself, yeah.
|
Can this be merged as is @apoelstra, the semver CI fail is unrelated (#3018). |
Midstates are not generic objects; they don't have universal cryptographic properties and if you are using them you should be using a specific midstate type. Therefore it shouldn't be part of
GeneralHashorHashEngine. Furthermore, in practice it seems likesha2midstates are the only ones that anybody uses, at least in bitcoin.Remove the midstate stuff from the
GeneralHashandHashEnginetraits. Keep themidstatefunctionality as inherent functions if it is used internally. Keep the functionality onsha256as inherent public functions.Done as a first step towards #2918.