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

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 0: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)

0        uint256 result = GetSHA256();

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

0        uint256 sha256 = GetSHA256();
1        ctx.Reset().Write(sha256.begin(), CSHA256::OUTPUT_SIZE);
2        return GetSHA256();

JeremyRubin commented at 0: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

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

Conflicts