💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#issuecomment-3481130144)
@vasild @ryanofsky

Kindly asking you to refresh your ACKs after the comment [update](https://github.com/bitcoin/bitcoin/pull/33445#issuecomment-3457125965).

👍 hebasto approved a pull request: "depends: disable variables, rules and suffixes."
(https://github.com/bitcoin/bitcoin/pull/33045#pullrequestreview-3411598078)
re-ACK 52b1595850f63b65701a405d31045faa59231c75.

✅ hebasto closed a pull request: "Add a menu action to restore then migrate a legacy wallet"
(https://github.com/bitcoin-core/gui/pull/877)

📝 hebasto reopened a pull request: "Add a menu action to restore then migrate a legacy wallet"
(https://github.com/bitcoin-core/gui/pull/877)
Some users will have a backup of their legacy wallet. These cannot be restored since the "Restore Wallet" action expects to be able to load the wallet after restoring, and this fails for legacy wallets now that they are deleted. Furthermore, the "Migrate Wallet" action only allows users to migrate wallets that are in the wallets directory, so such backups cannot be migrated from the GUI.

This PR resolves this issue by adding a menu item in the "Migrate Wallet" menu which allows users to selec
...

🤔 pinheadmz reviewed a pull request: "zmq: Log bind error at Error level, abort startup on init error"
(https://github.com/bitcoin/bitcoin/pull/33727#pullrequestreview-3411664324)
concept ACK

💬 pinheadmz commented on pull request "zmq: Log bind error at Error level, abort startup on init error":
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2486961157)
Would be nice to assert the `expected_msg` here. Also, to more explicitly cover the issue in #33715 I'd expect to see a test where zmq is set to a valid tcp address but one that is already in use (you should be able to attempt a bind to node's p2p or rpc port)

💬 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