💬 fanquake commented on pull request "depends: fix libevent `_WIN32_WINNT` usage":
(https://github.com/bitcoin/bitcoin/pull/32837#issuecomment-3061722462)
Guix Build (aarch64):
```bash
54f1ec86a1e595a5529f115a333c5f6cf575ff35f0b87ed501f36f83e28d9063 guix-build-f5647c6c5ae8/output/aarch64-linux-gnu/SHA256SUMS.part
e9db7e23661bac03b87e38dfc974d27b72ce8d684d9cff7e7105c735b8bab7af guix-build-f5647c6c5ae8/output/aarch64-linux-gnu/bitcoin-f5647c6c5ae8-aarch64-linux-gnu-debug.tar.gz
55c5b8685af798bbe36db8d1bc6a4fa90a10f83c68c8793d08512af4d7a9e326 guix-build-f5647c6c5ae8/output/aarch64-linux-gnu/bitcoin-f5647c6c5ae8-aarch64-linux-gnu.tar.gz
c67cf3
...

💬 hebasto commented on pull request "depends: Force `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE`":
(https://github.com/bitcoin/bitcoin/pull/32943#issuecomment-3061725053)
> Could we also set `CMAKE_FIND_USE_PACKAGE_REGISTRY` to false in the toolchain file as a belt-and-suspenders in case some future package manually opts-in using `CMAKE_EXPORT_PACKAGE_REGISTRY` ?

I don't think it prevents issues like https://github.com/bitcoin/bitcoin/issues/32938, which occur when the toolchain file is not used.

💬 hebasto commented on pull request "depends: Force `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE`":
(https://github.com/bitcoin/bitcoin/pull/32943#issuecomment-3061753523)
> This is going to need backporting, to fix the 29.x branch.

I think it should be also backported to the 28.x branch, considering https://github.com/bitcoin/bitcoin/commit/f59e9057e2aa596b54cf9e85bab35c3ead137547.

Additionally, we might want to advise users to delete the `~/.cmake/packages/libevent` directory on their machines.

💬 hebasto commented on pull request "New SVG, Icons, PNGs and X PixMaps":
(https://github.com/bitcoin-core/gui/pull/879#issuecomment-3061802573)
> > Why?
>
> Padding (not margin) has been removed because it is unnecessary and visually disruptive. Padding serves no purpose and only worsens the layout.

Not being a designer, these claims seem like a matter of taste to me. It would be helpful to hear other designers' opinions.

> > Bitcoin Core has its own logo for years. I don't see your reference as a justification for this change.
>
> I discussed this PR with @jonasschnelli on IRC a few days ago. As the creator of the original
...

💬 hebasto commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2200427146)
Since https://codeberg.org/guix/guix/pulls/353 uses the newer `pyproject-build-system` and `python-scikit-build-core`, it no longer requires an extra build step to set the `PYTHONPATH` environment variable.

⚠️ cyjseagull opened an issue: "Why bitcoin use so many RecursiveMutex"
(https://github.com/bitcoin/bitcoin/issues/32946)
I have noticed that in the Bitcoin source code, almost all thread-safe sections use `RecursiveGuard`.
As we all know, `RecursiveGuard` is **exclusive**, which can **impact system performance and concurrency**.

At the same time, I have also noticed [another issue](https://github.com/bitcoin/bitcoin/issues/19303) specifically tracking the replacement of `RecursiveMutex` with `Mutex`, which would help reduce some of the locking overhead.

However, I think `Mutex` is still **exclusive**. For modul
...

✅ cyjseagull closed an issue: "Why bitcoin use so many RecursiveMutex"
(https://github.com/bitcoin/bitcoin/issues/32934)

🤔 marcofleon reviewed a pull request: "fuzz: Add missing calls to `SetMockTime` for determinism"
(https://github.com/bitcoin/bitcoin/pull/32927#pullrequestreview-3009814486)
post merge ACK fa8862723c14cdd471bbffc7a4897ece2e6d8a7f

✅ fanquake closed an issue: "Why bitcoin use so many RecursiveMutex"
(https://github.com/bitcoin/bitcoin/issues/32946)

⚠️ cyjseagull opened an issue: "Why bitcoin use so many RecursiveMutex"
(https://github.com/bitcoin/bitcoin/issues/32947)
### Please describe the feature you'd like to see added.

I have noticed that in the Bitcoin source code, almost all thread-safe sections use RecursiveGuard.
As we all know, RecursiveGuard is exclusive, which can impact system performance and concurrency.

At the same time, I have also noticed https://github.com/bitcoin/bitcoin/issues/19303 specifically tracking the replacement of RecursiveMutex with Mutex, which would help reduce some of the locking overhead.

However, I think Mutex is still ex
...

💬 jonasschnelli commented on pull request "New SVG, Icons, PNGs and X PixMaps":
(https://github.com/bitcoin-core/gui/pull/879#issuecomment-3061871060)
I briefly discussed the issue of copyright and authorship on the SVG with the PR author in #bitcoin (IRC).

It sounded to me that the PR author was worried about the copyright and MIT licensing as well as who was the original designer. Unclear what the intention of this PR is as this also changes the copyright in the file.

I have no opinion on visual style.

💬 danielabrozzoni commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3061907541)
Hey @jonatack, I noticed there are still a couple of comments open. Let me know if you'd like any help addressing them :)

🤔 hebasto reviewed a pull request: "depends: fix libevent `_WIN32_WINNT` usage"
(https://github.com/bitcoin/bitcoin/pull/32837#pullrequestreview-3009917656)
My Guix build:
```
aarch64
54f1ec86a1e595a5529f115a333c5f6cf575ff35f0b87ed501f36f83e28d9063 guix-build-f5647c6c5ae8/output/aarch64-linux-gnu/SHA256SUMS.part
e9db7e23661bac03b87e38dfc974d27b72ce8d684d9cff7e7105c735b8bab7af guix-build-f5647c6c5ae8/output/aarch64-linux-gnu/bitcoin-f5647c6c5ae8-aarch64-linux-gnu-debug.tar.gz
55c5b8685af798bbe36db8d1bc6a4fa90a10f83c68c8793d08512af4d7a9e326 guix-build-f5647c6c5ae8/output/aarch64-linux-gnu/bitcoin-f5647c6c5ae8-aarch64-linux-gnu.tar.gz
c67cf3eb
...

👍 maflcko approved a pull request: "refactor: Convert GenTxid to `std::variant`"
(https://github.com/bitcoin/bitcoin/pull/32631#pullrequestreview-3009084888)
left two nits that can be ignored or followed-up later on.

review ACK a60f863d3e276534444571282f432b913d3967db 🎽

<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+U4AP1RJMi+krxU1A3Yux4bpwZNL
...

💬 maflcko commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2200149905)
nit in 11d28f21bb8f0c3094934b3fef45871f73bb216a (first commit): Could add a comment to this class, documenting that it will be removed in the last commit?

💬 maflcko commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2199951996)
nit 11d28f21bb8f0c3094934b3fef45871f73bb216a: This creates a copy, when read-only access is needed. (clang -O2 seems to strip the copy, but gcc may not). You can avoid it via:

