👍 dergoegge approved a pull request: "ci: Add missing -O2 to valgrind tasks"
(https://github.com/bitcoin/bitcoin/pull/28071#pullrequestreview-1528269852)
utACK fa4ccf15118a5e2dcd2a98283b9e6c7e1955a7f1

👍 dergoegge approved a pull request: "fuzz: Flatten all FUZZ_TARGET macros into one"
(https://github.com/bitcoin/bitcoin/pull/28065#pullrequestreview-1528293665)
Code review ACK fa6fa1f7b0120c13be0a515072065db82855b111

💬 dergoegge commented on pull request "fuzz: Flatten all FUZZ_TARGET macros into one":
(https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1262468302)
nit: I guess we could have `g_fuzz_targets` store the opts directly? Would mean less churn when adding an option.

💬 dergoegge commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1634145483)
Concept ACK

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262178924)
This check can be omitted from commit dc2361401c `net: move CreateSock() calls from ConnectNode() to netbase methods` since the code checks `if (!sock)` earlier. `CreateSock()` will never return a non-empty `unique_ptr` that contains a `Sock` object with a bogus internal file descriptor (`INVALID_SOCKED`).

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262322061)
The interface of taking `unique_ptr` as an argument and returning the same is a bit weird. Usually functions take `unique_ptr` when the ownership of the object is transferred from the caller to the function. But in this case it is transferred to the function which then returns it back to the caller. It would be better to have an interface like:

```cpp
bool ConnectToSocket(const Sock& sock, ...);

// and call it like:

sock = ... // is unique_ptr<Sock>
if (!ConnectToSocket(*sock, ...)) {
...

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262327319)
Make the string argument const reference and the function `static` since it is used only in `netbase.cpp`:

```suggestion
static std::unique_ptr<Sock> ConnectToSocket(std::unique_ptr<Sock> sock, struct sockaddr* sockaddr, socklen_t len, const std::string& dest_str, bool manual_connection)
```

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262368024)
There is no need to use the generic `sockaddr_storage` since we know it is going to be `sockaddr_un`. Also, clear it for compatibility, see the example in https://www.man7.org/linux/man-pages/man7/unix.7.html

```suggestion
struct sockaddr_un addrun;
memset(&addrun, 0, sizeof(addrun));
```

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262371026)
This and below I guess have to be inside `#if HAVE_SOCKADDR_UN`, otherwise it does not compile on Windows (CI has choked on it).

https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/ is relevant.

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262372825)
`strcpy()` is a dangerous function because it can be easily misused in a way that leads to security flaws. There is not a single usage of it in Bitcoin Core. In this particular case if `path` is longer it will buffer overflow the fixed size array `sun_path[]`.

I would suggest to do something like this:

```cpp
// leave the last char in addrun.sun_path[] to be always '\0'
memcpy(addrun.sun_path, path.c_str(), std::min(sizeof(addrun.sun_path) - 1, path.length()));
```

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262362898)
Calling this on an invalid `Proxy` (e.g. default-constructed) will cause weird results. Better check `if (!IsValid()) { log; return {}; }` at the start of this function.

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262417198)
The `len` calculation seems to be wrong since `sizeof(paddrun)` is just 8 bytes (it is a pointer). Anyway, you can use just `sizeof(sockaddr)` (not the pointer) or `sizeof(addrun)` if you change it to `sockaddr_un addrun;`

```suggestion
return ConnectToSocket(std::move(sock), (struct sockaddr*)&addrun, sizeof(addrun), path, /*manual_connection=*/true);
```

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262421705)
What is the meaning of this comment here?

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262436716)
The `Proxy::proxy` member is still accessed directly from some high level code:

```
src/net.cpp:494: std::make_unique<i2p::sam::Session>(proxy.proxy, &interruptNet);
src/qt/optionsdialog.cpp:416: ui->proxyReachIPv4->setChecked(has_proxy && proxy.proxy == ui_proxy);
src/qt/optionsdialog.cpp:419: ui->proxyReachIPv6->setChecked(has_proxy && proxy.proxy == ui_proxy);
src/qt/optionsdialog.cpp:422: ui->proxyReachTor->setChecked(has_proxy && proxy.proxy == ui_
...

📝 Lehonti opened a pull request: "Small refactoring of iterator type declaration"
(https://github.com/bitcoin/bitcoin/pull/28073)
There are a few functions in `httpserver.cpp` (`HTTPBindAddresses` and `UnregisterHTTPHandler`) that are declaring `std::vector::iterator` explicitly, which is really verbose and visually noisy. My guess is that they were written pre-C++11 and they would be written differently today, so I modified them a little and used `auto` instead.

Also, I inverted the logic of an `if` block in `UnregisterHTTPHandler` in order to remove a nesting level.

💬 dergoegge commented on pull request "[Silent Payments]: Base functionality":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1262495049)
Maybe make this a uint256&? Passing a raw pointer without a size (expecting it to be 32 byte) seems a little bug prone.
```suggestion
XOnlyPubKey AddTweak(const uint256& tweak32) const;
```

💬 dergoegge commented on pull request "[Silent Payments]: Base functionality":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1262496803)
Same as other suggestion
```suggestion
CKey AddTweak(const uint256& tweak32) const;

//! Tweak a secret key by multiplying it by a scalar value.
CKey MultiplyTweak(const uint256& tweak32) const;
```

💬 dergoegge commented on pull request "[Silent Payments]: Base functionality":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1262483480)
In my opinion, this flag doesn't belong here because only the receiver/sender of a silent payment would care about it, which would make it a piece of wallet meta-data.

Without knowing much about the wallet my guess would be that it should be stored on `CWalletTx`?

💬 MarcoFalke commented on pull request "Small refactoring of iterator type declaration":
(https://github.com/bitcoin/bitcoin/pull/28073#issuecomment-1634193795)
Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:

* Time spent on review
* Accidental introduction of bugs
* (Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer time or introduce bugs.

For more in
...

💬 MarcoFalke commented on pull request "Small refactoring of iterator type declaration":
(https://github.com/bitcoin/bitcoin/pull/28073#discussion_r1262519160)
Should probably use the C++17 structured bindings, but again, probably not worth to change it, unless there are other changes