💬 Sjors commented on pull request "net: reduce CAddress usage to CService or CNetAddr":
(https://github.com/bitcoin/bitcoin/pull/31854#issuecomment-2658830814)
ACK cd4bfaee10

💬 fanquake commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2658854038)
> to list the stuff I noticed that is using GCC coverage:

The only thing mentioned here is corecheck. Is there anything else (other than your use)?

> Figure out what the coverage workflow should look like. "Should there be gcc support at all?"

If we are currently unsure what our coverage support should even look like, I'm not sure we should ship something just because it exists, maybe just to delete it in the next release; that's why it's attached to a milestone. It seems reasonable to come t
...

🚀 fanquake merged a pull request: "Cleanups to port mapping module post UPnP drop"
(https://github.com/bitcoin/bitcoin/pull/31157)

💬 fanquake commented on pull request "depends: set `CMAKE_*_COMPILER_TARGET` in toolchain":
(https://github.com/bitcoin/bitcoin/pull/31849#discussion_r1955899910)
Added.

💬 fanquake commented on pull request "depends: set `CMAKE_*_COMPILER_TARGET` in toolchain":
(https://github.com/bitcoin/bitcoin/pull/31849#issuecomment-2658901801)
> This will become a persistent source of confusion and complains,

It's only visible during a verbose build, and only happens when cross-compiling for macOS, on Linux. Which is certainly a less common build for anyone to actually be doing (outside of a Guix build, where it also wouldn't be visible).

> Would it be worth filtering out --target=$ARCH-apple-darwin from CMAKE_<LANG>_COMPILER in the toolchain file?

It probably depends on how much code is needed, but I'm not sure filtering sho
...

🤔 eval-exec reviewed a pull request: "random: Initialize variables in hardware RNG functions"
(https://github.com/bitcoin/bitcoin/pull/31863#pullrequestreview-2617370922)
Hi @theuni, I'd appreciate it if you could review this PR.

💬 vasild commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2658930755)
I tried the coverage instructions from `doc/developer-notes.md` and gcc/gcov generated a reasonable report. It does not look broken. No reason to remove a working, documented and used feature, with urgency, in 29.

💬 Sjors commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2658932409)
Tested 9b033bebb18dfd609c02736292f37cc6589fcc8d a bit on arm macOS 15.3.

🚀 fanquake merged a pull request: "cmake: Improve compatibility with Python version managers"
(https://github.com/bitcoin/bitcoin/pull/31421)

💬 maflcko commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2658944005)
> Can you elaborate on "all the GCC/lcov/gcov problems" you mentioned here: [#31588 (comment)](https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2648315209)? Seems like there is also some [agreement there](https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2651994364) to switch to LLVM/Clang.

I don't use macOS, nor have I tested this specific bash script on the different Linux distros mentioned, so I can't vouch for the problems, but it looks like some people ran into problems
...

💬 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
...

💬 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.

💬 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.

💬 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)
```

💬 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
...

👍 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

💬 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

💬 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
...

💬 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

💬 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.