💬 sipa commented on pull request "rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1263804601)
WIP just means "this is not ready for review yet".

You're always expected to address review comments, whether that means making changes, or giving a justification why it's better not to.

💬 MarcoFalke commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1635954359)
Just ran both CI configs locally on aarch64 and they passed.

💬 hebasto commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1635956894)
Will it be the first Rust code in our codebase?

💬 fanquake commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1635957683)
> Apparently, this macro prevents compiler warnings

Either it does or it doesn't? If it doesn't, then any workarounds should be removed, and warnings re-enabled.

If it doesn't, there's no point to this refactoring at all, if it doesn't even acheive the stated goals.

> I find it frustrating to play compiler golf against g++ in the CI on every code push that uses a switch-case on an enum class.

Which CI is this? The only one where it is an issue has the warnings disabled?

🤔 furszy reviewed a pull request: "wallet: don't duplicate change output if already exist"
(https://github.com/bitcoin/bitcoin/pull/27601#pullrequestreview-1530430019)
Updated. Added required changes to make coin selection, change amount and fee calculation processes take into consideration the presence of an existing change output. Improving the accuracy of the results and avoiding cases where we end up having to increase the change output at the end of the process by the fee surplus.

Changes:
* Cleaned two change related members from the `CoinSelectedParams` struct that are not used inside the coin selection process. They were only intermediate results
...

: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 😅