👍 ryanofsky approved a pull request: "precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442#pullrequestreview-3411441889)
Code review ACK 89bbbbd257063118e6968c409e52632835b76ce8. Make sense to replace the optimized `SipHashUint256` and `SipHashUint256Extra` functions with something more generic and avoid the need for them to repeat the same preprocessing when called multiple times. I left various suggestions below but they are not important. This already seems like a clear improvement in its current form.

💬 ryanofsky commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2486788376)
In commit "optimization: Introduce PresaltedSipHasher for repeated hashing" (8568340be2c679713703edc3d418272ac78d1147)

This code would feel safer if `v` vector was declared const, to provide some guarantee that `v[0]` will actually equal `C0 ^ k0` (and so on) at the time this is called.

EDIT: Implemented this const suggestion in diff below

💬 ryanofsky commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2486797297)
In commit "optimization: Introduce PresaltedSipHasher for repeated hashing" (8568340be2c679713703edc3d418272ac78d1147)

Can the description of this class from commit description be added here so this has some documentation?

Also it seems like distinction between the `CSipHasher` and `PresaltedSipHasher` classes is not really that one is presalted and the other isn't. They seem both seem presalted and have the same members and do the same work in the constructor.

Would seem more accurate
...

💬 ryanofsky commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2486893190)
In commit "optimization: Introduce PresaltedSipHasher for repeated hashing" (8568340be2c679713703edc3d418272ac78d1147)

Would be good to document that calling this is equivalent to calling `SipHasher(k0, k1).Write(val).Write(extra).Finalize()`

💬 ryanofsky commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2486998959)
In commit "optimization: Introduce PresaltedSipHasher for repeated hashing" (8568340be2c679713703edc3d418272ac78d1147)

I can see this code is just being moved, not duplicated again, but since this does duplicate the CSipHasher constructor code, would seem good to just move and define once in a single place.

Would suggest a change like the following (this change also makes PresaltedSipHasher state constant as suggested earlier):

```diff
--- a/src/crypto/siphash.cpp
+++ b/src/crypto/si
...

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487089474)
If `GetSkipHeight` is `inline`, I get the error:

```
[ 75%] Built target bitcoin-node
/usr/bin/ld: CMakeFiles/test_bitcoin.dir/chain_tests.cpp.o: in function `chain_tests::get_skip_height_test::test_method()':
/home/bitcoin/bitcoin-core/build/src/test/./test/chain_tests.cpp:50:(.text+0x68b): undefined reference to `GetSkipHeight(int)'
/usr/bin/ld: /home/bitcoin/bitcoin-core/build/src/test/./test/chain_tests.cpp:51:(.text+0x7cf): undefined reference to `GetSkipHeight(int)'
/usr/bin/ld: /h
...

💬 purpleKarrot commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3481391205)
nit: C++ defines types like uint64_t in the namespace std and then, optionally (!!!), also in the global namespace for compatibility with C. We should prefer std::uint64_t.

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487099898)
Oh, I see. There are in fact n+1 elements. Will change.

👍 ryanofsky approved a pull request: "ci: Update Clang in "tidy" job"
(https://github.com/bitcoin/bitcoin/pull/33445#pullrequestreview-3411868329)
Code review ACK 5d784bebaff5e3acc0b5180ee51d9a16aec0e356, just adding clang version comment since last review.

Would still be nice to remove ipc build code from the test directory as a followup too (https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2379651340)

💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3481418578)
> But if sizes are serialized as `uint32_t`, I assume only the value range of `uint32_t` is valid, even on 64bit machines. In what scenario can `size_t` lead to a problem?

It is explained in the CVE (linked in the description):

"Before writing a block to disk, Bitcoin Core checks that its size is within a normal range. This check would overflow on 32-bit systems for blocks over 1GB..."

The basic idea is that the following code is brittle:

```
u32 value = result of some calculation,
...

👍 hebasto approved a pull request: "Add createwallet, createwalletdescriptor, and migratewallet to history filter"
(https://github.com/bitcoin-core/gui/pull/901#pullrequestreview-3411906243)
ACK 4e352efa2ce756c668664486c99d003eef530e0c.

🚀 hebasto merged a pull request: "Add createwallet, createwalletdescriptor, and migratewallet to history filter"
(https://github.com/bitcoin-core/gui/pull/901)

🤔 hodlinator reviewed a pull request: "log: reduce excessive "rolling back/forward" messages during block replay"
(https://github.com/bitcoin/bitcoin/pull/33443#pullrequestreview-3411788483)
Concept ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596

💬 hodlinator commented on pull request "log: reduce excessive "rolling back/forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2487048889)
In the commit message + PR description, what purpose does the `[*] `-prefix from the Before-example serve?

nit: Should uppercase "ibd" in PR desc.

💬 hebasto commented on pull request "[30.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33609#issuecomment-3481469144)
Add https://github.com/bitcoin-core/gui/pull/901?

💬 purpleKarrot commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3481472896)
@maflcko, I get that checking for overflow after the cause is not going to work.
But if we have a buggy calculation that may overflow u32, changing the type to u64 is not a proper fix. What if it overflows u64? How do you want to detect that?

Would you agree that, if the calculation is fixed to detect overflow before the cause then using `size_t` would not be a problem at all?

💬 hebasto commented on pull request "Modernize custom filtering":
(https://github.com/bitcoin-core/gui/pull/899#discussion_r2487168803)
Thanks! Fixed.

💬 stickies-v commented on pull request "log: reduce excessive "rolling back/forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2487174964)
http://github.com/bitcoin/bitcoin/pull/32604/commits/d541409a64c60d127ff912dad9dea949d45dbd8c#diff-44fd50b51e8fc6799d38f193237fb921ec9d34306c448f64837524a17bac06eeR471-R472 introduced prefixing all messages with `[*]` to indicate that ratelimiting is currently being applied.

💬 hebasto commented on pull request "Remove HD seed reference from blank wallet tooltip":
(https://github.com/bitcoin-core/gui/pull/908#issuecomment-3481506281)
cc @achow101

💬 0xB10C commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2487213289)
I plan have a closer look.

I don't use the utxocache tracepoints at the moment.