👍 willcl-ark approved a pull request: "ci: Skip broken Wine64 tests by default"
(https://github.com/bitcoin/bitcoin/pull/31284#pullrequestreview-2468417480)
ACK fa5e7064597fc51366f082e3d07a4591e576db38

I agree with disabling these tests, and then pursuing a followup e.g. #31176 to run them natively on windows. There's no point in debugging false positives (and no? real positives).

💬 Sjors commented on issue "macOS 13.7 depends build can't find qt":
(https://github.com/bitcoin/bitcoin/issues/31050#issuecomment-2506339307)
It does indeed seem related to symlinks though, maybe something slightly different than what #31361 tries to fix. If I go from the symlinked to `~/dev` to its target (`cd /Volumes/SSD/Dev/bitcoin`), and then do the above `cmake -B ...` it works fine.

💬 Sjors commented on pull request "cmake, qt: Use absolute paths for includes in MOC-generated files":
(https://github.com/bitcoin/bitcoin/pull/31361#issuecomment-2506345496)
See #31050, this doesn't fix the scenario where the whole source dir is a symlink for me on macOS 13.7. But it's good that it does fix other things.

💬 maflcko commented on pull request "util: Drop boost posix_time in ParseISO8601DateTime":
(https://github.com/bitcoin/bitcoin/pull/31391#issuecomment-2506357477)
Looks like MSVC is broken, according to the GHA run:

```
D:/a/bitcoin/bitcoin/src/test/util_tests.cpp(334): error: in "util_tests/util_ParseISO8601DateTime": check ParseISO8601DateTime("9999-12-31T23:59:59Z").value() == 253402300799 has failed [-4295736961 != 253402300799]
```

It would be good if a someone using Windows reproduced this locally and then reported the bug to MSVC. Godbolt draft: https://godbolt.org/z/4WYn6E61W

💬 0xB10C commented on something "":
(https://github.com/bitcoin/bitcoin/commit/e6ff612fe6a41978d2c9dba7af0da36db24b9d85#r149713050)
The name should probably include `ci` somewhere

💬 hebasto commented on pull request "guix: use GCC 13 to build releases":
(https://github.com/bitcoin/bitcoin/pull/29881#issuecomment-2506436251)
My Guix build:
```
aarch64
827361e6e7a412ee6178440519b0716551c22fca9799a0a6cf17155790bfeef8 guix-build-09c2323e751f/output/aarch64-linux-gnu/SHA256SUMS.part
8cd3752d49cd087d8bc39947788b9bc2af9830063786b974debe651bc0fde99f guix-build-09c2323e751f/output/aarch64-linux-gnu/bitcoin-09c2323e751f-aarch64-linux-gnu-debug.tar.gz
56b76fcd39c8cb424ae04a5405ac95d836955334e53f7ff57c7bfe2219b05091 guix-build-09c2323e751f/output/aarch64-linux-gnu/bitcoin-09c2323e751f-aarch64-linux-gnu.tar.gz
62edbcf5
...

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1862455177)
Reverted in later pushes.

👍 TheCharlatan approved a pull request: "Remove `src/config` directory"
(https://github.com/bitcoin/bitcoin/pull/31390#pullrequestreview-2468548777)
ACK 935973b315fa3b99f5e79b360bcb66f473bb7b95

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1862479984)
To me this is a good pattern:
1. Document former *correct behavior* by adding tests.
2. Change implementation and tests to conform to *new behavior*.

However, if 1) is about documenting *buggy behavior*, committing a test for that in a way that makes them succeed feels off to me. In that case I would prefer adding tests that *prove failure* with some special annotation in the commit message for CI to expect failure like what @l0rinc [linked here (Spock)](https://github.com/bitcoin/bitcoin/p
...

📝 vasild converted_to_draft a pull request: "ci: detect outbound internet traffic generated while running tests"
(https://github.com/bitcoin/bitcoin/pull/31349)
Make sure that CI fails if some of the tests generate an outbound traffic on the non-loopback interface.

Resolves https://github.com/bitcoin/bitcoin/issues/31339

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2506560308)
Converted to draft for a while, testing docker with full privileges (need cirrus which does not run in my personal fork).

💬 ShreeHarsha-22 commented on issue "Add `sat_to_btc()` and conversely `btc_to_sat()` util functions in functional tests":
(https://github.com/bitcoin/bitcoin/issues/31345#issuecomment-2506647018)
Hey @rkrux, could you please assign this issue to me? I’d love to take a look and work on it.

💬 TheCharlatan commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1862626447)
This is also a debug only log, and it otherwise does not seem to be hitting any hot code paths, so I am not too concerned either.

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1862652609)
`GetAuthCookieFile()` internally uses `GetPathArg()` and follows it's pattern (predating this PR):
https://github.com/bitcoin/bitcoin/blob/7590e93bc73b3bbac641f05d490fd5c984156b33/src/common/args.cpp#L272-L274

But might change to `optional` if you insist. Tried out changing it in 01fa9d41d12a68f88f6251ae43234dd250a53bd5. Works fairly well, even uncovered a minor pre-existing bug in that if `fs::permissions` fails, we currently print out the wrong filename. With that bug it feels like it coul
...

💬 rebroad commented on issue "Prioritize processing of peers based on their CPU usage":
(https://github.com/bitcoin/bitcoin/issues/31033#issuecomment-2506724582)
> We already keep stats for each and every peer, displayed in the `getpeerinfo` RPC, e.g. `bitcoin-cli getpeerinfo`.

do those stats include CPU time though?

💬 mzumsande commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1862673015)
it's unconditional once out of IBD. But allowing untrusted parties access to this rpc is unsafe anyway (can spam the node with low-work blocks since `force_processing` and `min_pow_checked` are true for the `ProcessNewBlock` call).

👍 BrandonOdiwuor approved a pull request: "Remove `src/config` directory"
(https://github.com/bitcoin/bitcoin/pull/31390#pullrequestreview-2468804522)
ACK 935973b315fa3b99f5e79b360bcb66f473bb7b95

💬 theStack commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2506900085)
Opened a PR for BIP-373 which aims to improve the clarity of some PSBT fields containing pubkeys, which should hopefully also make reviewing this PR a bit easier: https://github.com/bitcoin/bips/pull/1705 (I sometimes had to look at the code first to find out how the specification is really to be interpreted...)

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1862855947)
> -maxconnections=0 -privatebrodcast and then addnode your trusted peer

Thanks for the idea, but sadly this does not work for this either. It will happily add only my trusted node, but it will not create the private broadcast connections when actually trying to send. It will just wait at 0 connections.

Since we break the maxconnections value already for the addnode exception, could we make an exception for private broadcast connections as well?

🤔 mzumsande reviewed a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2469192751)
Concept ACK