💬 pinheadmz commented on pull request "zmq: Log bind error at Error level, abort startup on init error":
(https://github.com/bitcoin/bitcoin/pull/33727#issuecomment-3481214931)
Would be nice to get review from @promag or @laanwj because of the discussion in https://github.com/bitcoin/bitcoin/issues/17185 and https://github.com/bitcoin/bitcoin/pull/17445

💬 enirox001 commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2486986310)
Nice observation. This was intended for clarity when reading the code, but based on context, the use of the amounts can be inferred, making it redundant.

Simplified to use a list holding the amounts, removing the strings.

💬 enirox001 commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2486986380)
This assertion has been improved. It now checks that all stats dictionaries have the same set of keys, which is the intended test logic.

💬 enirox001 commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3481253934)
> With some context from the discussions, I think both of them should be updated to reflect what this PR is really doing.

I have updated the PR title and description based on the conversations made to better depict what the PR is doing @brunoerg .

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487011524)
yeah, but why is n skipped (see my suggested code)

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487015862)
how so, what's the failure you're seeing? It was working for me locally

💬 enirox001 commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3481276496)
Changes made in https://github.com/bitcoin/bitcoin/commit/b89afc6e4d6e71df4d99619480d705e1d0708ba7

- Changed the title and description of the PR to better depict the purpose of PR
- Simplified the transaction data structure by replacing tuples with a list of amounts
- Fixed a redundant assertion to properly validate key consistency across stats

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2487024773)
median is less sensitive to outliers - and nlogn is fine in tests

💬 purpleKarrot commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3481365862)
> Consensus code should arrive at the same conclusion, regardless of the architecture it runs on.

True.

> Using architecture-specific types such as size_t can lead to issues.

Can it?

`sizeof(size_t)` may have different values on different architectures, so depending on `sizeof(size_t)` is problematic.

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?

🤔 hebasto reviewed a pull request: "Add a menu action to restore then migrate a legacy wallet"
(https://github.com/bitcoin-core/gui/pull/877#pullrequestreview-3411832114)
Tested on Ubuntu 25.10 using a legacy wallet created in Bitcoin Core 25.2.

Please resolve a silent merge conflict with https://github.com/bitcoin/bitcoin/pull/33550.

👍 stickies-v approved a pull request: "refactor: C++20 operators"
(https://github.com/bitcoin/bitcoin/pull/33771#pullrequestreview-3411834976)
utACK 48840bfc2d7beeac0ddf56a3c26b243156ec8936. Pretty straightforward cleanup taking advantage of C++20 improvements, nice.

💬 stickies-v commented on pull request "refactor: C++20 operators":
(https://github.com/bitcoin/bitcoin/pull/33771#discussion_r2487080201)
nit (here and for all other new `operator<=>`): perhaps better to be explicit about the `std::strong_ordering` return type? And perhaps making it `constexpr` while touching?

<details>
<summary>git diff on 48840bfc2d</summary>

```diff
diff --git a/src/prevector.h b/src/prevector.h
index d4d90c7350..595be4a603 100644
--- a/src/prevector.h
+++ b/src/prevector.h
@@ -7,6 +7,7 @@

#include <algorithm>
#include <cassert>
+#include <compare>
#include <cstddef>
#include <cstdint>

...

👍 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.