Refactoring CHashWriter & Get{Prevouts,Sequence,Outputs}Hash to SHA256 (Alternative to #18071) #19601

pull JeremyRubin wants to merge 3 commits into bitcoin:master from JeremyRubin:refactoring-hashers-2 changing 3 files +46 −19

JeremyRubin commented at 8:02 PM on July 27, 2020: contributor

Opened as an alternative to #18071 to be more similar to #17977.

I'm fine with either, deferring to others.

cc jnewbery Sjors
DrahtBot added the label Consensus on Jul 27, 2020
DrahtBot commented at 11:26 PM on July 27, 2020: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #17977 (Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa)
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.
instagibbs commented at 8:39 PM on July 30, 2020: member

I have mild preference for other PR but

utACK https://github.com/bitcoin/bitcoin/pull/19601/commits/2f450929c8fa29a56afe0a05cc482cea71c23ada
DrahtBot added the label Needs rebase on Aug 3, 2020
in src/script/interpreter.cpp:1300 in 2f450929c8 outdated
```
1296 | @@ -1297,9 +1297,12 @@ void PrecomputedTransactionData::Init(const T& txTo)
1297 |  
1298 |      // Cache is calculated only for transactions with witness
1299 |      if (txTo.HasWitness()) {
1300 | -        hashPrevouts = GetPrevoutHash(txTo);
1301 | -        hashSequence = GetSequenceHash(txTo);
1302 | -        hashOutputs = GetOutputsHash(txTo);
1303 | +        hashPrevouts = GetPrevoutsSHA256(txTo);
```
sipa commented at 8:02 PM on August 3, 2020:

Why this weird double assignment? = SHA256Uint256(GetPrevoutsSHA256(txTo)) is shorter and more clear IMO.

JeremyRubin commented at 12:28 AM on August 4, 2020:

This PR was made before #17977 (review) 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.

sipa commented at 7:34 PM on August 4, 2020:

Don't worry about such trivial rebasing impact on #17977.
sipa commented at 7:35 PM on August 4, 2020: member

I prefer this PR over #18071 (but see nit above).

in src/hash.h:217 in 2f450929c8 outdated

 212 | @@ -200,6 +213,9 @@ uint256 SerializeHash(const T& obj, int nType=SER_GETHASH, int nVersion=PROTOCOL
 213 |      return ss.GetHash();
 214 |  }
 215 |  
 216 | +/** Single-SHA256 a 32-byte input (represented as uint256). */
 217 | +uint256 SHA256Uint256(const uint256& input);

sipa commented at 7:36 PM on August 4, 2020:

Perhaps add NODISCARD here to guard against the issue you pointed out in the other PR.

practicalswift commented at 4:14 PM on August 5, 2020: contributor

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 NODISCARD to SHA256Uint256 would be nice as observed by sipa.
Add single sha256 call to CHashWriter b475d7d0fa
Add SHA256Uint256 helper functions 6510d0ff41
JeremyRubin force-pushed on Aug 6, 2020
DrahtBot removed the label Needs rebase on Aug 6, 2020
JeremyRubin force-pushed on Aug 6, 2020
JeremyRubin commented at 3:38 AM on August 7, 2020: contributor

rebased and added NODISCARD.
in src/script/interpreter.cpp:1263 in 8986838d3a outdated
```
1258 | @@ -1259,33 +1259,33 @@ class CTransactionSignatureSerializer
1259 |  };
1260 |  
1261 |  template <class T>
1262 | -uint256 GetPrevoutHash(const T& txTo)
1263 | +uint256 GetPrevoutsSHA256(const T& txTo)
```
jnewbery commented at 11:57 AM on August 7, 2020:

#17977 adds doxygen comments for this functions (https://github.com/bitcoin/bitcoin/pull/17977/files#diff-be2905e2f5218ecdbe4e55637dac75f3R1352). Mind doing the same here?

JeremyRubin commented at 6:09 PM on August 7, 2020:

done
jnewbery commented at 12:06 PM on August 7, 2020: member

utACK 8986838d3a4d52ce912e2ce385ec87a4a3d34cad.

One request for additional comments inline.
Refactor Get{Prevout,Sequence,Outputs}Hash to Get{Prevouts,Sequences,Outputs}SHA256.
Several proposals (Taproot, MuHash, CTV) require access to the single hash.
9ab4cafabd
JeremyRubin force-pushed on Aug 7, 2020
jnewbery commented at 7:23 PM on August 7, 2020: member

Code review ACK 9ab4cafabda227ccee5b241d7690fc67f9a27221
practicalswift commented at 4:08 AM on August 8, 2020: contributor

Nit: Could add unit tests for the newly introduced functions?
in src/hash.h:123 in 9ab4cafabd
```
 121 | +     * Invalidates this object.
 122 | +     */
 123 |      uint256 GetHash() {
 124 |          uint256 result;
 125 | -        ctx.Finalize(result);
 126 | +        ctx.Finalize(result.begin());
```
fjahr commented at 6:59 PM on August 8, 2020:

nit to consider if you retouch: this should work as well and might be a bit more concise (removing the first line in the function as well)

uint256 result = GetSHA256();

The whole function could even be written like this (but I think I slightly prefer the version above to this one):

uint256 sha256 = GetSHA256(); ctx.Reset().Write(sha256.begin(), CSHA256::OUTPUT_SIZE); return GetSHA256();
JeremyRubin commented at 12:12 AM on August 25, 2020:

resolving as a fixlater -- at some point, the SHA256 crypto interfaces are going to span-ified, and at that point this code will be touched up anyways :)
fjahr approved
fjahr commented at 7:29 PM on August 8, 2020: member

tested ACK 9ab4cafabda227ccee5b241d7690fc67f9a27221

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.
practicalswift commented at 6:35 AM on August 9, 2020: contributor

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 SHA256Uint256 is used in src/script/interpreter.cpp but not tested directly, or indirectly via a call from another tested function.

I don't think we should settle with "OK testing" for consensus critical code -- we want excellent testing :)
JeremyRubin commented at 5:08 PM on August 9, 2020: contributor

We should be hitting the PrecomputedTransactionData init all across the tests, and then validation would fail in numerous places if those have an error.
JeremyRubin commented at 5:10 PM on August 9, 2020: contributor

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).
sipa commented at 5:12 PM on August 9, 2020: member

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.
jonatack commented at 6:09 PM on August 20, 2020: member

Tested ACK 9ab4caf
jnewbery commented at 9:15 AM on August 24, 2020: member

@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
instagibbs commented at 1:13 PM on August 24, 2020: member

reACK https://github.com/bitcoin/bitcoin/pull/19601/commits/9ab4cafabda227ccee5b241d7690fc67f9a27221
fanquake merged this on Aug 25, 2020
fanquake closed this on Aug 25, 2020
sidhujag referenced this in commit 16d841beaa on Aug 25, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#17977 Implement BIP 340-342 validation (Schnorr/taproot/tapscript)#18071 Refactoring CHashWriter & Get{Prevouts,Sequence,Outputs}Hash to SHA256

Refactoring CHashWriter & Get{Prevouts,Sequence,Outputs}Hash to SHA256 (Alternative to #18071) #19601

Conflicts