💬 hebasto commented on pull request "Replace non-standard CLZ builtins with c++20's bit_width":
(https://github.com/bitcoin/bitcoin/pull/29057#issuecomment-1851535346)
Concept ACK.

💬 vasild commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1423659099)
> This has nothing to do with C++20

ok, rephrase (I mentioned C++20 because C++17 does not have `char8_t`):

What would be an example where it is broken by calling `string()` (1.) and ok by converting `wchar_t`->`char8_t`->`char` (2.2)?

💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1851561925)
Looking at the diff between the two versions, I see these changes:
1. which peers are taken into account (`FEELER`, `ADDRFETCH`, `BLOCKRELAY` are also considered)
2. the 199-bug is brought back
3. fixed addition overflow; an alternative would divide each element by 2 and then add without overflow risk, it saves us the sloppiness `int64_max/2`; but i don't care that much)

Not sure i see `keeps warning condition for a large median time offset as it is on master`.

Generally, while I think
...

💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1851608976)
ACK https://github.com/bitcoin/bitcoin/commit/576bee88fd36e207b7288077626947a1fce0fc33

💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1851611159)
> ACK [18669d7](https://github.com/bitcoin/bitcoin/commit/18669d753a15f997f7363dcaf0abf230b851b224)

guessing you meant to ack https://github.com/bitcoin/bitcoin/commit/576bee88fd36e207b7288077626947a1fce0fc33 ? :wink:

💬 hebasto commented on pull request "Replace non-standard CLZ builtins with c++20's bit_width":
(https://github.com/bitcoin/bitcoin/pull/29057#issuecomment-1851613999)
> ... but I wanted to allow our tests/fuzzers to run with this change first to help convince reviewers that `std::bit_width` is a drop-in replacement for`CountBits`.

I'm convinced :)

💬 naumenkogs commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1851617327)
Concept ACK

👍 maflcko approved a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1777064592)
It would be good to explain why the behavior changes are needed. Otherwise,

concept ACK eaf915d61d470372e63f41f11d6a873c1936f73f 👇

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+
...

💬 maflcko commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423755089)
nit in eaf915d61d470372e63f41f11d6a873c1936f73f: Can drop the `()` around `Assert()`?

💬 maflcko commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423738990)
same

💬 maflcko commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423753937)
nit in eaf915d61d470372e63f41f11d6a873c1936f73f: "*the* main thread"

💬 maflcko commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1423738157)
ae6642973dead188514aceec30ae5d73404ebcdf: Is it required to change the behavior here? Why not keep the abort?

📝 hebasto opened a pull request: "Revert "ci: Only run functional tests on windows in master""
(https://github.com/bitcoin/bitcoin/pull/29059)
This PR reverts commit aba4a5887b44bf7cbee9ea0a8e02bb92c1b4147b from https://github.com/bitcoin/bitcoin/pull/28567.

The Windows-specific code received [quality](https://github.com/bitcoin/bitcoin/pull/28486) and [performance](https://github.com/bitcoin/bitcoin/pull/29045) improvements recently. So there are no reasons to skip functional tests in PRs anymore.

In my own repo, I've run the GHA Windows job more than 100 times with no failure.

💬 maflcko commented on pull request "Revert "ci: Only run functional tests on windows in master"":
(https://github.com/bitcoin/bitcoin/pull/29059#discussion_r1423789241)
How much longer would it take to also run `--extended`?

💬 hebasto commented on pull request "Revert "ci: Only run functional tests on windows in master"":
(https://github.com/bitcoin/bitcoin/pull/29059#discussion_r1423803255)
> How much longer would it take to also run `--extended`?

Appr. 10 minutes.

The `feature_dbcrash.py` test takes the longest time.

🚀 fanquake merged a pull request: "refactor: rpc: Pass CBlockIndex by reference instead of pointer"
(https://github.com/bitcoin/bitcoin/pull/29021)

💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1851803284)
Removing adjusted time from consensus is a big win. Having time samples received from peers be involved in consensus checks is not great and not effective against NTP based attacks (https://github.com/bitcoin/bitcoin/pull/25908#issuecomment-1264632475, https://github.com/bitcoin/bitcoin/pull/25908#issuecomment-1271239626). The warning itself is not as interesting to me but most of the review on both #25908 and this PR is about the warning and its threshold.

I don't want changing the warning (
...

👍 brunoerg approved a pull request: "doc/reduce-traffic: update/clarify max outbound connection count"
(https://github.com/bitcoin/bitcoin/pull/29052#pullrequestreview-1777200873)
ACK d58f89d355faf46b68d0ff8095699d0aff41959c

💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1851813782)
> an alternative would divide each element by 2 and then add without overflow risk

Done.

💬 maflcko commented on pull request "Revert "ci: Only run functional tests on windows in master"":
(https://github.com/bitcoin/bitcoin/pull/29059#issuecomment-1851817205)
lgtm ACK 7b22cd80e050b903b5765133b8116f4b17ce0296