💬 fanquake commented on pull request "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it":
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3563315913)
> In my view, dropping support for x86_64-w64-mingw32 should be considered separately and at a later stage, once the codebase becomes incompatible with MSVCRT or the toolchain landscape has matured.
Shouldn't it be removed once we remove it from the CI? which, according to the PR description in #33764, will be done once the Guix migration is done:
> MSVCRT-related CI jobs should be removed from the CI framework once the migration to UCRT is complete.
Just trying to get a clearer idea of w
...
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3563315913)
> In my view, dropping support for x86_64-w64-mingw32 should be considered separately and at a later stage, once the codebase becomes incompatible with MSVCRT or the toolchain landscape has matured.
Shouldn't it be removed once we remove it from the CI? which, according to the PR description in #33764, will be done once the Guix migration is done:
> MSVCRT-related CI jobs should be removed from the CI framework once the migration to UCRT is complete.
Just trying to get a clearer idea of w
...
🤔 l0rinc requested changes to a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3493026362)
Concept ACK, but if we claim best-effort, we should cover the M series as well with latest AppleClang and latest LLVM.
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3493026362)
Concept ACK, but if we claim best-effort, we should cover the M series as well with latest AppleClang and latest LLVM.
💬 l0rinc commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2549969970)
is this removal related to the mac cleanup?
it seems this was before the Mac section originally https://github.com/bitcoin/bitcoin/commit/33dd764984def9371f324d3add19ee894a0260bf#diff-7ff2e1d1ed59fa96766a663236d71bfdd00b4af3321fde39f7dbf022bd968a0aL87-L88
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2549969970)
is this removal related to the mac cleanup?
it seems this was before the Mac section originally https://github.com/bitcoin/bitcoin/commit/33dd764984def9371f324d3add19ee894a0260bf#diff-7ff2e1d1ed59fa96766a663236d71bfdd00b4af3321fde39f7dbf022bd968a0aL87-L88
💬 l0rinc commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2549966345)
can we specify in the beginning that we recommend linux and still provide the link to the mac section?
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2549966345)
can we specify in the beginning that we recommend linux and still provide the link to the mac section?
💬 l0rinc commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2549976233)
since this is an alternative to the above linux one and since the build directory is changed now, should we add
```md
cmake --build build_fuzz_nosan -j$(nproc) && \
FUZZ=coins_view_db build_fuzz_nosan/bin/fuzz
```
after this?
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2549976233)
since this is an alternative to the above linux one and since the build directory is changed now, should we add
```md
cmake --build build_fuzz_nosan -j$(nproc) && \
FUZZ=coins_view_db build_fuzz_nosan/bin/fuzz
```
after this?
💬 l0rinc commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2549997346)
As described in https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-3361478681 this doesn't seem to be enough for me, I'm getting a lot of failures locally on my non-intel-based Mac.
It needs a few more parameters to avoid being tangled with local `AppleClang`:
```
rm -rfd build_fuzz_nosan && cmake --preset=libfuzzer-nosan \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_OSX_SYSROOT="$(xcrun --show-sdk-pat
...
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2549997346)
As described in https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-3361478681 this doesn't seem to be enough for me, I'm getting a lot of failures locally on my non-intel-based Mac.
It needs a few more parameters to avoid being tangled with local `AppleClang`:
```
rm -rfd build_fuzz_nosan && cmake --preset=libfuzzer-nosan \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_OSX_SYSROOT="$(xcrun --show-sdk-pat
...
🤔 hodlinator reviewed a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3492740506)
Concept ACK e14650967dc95da5c10e0d6183b6eac3e8243fe5
#32541 contained some useful tricks that seem to be useful to shrink other index implementations. Thanks for letting it go in favor of this less invasive change though!
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3492740506)
Concept ACK e14650967dc95da5c10e0d6183b6eac3e8243fe5
#32541 contained some useful tricks that seem to be useful to shrink other index implementations. Thanks for letting it go in favor of this less invasive change though!
💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2549751593)
No need to have `std::optional` for the offset, could just be sent in as `0` when reading whole blocks. Same for parameter types in rest.cpp.
```suggestion
bool BlockManager::ReadRawBlockPart(std::vector<std::byte>& data, const FlatFilePos& pos, size_t part_offset, std::optional<size_t> part_size) const
```
<details><summary>Full compiling diff (includes suggestion to skip noop seeks, see other comment).</summary>
```diff
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index
...
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2549751593)
No need to have `std::optional` for the offset, could just be sent in as `0` when reading whole blocks. Same for parameter types in rest.cpp.
```suggestion
bool BlockManager::ReadRawBlockPart(std::vector<std::byte>& data, const FlatFilePos& pos, size_t part_offset, std::optional<size_t> part_size) const
```
<details><summary>Full compiling diff (includes suggestion to skip noop seeks, see other comment).</summary>
```diff
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index
...
💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2549741549)
nit: Could avoid syscall in common case
```suggestion
if (offset > 0) filein.seek(offset, SEEK_CUR);
```
Other PR avoiding similar syscall which was leading to slowdowns on Windows: #30884
Alternatively one could perform the noop-check inside of `AutoFile` itself.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2549741549)
nit: Could avoid syscall in common case
```suggestion
if (offset > 0) filein.seek(offset, SEEK_CUR);
```
Other PR avoiding similar syscall which was leading to slowdowns on Windows: #30884
Alternatively one could perform the noop-check inside of `AutoFile` itself.
💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2549772096)
nit: Could call `block_part.clear()` between each read.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2549772096)
nit: Could call `block_part.clear()` between each read.
💬 hebasto commented on pull request "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it":
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3563408869)
> > In my view, dropping support for x86_64-w64-mingw32 should be considered separately and at a later stage, once the codebase becomes incompatible with MSVCRT or the toolchain landscape has matured.
>
> Shouldn't it be removed once we remove it from the CI? which, according to the PR description in #33764, will be done once the Guix migration is done:
Agreed.
> > MSVCRT-related CI jobs should be removed from the CI framework once the migration to UCRT is complete.
>
> Just trying t
...
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3563408869)
> > In my view, dropping support for x86_64-w64-mingw32 should be considered separately and at a later stage, once the codebase becomes incompatible with MSVCRT or the toolchain landscape has matured.
>
> Shouldn't it be removed once we remove it from the CI? which, according to the PR description in #33764, will be done once the Guix migration is done:
Agreed.
> > MSVCRT-related CI jobs should be removed from the CI framework once the migration to UCRT is complete.
>
> Just trying t
...
💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2550080275)
> Yeah, I guess there were some silent conflicts.
Indeed. Fixed.
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2550080275)
> Yeah, I guess there were some silent conflicts.
Indeed. Fixed.
💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3563434095)
> [#33764 (comment)](https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2549961718)
Thanks! Fixed.
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3563434095)
> [#33764 (comment)](https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2549961718)
Thanks! Fixed.
💬 diegoviola commented on pull request "Fix bitcoin-qt visual glitches on Wayland":
(https://github.com/bitcoin-core/gui/pull/904#issuecomment-3563438977)
> For example, on Fedora 43 with Gnome 49 and Wayland
Ah, I haven't tested this on GNOME. I suspect the use of `showNormal()` likely helps with the desired window behavior, which is why the revert works better.
(https://github.com/bitcoin-core/gui/pull/904#issuecomment-3563438977)
> For example, on Fedora 43 with Gnome 49 and Wayland
Ah, I haven't tested this on GNOME. I suspect the use of `showNormal()` likely helps with the desired window behavior, which is why the revert works better.
💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2550098336)
Not a very strict reason, but to allow more flexibility and not to break the search for minimal whitespaces or indentation changes.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2550098336)
Not a very strict reason, but to allow more flexibility and not to break the search for minimal whitespaces or indentation changes.
💬 hebasto commented on pull request "cmake: make missing Python interpreter behaviour more explicit":
(https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2550139808)
> Concept ACK
>
> I am however unable to trigger the fatal error on `Ubuntu 24.04.3 LTS` with `Python 3.9.23`
I cannot reproduce the issue:
```
$ cat /etc/os-release | head -1
PRETTY_NAME="Ubuntu 24.04.3 LTS"
$ python3 --version
Python 3.9.23
$ cmake -B build
<snip>
-- Could NOT find Python3 (missing: Python3_EXECUTABLE Interpreter) (Required is at least version "3.10")
Reason given by package:
Interpreter: Wrong version for the interpreter "/home/hebasto/.pyenv/shim
...
(https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2550139808)
> Concept ACK
>
> I am however unable to trigger the fatal error on `Ubuntu 24.04.3 LTS` with `Python 3.9.23`
I cannot reproduce the issue:
```
$ cat /etc/os-release | head -1
PRETTY_NAME="Ubuntu 24.04.3 LTS"
$ python3 --version
Python 3.9.23
$ cmake -B build
<snip>
-- Could NOT find Python3 (missing: Python3_EXECUTABLE Interpreter) (Required is at least version "3.10")
Reason given by package:
Interpreter: Wrong version for the interpreter "/home/hebasto/.pyenv/shim
...
📝 Sjors opened a pull request: "mining: add getMemoryLoad() and track template non-mempool memory footprint"
(https://github.com/bitcoin/bitcoin/pull/33922)
Implements the template memory footprint tracking discussed #33899, but does not yet impose a limit.
(https://github.com/bitcoin/bitcoin/pull/33922)
Implements the template memory footprint tracking discussed #33899, but does not yet impose a limit.
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2550158500)
Agree that would be slightly more optimal, avoiding some needless pushes/pops. Taken in latest push + changed `auto& i` to `Node& n` for clarity.
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2550158500)
Agree that would be slightly more optimal, avoiding some needless pushes/pops. Taken in latest push + changed `auto& i` to `Node& n` for clarity.
💬 Sjors commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3563543871)
I haven't benchmarked this yet on mainnet, so I'm not sure if checking every (unique) transaction for mempool presence is unacceptably expensive.
If people prefer, I could also add a way for the `getblocktemplate` RPC to opt-out of the memory bookkeeping, since it holds on to one template max and no longer than a minute.
(https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3563543871)
I haven't benchmarked this yet on mainnet, so I'm not sure if checking every (unique) transaction for mempool presence is unacceptably expensive.
If people prefer, I could also add a way for the `getblocktemplate` RPC to opt-out of the memory bookkeeping, since it holds on to one template max and no longer than a minute.
💬 mzumsande commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2550173315)
hmm, the conflicting a0eaa4492548800ba1b2cdd8232195ab5d5c49c7 was merged on August 8. I thought we had regular re-runs of the CI with respect to current master that should have failed - or was the CI already red yesterday and I missed it?
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2550173315)
hmm, the conflicting a0eaa4492548800ba1b2cdd8232195ab5d5c49c7 was merged on August 8. I thought we had regular re-runs of the CI with respect to current master that should have failed - or was the CI already red yesterday and I missed it?