rust-bitcoin / rust-bitcoin / 10528322931

83%

push

github

Merge rust-bitcoin/rust-bitcoin#3222: Siphash24 cleanup

6e5bd473a Improve siphash's `as_u64` -> `to_u64` rename (Martin Habovstiak)
1a9149220 Clean up the siphash mess (Martin Habovstiak)

Pull request description:

  Never break SSOT. Never break SSOT. Never break SSOT. Never break SSOT. Never break SSOT. Never break SSOT. Never break SSOT. ...

  Previously we had removed `Default` impl on `siphash24::HashEngine` by
  reimplementing the type manually. This was a really bad idea as it
  inevitably led to API differences that broke the build which we didn't
  notice because of unrelated bug. It should've just split the macro from
  the start as was suggested but it was claimed to be difficult, I don't
  think was the case as can be seen by this PR.

  This commit does what the previous one should've done: it renames the
  macro to have `_no_default` suffix, creates another one of the original
  name that calls into `_no_default` one and moves anything related to
  `Default`. This cleanly ensures all previous hashes stay the same while
  siphash gets `Default` removed. This also removes all now-conflicting
  impls from `siphash24` module which makes the module almost identical to
  what it looked like before the change. The only differences are removed
  `Default`/`new`, fixes in tests and recent rename of `as_u64` to
  `to_u64`.

  FTR both of our recent bugs were caused by breaking SSOT. It confirms again my life experience that breaking it will almost certainly bite one in the ass. If there is the most important principle in programming it's quite likely this one.

  I was trying to be more "friendly" towards contributors breaking it but I no longer feel that this is a good choice, especially for complicated PRs. Thus I plan to be more aggressive here and NACK pretty much all breaks of SSOT that aren't provably unsolvable.

ACKs for top commit:
  apoelstra:
    ACK 6e5bd473a67d5a3ba701dd... (continued)

5 of 14 new or added lines in 2 files covered. (35.71%)

19645 of 23723 relevant lines covered (82.81%)

15611.22 hits per line