💬 maflcko commented on pull request "fuzz: Fix misplaced SeedRand::ZEROS":
(https://github.com/bitcoin/bitcoin/pull/31521#issuecomment-2555198249)
> But (unfortunately?) the checker doesn't really check that. It only checks that if a fuzz test uses the RNG, then it must seed it with zeros at some point during the test. The checker doesn't check that seeding happens **before** using the RNG.

Sure, makes sense to add this assert as well

👍 theuni approved a pull request: "guix: latest 2.31 glibc"
(https://github.com/bitcoin/bitcoin/pull/31529#pullrequestreview-2515661465)
utACK b8710201fbd01bf2cba3c3b1fd312316b1ae22af

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1892975685)
> wen `s/Span/std::span/` all over the place?

now: https://github.com/bitcoin/bitcoin/pull/31519

💬 vasild commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2555525700)
Concept ACK

📝 maflcko opened a pull request: " test: Embed univalue json tests in binary "
(https://github.com/bitcoin/bitcoin/pull/31542)
All other benchmarks and tests have their data embedded, except for the univalue json tests.

This is not only confusing, but also problematic, when the test binary is moved to a different system for testing, because one has to put the test files in the source dir that was used at compile-time.

Fix all issues by embedding them. Also, re-enable a disabled test. Also, fix an issue in the GenerateHeaderFromJson.cmake.

Requested in https://github.com/bitcoin/bitcoin/pull/31434/files#r1876000
...

📝 hebasto opened a pull request: "cmake: Always provide `RPATH` on NetBSD"
(https://github.com/bitcoin/bitcoin/pull/31543)
Apparently, runtime paths cannot be skipped on NetBSD, even for system-wide packages.

On NetBSD 10.0:
- on the master branch @ bb57017b2945d5e0bbd95c7f1a9369a8ab7c6fcd:
```
$ cmake -B build -DCMAKE_C_COMPILER="/usr/pkg/gcc14/bin/gcc" -DCMAKE_CXX_COMPILER="/usr/pkg/gcc14/bin/g++"
$ cmake --build build
$ ./build/src/bitcoin-wallet -version
./build/src/bitcoin-wallet: Shared object "libsqlite3.so.0" not found
$ cmake --install build --prefix /home/hebasto/INSTALL
$ /home/hebasto/INSTALL/
...

💬 hebasto commented on pull request "qa: Limit `-maxconnections` in tests":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2555661793)
Tracked in https://github.com/hebasto/bitcoin-core-nightly/issues/6.

💬 hebasto commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2555662120)
Tracked in https://github.com/hebasto/bitcoin-core-nightly/issues/6.

💬 hebasto commented on pull request "cmake: Always provide `RPATH` on NetBSD":
(https://github.com/bitcoin/bitcoin/pull/31543#issuecomment-2555662426)
Tracked in https://github.com/hebasto/bitcoin-core-nightly/issues/6.

💬 theuni commented on pull request "cmake: Always provide `RPATH` on NetBSD":
(https://github.com/bitcoin/bitcoin/pull/31543#issuecomment-2555679006)
What makes NetBSD special in this regard?

💬 ryanofsky commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893029530)
re: https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1886502862

> prefer raising `AssertionError` in less roundabout way:

Reason it's written this way is so `-O` or `PYTHONOPTIMIZE` doesn't skip checks this test is trying to perform. (IMO it only makes sense to use `assert` to check for internal assumptions made by test code, which could be skipped without changing coverage provided by the test.)

💬 ryanofsky commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893029635)
re: https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1886540011

> Instead of waiting for `addcon_thread_started`, one could wait directly on the asserted content, removing the need to add functionality for yeilding the log content

Reason for doing this is to fit in with existing tests in this file, and to fail right away instead of timing out when the expected log prints are missing. Tests that time out instead of failing are unpleasant to work with.

💬 ryanofsky commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893052948)
re: https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1886811373

I think the comment was accurate but I deleted it now since I already deleted the other comments about this code, too. (The comment was saying that if -noconnect was specified the seednode would be ignored but no "-seednode is ignored when -connect is used" warning would be shown, because `m_specified_outgoing` would be empty. I don't think having the `m_specified_outgoing` condition for that warning makes sense, just li
...

💬 ryanofsky commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893029815)
re: https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1886526104

> Could add comment about why this is useful, either in the code or commit ([f6d6383](https://github.com/bitcoin/bitcoin/commit/f6d638331fff8a9b1bd670abe65ee70d1441ad9a))?

Thanks, added comment to explain this.

🤔 ryanofsky reviewed a pull request: "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2516002191)
Thanks for the new review!

Updated 54ad580775b8379a62b05b1f2cdf8b7fde993306 -> 80608ba5d282ae8713ea0136ea9a0208b254c082 ([`pr/listset.5`](https://github.com/ryanofsky/bitcoin/commits/pr/listset.5) -> [`pr/listset.6`](https://github.com/ryanofsky/bitcoin/commits/pr/listset.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/listset.5..pr/listset.6)) improving various comments and adding a new commit to handle `-noasmap` based on Martin's suggestion https://github.com/bitcoin/bitcoin/
...

💬 ryanofsky commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893029408)
re: https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1886570537

> nit: Would be nice with a corresponding `else` + check that at least warned if other categories were being mixed together with `0`/`none`.

Definitely agree. I don't think it relates to this PR, but the error checking implemented in this part of code is not great.

💬 ryanofsky commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893029305)
re: https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1886587869

> nit: Just seems plain wrong to use `GetArgs` for this parameter if we only allow one value. The only benefit of doing it this way is if one doesn't want to silently overwrite a previous setting value with a following one I guess?

Yes that goes back to introduction of this code in e8990f121405af8cd539b904ef082439261e6c93 from #18267. I don't think this is ideal, but I could also understand allow not wanting to allow
...

💬 TheCharlatan commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#issuecomment-2555691218)
Nice, Concept ACK

💬 hebasto commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#issuecomment-2555699712)
Concept ACK. Thank you for picking this up!

💬 laanwj commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1893147509)
xcb libraries are part of the X system libraries, everyone running X11 (or xwayland) on linux will have them, someone not running that isn't going to run bitcoin-qt
there's no problem here