💬 MarcoFalke commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245084467)
style-nit: Seems odd to have three asserts to check two things. Can be written shorter and easier to read:

```suggestion
p = [p for p in self.getpeerinfo() if p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}"]
assert_equal(len(p), 1)
assert_equal(p[0]["subver"], P2P_SUBVERSION)
```

💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1611246591)
> ... unsigned integer overflow ...

Maybe collecting the number of "full outbound or manual" connections on the fly was not so bad? I am not objecting pre-collecting and maintaining that number in `CConnman`, just thinking aloud. That looks a bit more tricky and maybe more prone to bugs (like the above). It is also not generic - the "full outbound or manual" number is used only in this specific case and unlikely to be used elsewhere in the future. A more generic approach, that could possibly
...

💬 carnhofdaki commented on issue "Add maxrelaytxfee":
(https://github.com/bitcoin/bitcoin/issues/27983#issuecomment-1611258937)
OK, thanks.

✅ carnhofdaki closed an issue: "Add maxrelaytxfee"
(https://github.com/bitcoin/bitcoin/issues/27983)

💬 kristapsk commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1611260025)
Concept ACK

💬 hebasto commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1611271297)
Guix build on `x86_64`:
```
7851f08737a0133002c0224d4bd5e52aed7eaee91adac46f1247ef164775d037 guix-build-23d39c5079cf/output/arm64-apple-darwin/SHA256SUMS.part
172e378d18cd9c624e4cdce51228bfbb43d25098871c18911c70eeb1c2390f34 guix-build-23d39c5079cf/output/arm64-apple-darwin/bitcoin-23d39c5079cf-arm64-apple-darwin-unsigned.tar.gz
3de4d2cec142a8464788da7571b2f09a47893c7f8d75fe52912ecfcefac72242 guix-build-23d39c5079cf/output/arm64-apple-darwin/bitcoin-23d39c5079cf-arm64-apple-darwin-unsigned
...

💬 Sjors commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1611297395)
#19225 :-)

🚀 fanquake merged a pull request: "ci: remove duplicate bsdmainutils from CI configs"
(https://github.com/bitcoin/bitcoin/pull/27987)

🤔 stickies-v reviewed a pull request: "http: add evhttp_connection_set_closecb to avoid g_requests hang"
(https://github.com/bitcoin/bitcoin/pull/27909#pullrequestreview-1502978676)
Approach ACK 710985d6953eb73b5edddaa7d4d54b58c90d1d34

I think improving remote client disconnect detection is best left for a separate PR; this is a worthwhile improvement on its own and I think uncontroversial.

💬 stickies-v commented on pull request "http: add evhttp_connection_set_closecb to avoid g_requests hang":
(https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1245140984)
nit
```suggestion
auto conn{evhttp_request_get_connection(req)};
```

💬 stickies-v commented on pull request "http: add evhttp_connection_set_closecb to avoid g_requests hang":
(https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1245143903)
I'm curious why we make `notify_all()` dependent on `g_requests.empty()` here, when in all other places we notify for all added and removed events?

```suggestion
if (g_requests.erase(req)) g_requests_cv.notify_all();
```

💬 stickies-v commented on pull request "http: add evhttp_connection_set_closecb to avoid g_requests hang":
(https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1245141287)
nit
```suggestion
auto req{static_cast<evhttp_request*>(arg)};
```

💬 sipa commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1611334433)
@Sjors Thanks, I had forgotten about that. Added a reference.

⚠️ dergoegge opened an issue: "libsecp256k1 not instrumented when building with sanitizers"
(https://github.com/bitcoin/bitcoin/issues/27990)
Our libsecp256k1 builds are not instrumented when building with sanitizers (using `--with-sanitizers`).

For example building with:
```
./configure --enable-fuzz --with-sanitizers=fuzzer
make
```
will not instrument secp code paths for fuzzing. Can be checked with `objdump`, e.g. `objdump --disassemble-symbols=secp256k1_xonly_pubkey_serialize src/test/fuzz/fuzz`.

As a workaround it is possible to set `CFLAGS` to use the desired sanitizers (e.g. CFLAGS=`-fsanitize=fuzzer-no-link`) but i
...

💬 fanquake commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1611341497)
Not sure what is happening here, but when testing this commit (rebased on master), on aarch64, `make check` just "dies" in the wallet tests? https://gist.github.com/fanquake/087fdff84b0d5379d94859de47a0e797

💬 dergoegge commented on pull request "refactor: Continue moving application data from CNode to Peer":
(https://github.com/bitcoin/bitcoin/pull/26621#issuecomment-1611345224)
> Needs rebase if still relevant

Rebased

💬 TheCharlatan commented on pull request "guix: Clean up manifest":
(https://github.com/bitcoin/bitcoin/pull/27811#issuecomment-1611386623)
ACK a51d7abf1e13c532c7acf437c3a65a9511b44987

Guix builds:
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
6edd213a90a043d09da144b99332ab16a320774098d35de28464262b25f260b2 guix-build-a51d7abf1e13/output/aarch64-linux-gnu/SHA256SUMS.part
1ececa8121d51a5c358e25e1a4b529413faadef5721387005db2b928c05cad3d guix-build-a51d7abf1e13/output/aarch64-linux-gnu/bitcoin-a51d7abf1e13-aarch64-linux-gnu-debug.tar.gz
fbc1724995b2d9
...

💬 MarcoFalke commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1611401222)
Ah, sorry that's a bug. Can you check `log_path=${BASE_SCRATCH_DIR}/sanitizer-output/tsan`?

💬 hebasto commented on pull request "guix: Clean up manifest":
(https://github.com/bitcoin/bitcoin/pull/27811#issuecomment-1611401634)
Guix builds:
```
6edd213a90a043d09da144b99332ab16a320774098d35de28464262b25f260b2 guix-build-a51d7abf1e13/output/aarch64-linux-gnu/SHA256SUMS.part
1ececa8121d51a5c358e25e1a4b529413faadef5721387005db2b928c05cad3d guix-build-a51d7abf1e13/output/aarch64-linux-gnu/bitcoin-a51d7abf1e13-aarch64-linux-gnu-debug.tar.gz
fbc1724995b2d9a43c0bccbb3c20e02d115d9aeaecce0792bde46d9fedc0dbe0 guix-build-a51d7abf1e13/output/aarch64-linux-gnu/bitcoin-a51d7abf1e13-aarch64-linux-gnu.tar.gz
8a9edb553d6e6db1d63
...

💬 MarcoFalke commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1611403242)
Anyway, I'd guess it's the bdb upstream data race, so maybe for now try without bdb?