💬 ItIsOHM commented on pull request "rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1263848850)
Ahh okay, I'm sorry about that. Will make these changes asap and push another commit.

💬 fanquake commented on issue "Spurious (?) valgrind failure for p2p_compactblocks.py":
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1636006713)
> Maybe someone can report the bug to valgrind upstream?

Looks like the issue no-longer reproduces using the steps from https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1563230842 (for me) using current master. Does this still repro for anyone else?

💬 MarcoFalke commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1636031885)
> Which CI is this? The only one where it is an issue has the warnings disabled?

I don't use g++, nor Windows, locally, so this is a general problem of not knowing which function is marked noreturn by g++ or msvc internally. Having a macro that is known to work on all compilers is helpful, I think.

I don't care about updating the existing code, but I think having a macro for those that find it helpful, can't hurt, can it?

📝 vasild opened a 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)
### Background

In the `i2p::sam::Session` class:

`Listen()` does:
* if the session is not created yet
* create the control socket and on it:
* `HELLO`
* `SESSION CREATE ID=sessid`
* leave the control socked opened
* create a new socket and on it:
* `HELLO`
* `STREAM ACCEPT ID=sessid`
* read reply (`STREAM STATUS`)

Then a wait starts, for a peer to connect. When connected,

`Accept()` does:
* on the socket from `STREAM ACCEPT` from `Listen()`: read the Base64 identif
...

💬 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-1636048489)
Concept ACK. Have been testing the issue and various fixes by @vasild the past couple weeks in order to fix the issue described in

- https://github.com/bitcoin/bitcoin/issues/22759#issuecomment-903612128
- https://github.com/bitcoin/bitcoin/issues/22759#issuecomment-1599393359
- https://github.com/bitcoin/bitcoin/issues/22759#issuecomment-1609907115

and discussed on bitcoin-core-dev IRC at https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-07-07#941511.

💬 jonatack commented on issue "I2P: Creating SAM session with 127.0.0.1:7656":
(https://github.com/bitcoin/bitcoin/issues/22759#issuecomment-1636050278)
@rebroad @Sataur A fix to the issue you described has been proposed in https://github.com/bitcoin/bitcoin/pull/28077, if you'd like to test it.

💬 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
...