💬 fanquake commented on pull request "prevector: simplify implementation of comparison operators and match behavior of `std::vector`":
(https://github.com/bitcoin/bitcoin/pull/33772#issuecomment-3481100244)
As indicated by the CI, the macOS SDK we are targeting doesn't (yet) support `lexicographical_compare_three_way`. A Guix build will fail the same way:
```bash
In file included from /distsrc-base/distsrc-21318e8b5df5-arm64-apple-darwin/src/chainparams.cpp:6:
In file included from /distsrc-base/distsrc-21318e8b5df5-arm64-apple-darwin/src/chainparams.h:9:
In file included from /distsrc-base/distsrc-21318e8b5df5-arm64-apple-darwin/src/kernel/chainparams.h:11:
In file included from /distsrc-base
...

💬 fanquake commented on pull request "depends: disable variables, rules and suffixes.":
(https://github.com/bitcoin/bitcoin/pull/33045#issuecomment-3481126835)
Pushed up the long form for both options.

💬 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>

...