🚀 fanquake merged a pull request: "fuzz: Add missing calls to `SetMockTime` for determinism"
(https://github.com/bitcoin/bitcoin/pull/32927)
(https://github.com/bitcoin/bitcoin/pull/32927)
💬 hebasto commented on pull request "cmake: Use newer signature of `qt6_add_lrelease` when available":
(https://github.com/bitcoin/bitcoin/pull/32940#issuecomment-3061672234)
My Guix build:
```
aarch64
9a9cd30b66d0c150c6e843a37b89cdbae284e0e7c2829d7d6c64076453d4be1f guix-build-94931656b52f/output/aarch64-linux-gnu/SHA256SUMS.part
e7525f1c1cf720f53a292b404ccd63ca792631c14587a1a34978b33d35c4fece guix-build-94931656b52f/output/aarch64-linux-gnu/bitcoin-94931656b52f-aarch64-linux-gnu-debug.tar.gz
231c4e16929291d5d6edf1d3bf0c0ba2f9fda5a11586941a92f256f6bd1067c7 guix-build-94931656b52f/output/aarch64-linux-gnu/bitcoin-94931656b52f-aarch64-linux-gnu.tar.gz
f965bfc1
...
(https://github.com/bitcoin/bitcoin/pull/32940#issuecomment-3061672234)
My Guix build:
```
aarch64
9a9cd30b66d0c150c6e843a37b89cdbae284e0e7c2829d7d6c64076453d4be1f guix-build-94931656b52f/output/aarch64-linux-gnu/SHA256SUMS.part
e7525f1c1cf720f53a292b404ccd63ca792631c14587a1a34978b33d35c4fece guix-build-94931656b52f/output/aarch64-linux-gnu/bitcoin-94931656b52f-aarch64-linux-gnu-debug.tar.gz
231c4e16929291d5d6edf1d3bf0c0ba2f9fda5a11586941a92f256f6bd1067c7 guix-build-94931656b52f/output/aarch64-linux-gnu/bitcoin-94931656b52f-aarch64-linux-gnu.tar.gz
f965bfc1
...
👍 dergoegge approved a pull request: "Fuzz: extend CConnman tests"
(https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-3009670249)
Code review ACK 0802398e749c5e16fa7085cd87c91a31bbe043bd
(https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-3009670249)
Code review ACK 0802398e749c5e16fa7085cd87c91a31bbe043bd
💬 dergoegge commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r2200334391)
nit: it might make sense to name this differently, just to avoid someone confusing this function with a smart ptr's `reset()`.
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r2200334391)
nit: it might make sense to name this differently, just to avoid someone confusing this function with a smart ptr's `reset()`.
💬 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
...
(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.
(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.
(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
...
(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.
(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
...
(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)
(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
(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)
(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
...
(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.
(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 :)
(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
...
(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
...
(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?
(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());
```
(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());
```