Add HashWriter without ser-type and ser-version and use it where possible #25331

1. 
   maflcko commented at 10:03 am on June 10, 2022: member

   This was done in the context of #25284 , but I think it also makes sense standalone.

   The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.

   So do this here for HashWriter. CHashWriter remains in places where it is not yet possible.

2. DrahtBot added the label Refactoring on Jun 10, 2022
3. 
   DrahtBot commented at 5:45 pm on June 10, 2022: contributor

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #24058 (BIP-322 basic support by kallewoof)
   • #13062 (Make script interpreter independent from storage type CScript 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.

4. 
   meetsussie commented at 2:30 pm on June 11, 2022: none

   .
5. bitcoin deleted a comment on Jun 14, 2022
6. 
   laanwj commented at 8:43 pm on June 17, 2022: member

   Concept ACK
7. in src/chainparams.cpp:355 in fa9cc54c50 outdated

   351@@ -352,7 +352,7 @@ class SigNetParams : public CChainParams {
   352         consensus.vDeployments[Consensus::DEPLOYMENT_TAPROOT].min_activation_height = 0; // No activation delay
   353 
   354         // message start is defined as the first 4 bytes of the sha256d of the block script
   355-        CHashWriter h(SER_DISK, 0);
   

   ---

   ---

   

   Empact commented at 9:58 pm on June 17, 2022:

   nit: what’s the benefit of writing this HashWriter h{}; instead of HashWriter h;? Both default-initialize, right?

   ---

   

   maflcko commented at 6:27 am on June 18, 2022:

   The behaviour is the same with or without {}. However, the (style) benefit of always using {} is that it can be used consistently in code. For example, you couldn’t write Function(HashWriter) to construct a temporary.

   (Also it makes the diff smaller if someone were to add an argument later)

   Though, it’s really just a style preference.

8. in src/pubkey.cpp:214 in fa9cc54c50 outdated

   210@@ -211,16 +211,16 @@ bool XOnlyPubKey::VerifySchnorr(const uint256& msg, Span<const unsigned char> si
   211     return secp256k1_schnorrsig_verify(secp256k1_context_verify, sigbytes.data(), msg.begin(), 32, &pubkey);
   212 }
   213 
   214-static const CHashWriter HASHER_TAPTWEAK = TaggedHash("TapTweak");
   215+static const HashWriter HASHER_TAPTWEAK = TaggedHash("TapTweak");
   

   ---

   ---

   

   Empact commented at 10:00 pm on June 17, 2022:

   nit: curly bracket initialization here too?

   ---

   

   maflcko commented at 11:06 am on June 18, 2022:

   Thx, done.
9. maflcko force-pushed on Jun 18, 2022
10. Add HashWriter without ser-type and ser-version

    The moved parts can be reviewed with "--color-moved=dimmed-zebra".

    faa5425629
11. Use HashWriter where possible faf9accd66
12. maflcko force-pushed on Jul 20, 2022
13. 
    sipa commented at 4:48 pm on July 20, 2022: member

    utACK faf9accd662974a69390213fee1b5c6237846b42
14. maflcko requested review from Empact on Jul 20, 2022
15. maflcko requested review from ryanofsky on Jul 20, 2022
16. 
    Empact commented at 7:17 am on July 22, 2022: member

    utACK https://github.com/bitcoin/bitcoin/pull/25331/commits/faf9accd662974a69390213fee1b5c6237846b42

    nit: I still prefer default-initialization without {} - the brackets strike me as noise if unnecessary. #25331 (review)

17. fanquake merged this on Jul 22, 2022
18. fanquake closed this on Jul 22, 2022

    ---

19. maflcko deleted the branch on Jul 22, 2022
20. sidhujag referenced this in commit ff4a86be7d on Jul 22, 2022
21. bitcoin locked this on Jul 2, 2024

Contributors
maflcko DrahtBot meetsussie laanwj Empact sipa

Review Requested
Empact ryanofsky

Labels
Refactoring