:lock: fanquake locked an issue: "bitcoind dumps core when deriveaddresses is called with index 2147483647 (2^31-1)"
(https://github.com/bitcoin/bitcoin/issues/26274)

💬 kristapsk commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1635981682)
Why to introduce Rust as additional dependency just for the linter code?

💬 MarcoFalke commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1636003349)
> Why to introduce Rust as additional dependency just for the linter code?

Because I don't think it is a good use of developer time to write not type-safe code, only to later (after review or merge) discover that it crashes with a `TypeError`. (Just one example: https://github.com/bitcoin/bitcoin/pull/27921#issuecomment-1601003548, obviously the problem is a general one and it is left as an exercise to the reader to create a full list of past issues, including the ones that were more severe.)
...

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