```cpp
return std::tuple<bool,const uint256&>(a.IsWtxid(), a.ToUint256()) <=> std::tuple<bool,const uint256&>(b.IsWtxid(), b.ToUint256());
```

👍 hodlinator approved a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-3010008670)
re-ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f

Only able to spot improvements since previous ACK (https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2980338061). :)
Re-tested `cmake --install` + running `bitcoin` wrapper and `test_bitcoin`.

Agree with Sjors that codesigning should probably be re-tested since it's been modified (https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3047748032).

💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3062068009)
@TheCharlatan better?

💬 pablomartin4btc commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2200579176)
Do we still need `SetMinVersion()` (`GetVersion()`, `GetClosestWalletFeature()`, `IsFeatureSupported()`, `enum WalletFeature`) at all?

💬 TheCharlatan commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3062135707)
I'm still not fully convinced of the new layout, but also would not unequivocally block this. Maybe some more long term contributors should give their approval here though, since it does change how our binaries are shipped and where we are placing things going forward.

Re Russ' previous comment:
> based on what a good command line interface would look like, not based on idiosyncracies of previous releases.

There is a bunch of nuance here, since this also applies to pretty much all the bin
...

📝 Sjors opened a pull request: "refactor: cleanup index logging"
(https://github.com/bitcoin/bitcoin/pull/32948)
This PR removes the use of `__func__` from index logging, since we have `-logsourcelocations`.

It also improves readability by putting `GetName()` in a more logical place.

Before

> coinstatsindex: best block of the index not found. Please rebuild the index.

After:

> best block of coinstatsindex not found. Please rebuild the index.

I found myself maintaining this commit as part of https://github.com/Sjors/bitcoin/pull/86, but since that might never land here, it seemed better to
...