💬 achow101 commented on pull request "refactor: Testnet4 - Replace uint256S("str") -> uint256{"str"}":
(https://github.com/bitcoin/bitcoin/pull/30721#issuecomment-2331847706)
> Needs backport to 28.x branch

Unless this fixes a bug that I'm missing, refactors generally are not candidates for backport.

📝 fanquake opened a pull request: "cmake: scope Boost Test check to `vcpkg`"
(https://github.com/bitcoin/bitcoin/pull/30822)
This check was added for `vcpkg`, given how it packages Boost. However, we don't need to run the check for other platforms, and it's quite slow. So, scope it to just `vcpkg`.

On my machine, this reduces the time to run `time cmake -B build` from ~12 seconds, to ~6 seconds.

Fixes: #30787.

💬 ryanofsky commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1745679987)
That would be nice and if I can figure out a way to check for that more directly I could drop the second half of this new test. It looks like there is a log message "%i block-relay-only anchors will be tried for connections" that gets printed when `m_use_addrman_outgoing` is false so maybe I could check for that when -noconnect is specified, and have better confirmation that noconnect is working correctly, than this `dnsseed_ignored` check provides.

I think the first half of the test checking
...

👍 hebasto approved a pull request: "cmake: scope Boost Test check to `vcpkg`"
(https://github.com/bitcoin/bitcoin/pull/30822#pullrequestreview-2283298003)
ACK dda17b44e7a680412a231e64a5a09120a850bb1f, I have reviewed the code and it looks OK.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745687847)
Seems a bit excessive to me, could we maybe specialize the tests when those errors are actually added?

📝 fanquake opened a pull request: "cmake: add `USE_SOURCE_PERMISSIONS` to all `configure_file()` usage"
(https://github.com/bitcoin/bitcoin/pull/30823)
`USE_SOURCE_PERMISSIONS` is the default, so this should not change behaviour. However, being explicit makes it clear what we are doing.

Related to #30815.

See https://cmake.org/cmake/help/latest/command/configure_file.html#options.

💬 fanquake commented on issue "build: reproducibility issue with macOS Guix builds":
(https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2331921478)
Opened #30823 to add the explicit permissions usage. We can also use that PR to try and solve this issue, if wanted.

💬 hebasto commented on pull request "cmake: add `USE_SOURCE_PERMISSIONS` to all `configure_file()` usage":
(https://github.com/bitcoin/bitcoin/pull/30823#issuecomment-2331926727)
Could variations in the source directory permissions actually cause https://github.com/bitcoin/bitcoin/issues/30815?

🤔 ryanofsky reviewed a pull request: "contrib: fix check-deps.sh to check for weak symbols"
(https://github.com/bitcoin/bitcoin/pull/30415#pullrequestreview-2283332344)
Updated 3e4312eef78f233eb7ae1d7d85e497de34144f2e -> b14d87085fca777b1d14ff051d31d41932597f06 ([`pr/weakcheck.9`](https://github.com/ryanofsky/bitcoin/commits/pr/weakcheck.9) -> [`pr/weakcheck.10`](https://github.com/ryanofsky/bitcoin/commits/pr/weakcheck.10), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/weakcheck.9..pr/weakcheck.10)) with suggested change to use cmake --build wrapper instead of calling make directly

💬 ryanofsky commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1745708014)
re: https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1745115109

> I think we are trying to consistently use `cmake --build` over `make `.

Thanks, switched to cmake --build

👍 TheCharlatan approved a pull request: "contrib: fix check-deps.sh to check for weak symbols"
(https://github.com/bitcoin/bitcoin/pull/30415#pullrequestreview-2283344366)
Re-ACK b14d87085fca777b1d14ff051d31d41932597f06

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745716933)
> ugly

Both clang and gcc print the subexpression as well as the violating call, and the error will look similar to a "real" error (except for being more verbose).

A failure in your example would be simply a core dump, which is even worse, no?

Moving the checks from being done at compile-time to be done at runtime means that possible errors will be caught later.

💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1745679946)
It was a matter of definition. The bare minimum does not really account for connections, even though running the node without any connection won't be too useful.

💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1745689399)
Yeah, I agree, I was going to de-globalize most of them but saw that they were used for logging later, so I decided not to overcomplicate the change by addressing things that are not directly related. I'll do so with `nUserMaxConnections` given it's not used for anything else

💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#issuecomment-2331953267)
Took (most of?) your suggestions @TheCharlatan

💬 TheCharlatan commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1745721750)
Oh, that makes sense!

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2331961628)
Well. At the least i still need to port it to the new build system (and rebase and squash). There are likely some other smaller comments and nits here and there that i can easily address, but it's kind of become a mess (no one's fault but github and how it handles PRs with lots of comments), so need to take some time to sort it out.

💬 ryanofsky commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2331965165)
Updated b14d87085fca777b1d14ff051d31d41932597f06 -> 3ae35b427fe59bc9ab24d07c1adb46faa702de20 ([`pr/weakcheck.10`](https://github.com/ryanofsky/bitcoin/commits/pr/weakcheck.10) -> [`pr/weakcheck.11`](https://github.com/ryanofsky/bitcoin/commits/pr/weakcheck.11), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/weakcheck.10..pr/weakcheck.11)) simplifying by dropping `cd` since cmake --build ignores the current directory.

👍 maflcko approved a pull request: "cmake: scope Boost Test check to `vcpkg`"
(https://github.com/bitcoin/bitcoin/pull/30822#pullrequestreview-2283372310)
lgtm

The alternative https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324671298 seems fine as well.

💬 maflcko commented on pull request "cmake: scope Boost Test check to `vcpkg`":
(https://github.com/bitcoin/bitcoin/pull/30822#discussion_r1745732446)
Comment indent seems off compared to the other cmake code? I guess there is no `.cmake-format.yml`, so feel free to ignore.