💬 jonatack commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#discussion_r1263776033)
```suggestion
*Rationale*: The comment documents skipping the `default:` case and it complies with `clang-format` rules. The `UNREACHABLE` macro prevents firing of `-Wreturn-type`/`C4715` warnings on some compilers. In the RPC code, the `NONFATAL_UNREACHABLE` macro might be more appropriate instead.
```

💬 hebasto commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#discussion_r1263779434)
Thanks! Updated.

👍 brunoerg approved a pull request: "fuzz: wallet, add target for `Crypter`"
(https://github.com/bitcoin/bitcoin/pull/28074#pullrequestreview-1530384275)
reACK e010fddf15a6d4ff84bfebdcb33c9c1d9cea7f0a

💬 ItIsOHM commented on pull request "rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1263790063)
Actually I wasn't sure if I should since I had removed the WIP tag and asked for a review, but I can change them right now if that's okay.

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