Refactoring CHashWriter & Get{Prevouts,Sequence,Outputs}Hash to SHA256 (Alternative to #18071) #19601
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
I have mild preference for other PR but utACK 2f45092 |
src/script/interpreter.cpp
Outdated
There was a problem hiding this comment.
Why this weird double assignment? = SHA256Uint256(GetPrevoutsSHA256(txTo)) is shorter and more clear IMO.
There was a problem hiding this comment.
This PR was made before #17977 (comment) was added (or before I saw it at least).
I specifically made it so it would be easy on rebase to switch the first assignment and argument to SHA256Uint256 to m_hash_prevouts for taproot, but now since that logic is all different it's not clear that this is an easier rebase. In any case, this part of the patch was designed so that master is correct, but was intended to be blown out by #17977.
There was a problem hiding this comment.
Don't worry about such trivial rebasing impact on #17977.
|
I prefer this PR over #18071 (but see nit above). |
src/hash.h
Outdated
There was a problem hiding this comment.
Perhaps add NODISCARD here to guard against the issue you pointed out in the other PR.
|
Concept ACK I prefer this PR over the alternative (#1807)1: this one is simpler and thus more likely to be used correctly by callers. Adding |
JeremyRubin
force-pushed
the
refactoring-hashers-2
branch
from
2f45092 to
a8b95a3
Compare
JeremyRubin
force-pushed
the
refactoring-hashers-2
branch
from
a8b95a3 to
8986838
Compare
|
rebased and added NODISCARD. |
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
utACK 8986838d3a4d52ce912e2ce385ec87a4a3d34cad.
One request for additional comments inline.
…Outputs}SHA256. Several proposals (Taproot, MuHash, CTV) require access to the single hash.
JeremyRubin
force-pushed
the
refactoring-hashers-2
branch
from
8986838 to
9ab4caf
Compare
|
Code review ACK 9ab4caf |
|
Nit: Could add unit tests for the newly introduced functions? |
There was a problem hiding this comment.
tested ACK 9ab4caf
On tests: I think it's ok as is in this case because CSHA256 is tested extensively and the GetHash() function is used in several test cases as a utility. If the GetSHA256() would be used inside GetHash() it would be covered through those as well.
AFAICT I don't think we should settle with "OK testing" for consensus critical code -- we want excellent testing :) |
|
We should be hitting the PrecomputedTransactionData init all across the tests, and then validation would fail in numerous places if those have an error. |
|
In fact I'd go as far as to say if this code isn't already widely tested, we have bigger problems (because it would mean we have no tests covering our PrecomputedTransactionData). |
|
It's also an absolutely trivial function. Testing is important, but there is no point in wasting effort on more than a covering test for a single-line wrapper around otherwise very well-tested code. |
|
@sipa @instagibbs mind re-reviewing this? It's a fairly basic change and it'd be nice to get it merged to simplify #19055 and #17977 |
|
reACK 9ab4caf |
…Outputs}Hash to SHA256 (Alternative to bitcoin#18071) 9ab4caf Refactor Get{Prevout,Sequence,Outputs}Hash to Get{Prevouts,Sequences,Outputs}SHA256. (Jeremy Rubin) 6510d0f Add SHA256Uint256 helper functions (Jeremy Rubin) b475d7d Add single sha256 call to CHashWriter (Jeremy Rubin) Pull request description: Opened as an alternative to bitcoin#18071 to be more similar to bitcoin#17977. I'm fine with either, deferring to others. cc jnewbery Sjors ACKs for top commit: jnewbery: Code review ACK 9ab4caf jonatack: Tested ACK 9ab4caf fjahr: tested ACK 9ab4caf instagibbs: reACK bitcoin@9ab4caf Tree-SHA512: 93a7a47697f1657f027b18407bdcce16963f6b23d12372e7ac8fd4ee96769b3e2639369f9956fee669cc881b6338641cddfeeef1516c7104cb50ef4b880bb0a7
9 participants
Opened as an alternative to #18071 to be more similar to #17977.
I'm fine with either, deferring to others.
cc jnewbery Sjors