💬 willcl-ark commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1653142279)
I don't think I can give this much competent review I'm afraid. Partly because I can't get it working locally, and partly because I don't feel like I understand the changes fully yet.

I did try to run `MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_s390x.sh" ./ci/test_run_all.sh`, but it takes more than 2 hours to run (amd64 local arch), and I keep getting (possibly) unrelated errors like this: [390x_build_error.txt](https://github.com/bitcoin/bitcoin/files/12180726/390x_build_error.t
...

💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1653188244)
> but it takes more than 2 hours to run

Yes, this is expected. Instead of just running `bitcoind` through qemu, now the *whole* container is run through qemu.

> unrelated errors

Jup, this should be unrelated, I presume it also happens on current master. You may be able to fix it by upgrading your `qemu-s390x --version`. IIRC qemu 7.2 or later is required.

> this is roughly the same (but simpler to configure from our side) than running an x86 image, manually installing qemu, and then
...

💬 willcl-ark commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1653195617)
Thanks Marco, thats a very helpful explanation (and I nice changes for us!).

I will give it another go this afternoon after trying to update qemu.

💬 MarcoFalke commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#issuecomment-1653206694)
(reworked to second commit to only use a single `sed` command, and add documentation for why it is needed)

👍 TheCharlatan approved a pull request: "ci: document that -Wreturn-type has been fixed upstream (mingw-w64)"
(https://github.com/bitcoin/bitcoin/pull/28092#pullrequestreview-1549403983)
ACK 08eb5f1b67e2af009549717eb5c66b7d7905731f

I'm fine with the warnings being printed until the mingw toolchain is rolled out everywhere.

Guix build:
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
daeec3e7ea34ddf28200d95d96e3295cadfc48fba9d95234880
...

💬 fanquake commented on pull request "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653243314)
Updated to use the new plugin, https://github.com/theuni/bitcoin-tidy-plugin, and new check `bitcoin-unterminated-logprintf`.
Updated the PR description.

cc @theuni @stickies-v @dergoegge @TheCharlatan

👋 fanquake's pull request is ready for review: "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296)

💬 dergoegge commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653248089)
`lint-logs.py` can probably be deleted?

💬 naumenkogs commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1653253819)
ACK 7efb30b

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653257372)
> lint-logs.py can probably be deleted?

Added a commit to drop it.

💬 naumenkogs commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1653257672)
@pinheadmz In the example I had above `n_extra_slots` controls how many extra connections you allow so it should be safe.

👍 stickies-v approved a pull request: "refactor: Revert additional univalue check in ParseSighashString"
(https://github.com/bitcoin/bitcoin/pull/28162#pullrequestreview-1549441994)
ACK 06199a995f20c55583f6948cfe99e608679fcdf1

💬 MarcoFalke commented on pull request "refactor: Revert additional univalue check in ParseSighashString":
(https://github.com/bitcoin/bitcoin/pull/28162#issuecomment-1653269199)
lgtm ACK 06199a995f20c55583f6948cfe99e608679fcdf1

💬 naumenkogs commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1274659835)
3d06fa372c963a0957f54fe80a0ff3462cdb3a2d
nit: might want to rename the file too :)

💬 naumenkogs commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1276031483)
30b0f57b19eddd47fa137d0c20ae39174b54dbad

Doesn't this substantially change the observations of a user? If they get unlucky with the 10 seeds and gets stuck for 2 minutes, which was not the case before?

Maybe worth expanding a log `this might take up to 2 minutes.` or something. Ideally we won't wait anymore once we realize all those attempts failed already, but that might be difficult to code.

💬 stickies-v commented on pull request "refactor: Revert additional univalue check in ParseSighashString":
(https://github.com/bitcoin/bitcoin/pull/28162#discussion_r1276034368)
Ooh actually, would this be better? Equally clear (I think), but we still feed it unexpected types.

```diff
diff --git a/src/test/fuzz/parse_univalue.cpp b/src/test/fuzz/parse_univalue.cpp
index a3d6ab6375..df3410eab8 100644
--- a/src/test/fuzz/parse_univalue.cpp
+++ b/src/test/fuzz/parse_univalue.cpp
@@ -67,8 +67,10 @@ FUZZ_TARGET(parse_univalue, .init = initialize_parse_univalue)
} catch (const std::runtime_error&) {
}
try {
- if (univalue.isNull() || univalue
...

📝 willcl-ark opened a pull request: "init: Add option for rpccookie permissions (replace 26088)"
(https://github.com/bitcoin/bitcoin/pull/28167)
This PR picks up #26088 by @aureleoules and adds a bitcoind launch option `-rpccookieperms` to configure the file permissions of the cookie generated by bitcoin core.

Example usage: `./src/bitcoind -rpccookieperms=0640`.

I added a length check to `StringToOctal()` to address @luke-jr's review comment [here](https://github.com/bitcoin/bitcoin/pull/26088#discussion_r1275362802) and swapped the ordering of the permission setting and data writing in `GenerateAuthCookie()` as per his other [sug
...

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions":
(https://github.com/bitcoin/bitcoin/pull/26088#discussion_r1276039698)
Thanks for the reminder on this luke, as I said I'd take it up previously. I took these changes in opening #28167

💬 fanquake commented on pull request "guix: use GCC 12.3.0 to build releases":
(https://github.com/bitcoin/bitcoin/pull/27897#issuecomment-1653318660)
Pushed an update that incorporates a number of changes.
* Updated to latest time-machine, which includes a number of updates for packages we've upstreamed.
* Reworked our manifest to a state that I think is an improvement over what it used to be, and which avoids Guix build failures, i.e https://lists.gnu.org/archive/html/bug-guix/2023-07/msg00009.html.
* WIP commits are now split per platform.

Still todo:
* macOS libtapi failure.
* Re-enable / follow up with `powerpc64-linux-gnu` build
...

📝 MarcoFalke opened a pull request: "refactor: Remove unused raw-pointer read helper from univalue"
(https://github.com/bitcoin/bitcoin/pull/28168)
The helpers are unused outside of tests and redundant with the existing `bool read(std::string_view raw);`.

Fix both issues by removing them.

Also, simplify the tests code by removing a `std::string` constructor where possible.

🚀 fanquake merged a pull request: "ci: document that -Wreturn-type has been fixed upstream (mingw-w64)"
(https://github.com/bitcoin/bitcoin/pull/28092)