💬 jonatack commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1636052860)
Win64 CI doesn't like the braced initializer list: https://github.com/bitcoin/bitcoin/pull/28077/checks?check_run_id=15050079549.

```
test/i2p_tests.cpp: In member function ‘void i2p_tests::listen_ok_accept_fail::test_method()’:
test/i2p_tests.cpp:114:40: error: no matching function for call to ‘CService::CService(<brace-enclosed initializer list>)’
114 | CService{in_addr{.s_addr = 0x0100007f}, 7656},
```

💬 MarcoFalke commented on issue "Spurious (?) valgrind failure for p2p_compactblocks.py":
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1636053197)
I can still reproduce with https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1562731029.

💬 stickies-v commented on pull request "http: add evhttp_connection_set_closecb to avoid g_requests hang":
(https://github.com/bitcoin/bitcoin/pull/27909#issuecomment-1636058567)
> I wasn't able to reproduce it and still want to know what exactly happened, but I think removing the assert here is certainly better than having a crash.

I'm able to reproduce @dooglus's observed crash quite quickly (< 1 second usually) by blasting the server with requests from > 1 thread.

<details>
<summary>diff and script to make it crash</summary>

On top of d170e6c60cd9f46da1794448b8be55ba6f0b2922, first re-add the assertion:

```diff
diff --git a/src/httpserver.cpp b/src/https
...

💬 fanquake commented on issue "Spurious (?) valgrind failure for p2p_compactblocks.py":
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1636060128)
> I can still reproduce with https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1562731029.

Ah yep, it still happens (and I guess only happens) on x86_64, not on aarch64 (where I tested earlier).

👍 stickies-v approved a pull request: "rpc: doc: Added `longpollid` and `data` params to `template_request` #27998"
(https://github.com/bitcoin/bitcoin/pull/28056#pullrequestreview-1530570529)
tACK f6a26196cfb7e2c90e25f82b0e2f569a05013cae

Thanks for picking this up and swiftly addressing feedback!

💬 ItIsOHM commented on pull request "rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1636078168)
> tACK [f6a2619](https://github.com/bitcoin/bitcoin/commit/f6a26196cfb7e2c90e25f82b0e2f569a05013cae)
>
> Thanks for picking this up and swiftly addressing feedback!

Thanks so much to each and every reviewer here for tolerating a beginner contributor like me 😅

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1263941686)
This resulted in `netbase.cpp:415:27: runtime error: implicit conversion from type 'int' of value 210 (32-bit, signed) to type 'char' changed the value to -46 (8-bit, signed)`. This is ok in practice because the data is transferred as bytes anyway over the socket (not as integers). Anyway, to silence the sanitizer:

<details>
<summary>[patch] sock: introduce a generic Sock::SendComplete(const void*, size_t, ...)</summary>

```diff
commit 8467080a08327aaf0d853a73412456c44558a9b3
Parent: af
...

🚀 achow101 merged a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048)

🤔 mzumsande reviewed a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1530681852)
ACK 31eca93a9eb8e54f856d3f558aa3c831ef181d37

The PR description is outdated - its last sentence can be removed.

🤔 jonatack reviewed a pull request: "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/27411#pullrequestreview-1530682192)
Post-merge ACK

💬 jonatack commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1263967598)
f4754b9dfb84859166843fb2a1888fb3cfebf73c It would be nice to avoid low-level `Network` enum value comparisons when we have built-in higher level helpers we can use.

💬 jonatack commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1263969104)
While touching this, the `GetLocal()` getter helper is unused outside the class and could be

- moved from the header to the implementation
- made static and nodiscard
- converted from a bool with an out-param to a std::optional without an out-param

📝 jonatack opened a pull request: "net, refactor: remove unneeded exports, improve separation, use std::optional"
(https://github.com/bitcoin/bitcoin/pull/28078)
and other improvements noticed while reviewing #27411. See commit messages for details.

⚠️ jonatack opened an issue: "libsecp CI failure [no wallet, libbitcoinkernel] [focal]"
(https://github.com/bitcoin/bitcoin/issues/28079)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

CI failure: https://cirrus-ci.com/task/5453591138271232


### Expected behaviour

Expect CI task to pass for a pull with unrelated changes.

### Steps to reproduce

See https://cirrus-ci.com/task/5453591138271232


### Relevant log output

```
FAIL: tests
===========

test count = 64
random seed = 81af32fd7ab8c9cbc2e62a689f642106
src/modules/ellswift/tests_impl.h:396: test conditio
...

💬 kristapsk commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1636198809)
> Because I don't think it is a good use of developer and reviewer time to write not type-safe code, only to later (after review or merge) discover that it crashes with a `TypeError`.

Why not just use C++ then? (I'm not a Rust expert, maybe there are good reasons)

> I can't recall the last time I've run any linter outside of testing one after writing the code for it.

I try always to remember running linters locally before submitting a new PR (sometimes I fail and forget, need to admit)
...

💬 jonatack commented on issue "libsecp CI failure [no wallet, libbitcoinkernel] [focal]":
(https://github.com/bitcoin/bitcoin/issues/28079#issuecomment-1636218979)
Possibly spurious. The CI task passed after re-running it: https://cirrus-ci.com/task/5885691087814656

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1264030502)
Now its ` ERROR: Error during SOCKS5 proxy handshake: send(): Transport endpoint is not connected (107)` I'm still digging in to this...

💬 furszy commented on pull request "[WIP] descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#discussion_r1264064042)
fixed

👋 jonatack's pull request is ready for review: "net, refactor: remove unneeded exports, use helpers over low-level code, use optional"
(https://github.com/bitcoin/bitcoin/pull/28078)

💬 jonatack commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1636339566)
Initial code review ACK (didn't review the test changes yet), will test.