💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486784728)
Changed to `BOOST_CHECK_EQUAL`, added comments

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486787928)
Done

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3480999335)
Review comments addressed, mostly minor:

- Reduce the number of iterations to 10000 (from 30000), for faster test execution.
- Add comments on choice of test inputs.
- Use BOOST_REQUIRE_EQUAL or BOOST_CHECK_GT where possible.

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3481005723)
@sipa, as this is triggered by #33515 of yours, I would kindly ask for a review.

💬 willcl-ark commented on pull request "ci: Fix lint runner selection (and docker cache)":
(https://github.com/bitcoin/bitcoin/pull/33744#issuecomment-3481075031)
Sorry for the slow turnaround.

I pushed an additional commit to address (half of) @maflcko's nits. He is correct that GHA apparently does not validate inputs (even if you set `type: choice`), so it makes sense to remove the `options`. In order that we not miss if the wrong option is passed here in the future I added a sanity check which will annotate the CI run with a `Warning` when an unknown input is detected. See the bottom of the annotations of this run for what that looks like: https://g
...

💬 stickies-v commented on pull request "log: reduce excessive "rolling back/forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#issuecomment-3481078520)
Approach ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596, except for progress indication I don't see the need to unconditionally log every roll{back,forward} operation, so I strongly prefer logging it every n iterations over adding another non-ratelimited unconditional site.

💬 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