💬 hodlinator commented on pull request "test: Remove stale gettime test":
(https://github.com/bitcoin/bitcoin/pull/31846#issuecomment-2658947230)
Yeah, was experimenting with something like this as an extra sanity check.
```diff
diff --git a/src/util/time.cpp b/src/util/time.cpp
index cafc27e0d0..419ddb3702 100644
--- a/src/util/time.cpp
+++ b/src/util/time.cpp
@@ -34,6 +34,7 @@ NodeClock::time_point NodeClock::now() noexcept
mocktime :
std::chrono::system_clock::now().time_since_epoch()};
assert(ret > 0s);
+ assert(ret < std::chrono::years{200});
return time_point{ret};
};
```
But on
...
(https://github.com/bitcoin/bitcoin/pull/31846#issuecomment-2658947230)
Yeah, was experimenting with something like this as an extra sanity check.
```diff
diff --git a/src/util/time.cpp b/src/util/time.cpp
index cafc27e0d0..419ddb3702 100644
--- a/src/util/time.cpp
+++ b/src/util/time.cpp
@@ -34,6 +34,7 @@ NodeClock::time_point NodeClock::now() noexcept
mocktime :
std::chrono::system_clock::now().time_since_epoch()};
assert(ret > 0s);
+ assert(ret < std::chrono::years{200});
return time_point{ret};
};
```
But on
...
💬 fanquake commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2658948674)
Looking at Corecheck, does it use our `Coverage.cmake`? I can't find it being used anywhere there, seems like only calls to gcovr directly.
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2658948674)
Looking at Corecheck, does it use our `Coverage.cmake`? I can't find it being used anywhere there, seems like only calls to gcovr directly.
💬 vasild commented on pull request "[POC] cmake: Introduce LLVM's Source-based Code Coverage reports":
(https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2658958631)
> > Would we keep supporting generating coverage with gcc?
> I don't think we should keep both approaches.
Why not? GCC is the native compiler on Linux, a widely used OS for developing Bitcoin Core.
(https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2658958631)
> > Would we keep supporting generating coverage with gcc?
> I don't think we should keep both approaches.
Why not? GCC is the native compiler on Linux, a widely used OS for developing Bitcoin Core.
💬 janb84 commented on pull request "test: check `scanning` field from `getwalletinfo`":
(https://github.com/bitcoin/bitcoin/pull/31768#issuecomment-2658997619)
Post-merge ACK [bb0879d](https://github.com/bitcoin/bitcoin/commit/bb0879ddabc8b3a7253bc774d23b842937d18015)
Small improvement suggestion; also testing that the progress is between 0 and 1.
``` python
progress = wallet_info["scanning"]["progress"]
assert_approx(progress,0,1)
```
(https://github.com/bitcoin/bitcoin/pull/31768#issuecomment-2658997619)
Post-merge ACK [bb0879d](https://github.com/bitcoin/bitcoin/commit/bb0879ddabc8b3a7253bc774d23b842937d18015)
Small improvement suggestion; also testing that the progress is between 0 and 1.
``` python
progress = wallet_info["scanning"]["progress"]
assert_approx(progress,0,1)
```
💬 maflcko commented on pull request "random: Initialize variables in hardware RNG functions":
(https://github.com/bitcoin/bitcoin/pull/31863#discussion_r1955969666)
Setting the sanity check value to a default of `0` is fine, because if it was never set, it will run in an infinite loop, just as-if the rng failed.
However, setting the random value that is returned to a default value is problematic and controversial, because:
* It silences sanitizers such as msan and valgrind that detect uninitialized reads
* If we wanted to zero-init everything, we could just do https://github.com/bitcoin/bitcoin/issues/18892 (or a project-wide source change)
* The in
...
(https://github.com/bitcoin/bitcoin/pull/31863#discussion_r1955969666)
Setting the sanity check value to a default of `0` is fine, because if it was never set, it will run in an infinite loop, just as-if the rng failed.
However, setting the random value that is returned to a default value is problematic and controversial, because:
* It silences sanitizers such as msan and valgrind that detect uninitialized reads
* If we wanted to zero-init everything, we could just do https://github.com/bitcoin/bitcoin/issues/18892 (or a project-wide source change)
* The in
...
👍 laanwj approved a pull request: "net: reduce CAddress usage to CService or CNetAddr"
(https://github.com/bitcoin/bitcoin/pull/31854#pullrequestreview-2617470398)
Code review ACK cd4bfaee103591f212b88afd17969c16c1056eb6
(https://github.com/bitcoin/bitcoin/pull/31854#pullrequestreview-2617470398)
Code review ACK cd4bfaee103591f212b88afd17969c16c1056eb6
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1955976383)
Since `key_type` here is `std::shared_ptr<const Sock>` shouldn't this map also use the optimized hash/equal functions defined in:
https://github.com/bitcoin/bitcoin/blob/86528937e5c4da2e12c46085fc41e87ed759258e/src/util/sock.h#L208
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1955976383)
Since `key_type` here is `std::shared_ptr<const Sock>` shouldn't this map also use the optimized hash/equal functions defined in:
https://github.com/bitcoin/bitcoin/blob/86528937e5c4da2e12c46085fc41e87ed759258e/src/util/sock.h#L208
💬 Sjors commented on pull request "cmake: Improve compatibility with Python version managers":
(https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659034075)
Tested dead9086543671b07e6ce041821e4d2a6627075b on arm macOS 15.3 that before this PR cmake would find the Python version installed by Homebrew (3.13.1) rather than the one I set with PyEnv (3.10.14).
The system version at `/usr/bin/python3` is 3.9.6.
With this change it still does that and ignores PyEnv version:
```
-- Found Python3: /opt/homebrew/bin/python3 (found suitable version "3.13.1", minimum required is "3.10") found components: Interpreter
```
```
which -a python3
/op
...
(https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659034075)
Tested dead9086543671b07e6ce041821e4d2a6627075b on arm macOS 15.3 that before this PR cmake would find the Python version installed by Homebrew (3.13.1) rather than the one I set with PyEnv (3.10.14).
The system version at `/usr/bin/python3` is 3.9.6.
With this change it still does that and ignores PyEnv version:
```
-- Found Python3: /opt/homebrew/bin/python3 (found suitable version "3.13.1", minimum required is "3.10") found components: Interpreter
```
```
which -a python3
/op
...
💬 Sjors commented on pull request "cmake: Improve compatibility with Python version managers":
(https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659048632)
Same when testing on master @ 7bbd761e816b713ac88c87f8b0045ea2f958ecbf after this was merged. I can't uninstall python3 from Homebrew because it's a dependency for a bunch of things.
Full `cmake -B build` log: https://gist.github.com/Sjors/becdc73b4dadcfa26121f37a11665e59
(https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659048632)
Same when testing on master @ 7bbd761e816b713ac88c87f8b0045ea2f958ecbf after this was merged. I can't uninstall python3 from Homebrew because it's a dependency for a bunch of things.
Full `cmake -B build` log: https://gist.github.com/Sjors/becdc73b4dadcfa26121f37a11665e59
💬 eval-exec commented on pull request "random: Initialize variables in hardware RNG functions":
(https://github.com/bitcoin/bitcoin/pull/31863#discussion_r1955991364)
Agreed, seting `r1 = 0` is problematic.
(https://github.com/bitcoin/bitcoin/pull/31863#discussion_r1955991364)
Agreed, seting `r1 = 0` is problematic.
💬 hebasto commented on pull request "depends: set `CMAKE_*_COMPILER_TARGET` in toolchain":
(https://github.com/bitcoin/bitcoin/pull/31849#issuecomment-2659057610)
> > Would it be worth filtering out --target=$ARCH-apple-darwin from CMAKE__COMPILER in the toolchain file?
>
> It probably depends on how much code is needed, but I'm not sure filtering should be done unless its generic.
For example:
```diff
--- a/depends/toolchain.cmake.in
+++ b/depends/toolchain.cmake.in
@@ -29,8 +29,15 @@ if(NOT DEFINED CMAKE_C_FLAGS_DEBUG_INIT)
set(CMAKE_C_FLAGS_DEBUG_INIT "@CFLAGS_DEBUG@")
endif()
+macro(fliter_flags compiler_invocation_line)
+ if(CMA
...
(https://github.com/bitcoin/bitcoin/pull/31849#issuecomment-2659057610)
> > Would it be worth filtering out --target=$ARCH-apple-darwin from CMAKE__COMPILER in the toolchain file?
>
> It probably depends on how much code is needed, but I'm not sure filtering should be done unless its generic.
For example:
```diff
--- a/depends/toolchain.cmake.in
+++ b/depends/toolchain.cmake.in
@@ -29,8 +29,15 @@ if(NOT DEFINED CMAKE_C_FLAGS_DEBUG_INIT)
set(CMAKE_C_FLAGS_DEBUG_INIT "@CFLAGS_DEBUG@")
endif()
+macro(fliter_flags compiler_invocation_line)
+ if(CMA
...
💬 Sjors commented on pull request "cmake: Improve compatibility with Python version managers":
(https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659113645)
`brew unlink python3` "fixes" it for me. We could mention it as a build hint if someone runs into a problem. Not sure how likely that is to be problem though.
(https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659113645)
`brew unlink python3` "fixes" it for me. We could mention it as a build hint if someone runs into a problem. Not sure how likely that is to be problem though.
💬 fanquake commented on pull request "cmake: Improve compatibility with Python version managers":
(https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659124802)
It's not clear what the problem is here. Looking at https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659034075, it seems like the correct Python is being picked, because the brew installed python is `python3` on your system. If you want the pyenv Python to be picked, you should setup pyenv such that it's python is `python3` on your system.
(https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659124802)
It's not clear what the problem is here. Looking at https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659034075, it seems like the correct Python is being picked, because the brew installed python is `python3` on your system. If you want the pyenv Python to be picked, you should setup pyenv such that it's python is `python3` on your system.
💬 hebasto commented on pull request "cmake: Improve compatibility with Python version managers":
(https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659125423)
The initial [issue](https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516745547) was that CMake finds the Python version other than reported by `which python3` command.
> ```
> which -a python3
> /opt/homebrew/bin//python3
> /Users/sjors/.pyenv/shims/python3
> /opt/homebrew/bin/python3
> /usr/bin/python3
> ```
Your `PATH` looks a bit messy :)
(https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659125423)
The initial [issue](https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516745547) was that CMake finds the Python version other than reported by `which python3` command.
> ```
> which -a python3
> /opt/homebrew/bin//python3
> /Users/sjors/.pyenv/shims/python3
> /opt/homebrew/bin/python3
> /usr/bin/python3
> ```
Your `PATH` looks a bit messy :)
👍 willcl-ark approved a pull request: "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries"
(https://github.com/bitcoin/bitcoin/pull/31407#pullrequestreview-2617591133)
tACK 096525e92cc2f5a4318bae13cedd2cf36b928d5f
I have not thoroughly reviewed the code changes here, but I did read through all the changes. I was more interested in verifying the result of the changes was correct.
I tested the MacOS binaries on both an M3 and x86_64 (Intel) Macs. Of note however is the fact that the Intel Mac is 2017, and so can only run MacOS 10.13 (where 10.15 is the version giving us trouble with the new mandatory gatekeeper policies).
With that aside I can confirm t
...
(https://github.com/bitcoin/bitcoin/pull/31407#pullrequestreview-2617591133)
tACK 096525e92cc2f5a4318bae13cedd2cf36b928d5f
I have not thoroughly reviewed the code changes here, but I did read through all the changes. I was more interested in verifying the result of the changes was correct.
I tested the MacOS binaries on both an M3 and x86_64 (Intel) Macs. Of note however is the fact that the Intel Mac is 2017, and so can only run MacOS 10.13 (where 10.15 is the version giving us trouble with the new mandatory gatekeeper policies).
With that aside I can confirm t
...
💬 willcl-ark commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2659146803)
I don't want to delay getting this particular PR in before 29.0, but I do wonder if we might in the future consider tidying the the guix output directory a little? Currently I see:
```bash
$ fd -uu -e .tar.gz -e .zip -e .exe | cut -d '/' -f 2- | sort
bitcoin-096525e92cc2-aarch64-linux-gnu-debug.tar.gz
bitcoin-096525e92cc2-aarch64-linux-gnu.tar.gz
bitcoin-096525e92cc2-arm64-apple-darwin-codesigning.tar.gz
bitcoin-096525e92cc2-arm64-apple-darwin.tar.gz
bitcoin-096525e92cc2-arm64-apple-dar
...
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2659146803)
I don't want to delay getting this particular PR in before 29.0, but I do wonder if we might in the future consider tidying the the guix output directory a little? Currently I see:
```bash
$ fd -uu -e .tar.gz -e .zip -e .exe | cut -d '/' -f 2- | sort
bitcoin-096525e92cc2-aarch64-linux-gnu-debug.tar.gz
bitcoin-096525e92cc2-aarch64-linux-gnu.tar.gz
bitcoin-096525e92cc2-arm64-apple-darwin-codesigning.tar.gz
bitcoin-096525e92cc2-arm64-apple-darwin.tar.gz
bitcoin-096525e92cc2-arm64-apple-dar
...
📝 maflcko converted_to_draft a pull request: "test: Rename send_message to send_without_ping"
(https://github.com/bitcoin/bitcoin/pull/31859)
`send_message` is problematic, because it is easy to forget a `sync_with_ping` (or other `wait_until`), leading to intermittent test failures. (Example: https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1950370246)
There are more uses of `send_and_ping` in the codebase than `send_message`, so in most cases `send_and_ping` is needed anyway.
For the remaining cases, clearly document that no sync happens by renaming `send_message` to `send_without_ping`.
(https://github.com/bitcoin/bitcoin/pull/31859)
`send_message` is problematic, because it is easy to forget a `sync_with_ping` (or other `wait_until`), leading to intermittent test failures. (Example: https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1950370246)
There are more uses of `send_and_ping` in the codebase than `send_message`, so in most cases `send_and_ping` is needed anyway.
For the remaining cases, clearly document that no sync happens by renaming `send_message` to `send_without_ping`.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2659191470)
`563afdd975...e1671ff42c`: do https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1955976383
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2659191470)
`563afdd975...e1671ff42c`: do https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1955976383
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1956058967)
Yes, done!
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1956058967)
Yes, done!
💬 willcl-ark commented on pull request "cmake: Improve compatibility with Python version managers":
(https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659226739)
> If you want the pyenv Python to be picked, you should setup pyenv such that it's python is `python3` on your system.
Or said another way, rather than fixing by unlinking brew's python, if you want `pyenv` python to be picked up first, the usual move is to ensure the `$PYENV_ROOT/bin` component appears earlier than `/opt/homebrew/bin/` on your `$PATH`.
This is explained further in the [shell setup](https://github.com/pyenv/pyenv?tab=readme-ov-file#b-set-up-your-shell-environment-for-pyen
...
(https://github.com/bitcoin/bitcoin/pull/31421#issuecomment-2659226739)
> If you want the pyenv Python to be picked, you should setup pyenv such that it's python is `python3` on your system.
Or said another way, rather than fixing by unlinking brew's python, if you want `pyenv` python to be picked up first, the usual move is to ensure the `$PYENV_ROOT/bin` component appears earlier than `/opt/homebrew/bin/` on your `$PATH`.
This is explained further in the [shell setup](https://github.com/pyenv/pyenv?tab=readme-ov-file#b-set-up-your-shell-environment-for-pyen
...