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

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2206-hashwriter-👽 changing 14 files +65 −53
  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

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 09:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me