hash: remove redundant `secp256k1_sha256_initialize` in tagged hash midstate functions #1825

pull w0xlt wants to merge 1 commits into bitcoin-core:master from w0xlt:fix-redundant-sha256-initialize changing 7 files +80 −118

w0xlt commented at 7:38 PM on February 17, 2026: contributor

Each tagged hash midstate function (e.g., secp256k1_schnorrsig_sha256_tagged) calls secp256k1_sha256_initialize before immediately overwriting every field it sets: s[0] through s[7] and bytes. The buf[64] member does not need initialization either, because bytes is set to 64, which means the buffer position (bytes & 0x3F) (= bytes % 64) is 0, so buf is always written before being read.

Remove the 11 redundant secp256k1_sha256_initialize calls across the schnorrsig, ellswift, and musig modules.
real-or-random commented at 9:26 AM on February 18, 2026: contributor

This has been brought up before, see #1179. Let's maybe keep the discussion there (unless it's really specific to this PR).
real-or-random added the label tweak/refactor on Feb 18, 2026
w0xlt force-pushed on Feb 19, 2026
real-or-random commented at 2:01 PM on February 19, 2026: contributor

Concept ACK
theStack commented at 6:33 PM on February 23, 2026: contributor

Concept ACK

in src/hash_impl.h:45 in 10ecf81eec

  39 | @@ -40,6 +40,16 @@ static void secp256k1_sha256_initialize(secp256k1_sha256 *hash) {
  40 |      hash->bytes = 0;
  41 |  }
  42 |  
  43 | +/* Initialize a SHA256 hash state with a precomputed midstate.
  44 | + * The byte counter must be a multiple of 64, i.e., there must be no unwritten
  45 | + * bytes in the buffer. */

real-or-random commented at 9:35 AM on February 25, 2026:

nit: I think it's sufficient (and more consistent with the rest of the code base) to have the comment only in hash.h.

w0xlt commented at 12:31 AM on February 26, 2026:

Done in f48b1bfa5d40a4d7303b196017d2e298520d1066. Thanks.

in src/tests.c:770 in 10ecf81eec
```
 765 | +        secp256k1_sha256_write(&sha_tagged, msg + split, msg_len - split);
 766 | +        secp256k1_sha256_write(&sha_midstate, msg, msg_len);
 767 | +
 768 | +        secp256k1_sha256_finalize(&sha_tagged, hash_tagged);
 769 | +        secp256k1_sha256_finalize(&sha_midstate, hash_midstate);
 770 | +        CHECK(secp256k1_memcmp_var(hash_tagged, hash_midstate, sizeof(hash_tagged)) == 0);
```
real-or-random commented at 9:45 AM on February 25, 2026:

I think these are not necessary given that you check test_sha256_eq. Of course, no test is "necessary" in a strict sense, but did you have any specific reason in mind why to check this?

If you omit them, you could use test_sha256_tag_midstate.

w0xlt commented at 12:31 AM on February 26, 2026:

Done in f48b1bfa5d40a4d7303b196017d2e298520d1066. Thanks.
in src/tests.c:761 in 10ecf81eec
```
 756 | +        unsigned char hash_midstate[32];
 757 | +        size_t msg_len = msg_lens[i];
 758 | +        size_t split = msg_len / 2;
 759 | +
 760 | +        secp256k1_sha256_initialize_tagged(&sha_tagged, tag, sizeof(tag) - 1);
 761 | +        secp256k1_sha256_initialize_midstate(&sha_midstate, sha_tagged.bytes, sha_tagged.s);
```
real-or-random commented at 9:47 AM on February 25, 2026:

Having the midstate here as a literal would make this test work without accessing the (now) fully encapsulated members of the secp256k1_sha256 struct.

You could use https://gist.github.com/real-or-random/a4aaae5d04eee9f63e7a2e43d25bc2b1 or simply reuse an existing midstate from some module test.

w0xlt commented at 12:31 AM on February 26, 2026:

Done in f48b1bfa5d40a4d7303b196017d2e298520d1066. Thanks.
real-or-random commented at 9:51 AM on February 25, 2026: contributor

ACK mod nits

hash: add midstate initializer and use it for tagged hashes

Introduce secp256k1_sha256_initialize_midstate() in the hash layer and use it at all tagged-hash midstate call sites across schnorrsig, musig, and ellswift.

Document the byte-counter contract at the declaration site in hash.h and add run_sha256_initialize_midstate_tests() to directly verify helper behavior against initialize_tagged.

Also switch the helper to take const uint32_t state[8] to reduce argument-order risk at call sites.

f48b1bfa5d

w0xlt force-pushed on Feb 25, 2026
real-or-random approved
real-or-random commented at 8:39 AM on February 27, 2026: contributor

utACK f48b1bfa5d40a4d7303b196017d2e298520d1066
theStack approved
theStack commented at 5:52 PM on February 27, 2026: contributor

Code-review ACK f48b1bfa5d40a4d7303b196017d2e298520d1066
real-or-random merged this on Feb 27, 2026
real-or-random closed this on Feb 27, 2026
w0xlt deleted the branch on Mar 2, 2026

Contributors

Labels