💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1262327787)
Might be better to just assert this is `1`, or omit, as otherwise it would lead to issues anyway?

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1262338435)
nit: Shouldn't this use ceil instead of floor?

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1262340617)
(See https://github.com/llvm/llvm-project/commit/104cd749f5cca609a79303c0dad22bc041b5448a )

Switched to `auto` in any case.

💬 chinggg commented on pull request "fuzz: Modify tx_pool_standard target to test package processing":
(https://github.com/bitcoin/bitcoin/pull/25778#issuecomment-1633957629)
I am not sure if the package relay features have been implemented. Feel free to reach out to me if there is need to continue the development of this fuzz target.

💬 dergoegge commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1633985978)
Looks like this is implicitly doing the same as #28071? from the qa-assets test run: https://cirrus-ci.com/task/4623075795271680?logs=ci#L1952-L1955

```
Options used to compile and link:
external signer = no
multiprocess = no
with experimental syscall sandbox support = no
with libs = no
with wallet = yes
with sqlite = yes
with bdb = no
with gui / qt = no
with zmq = no
with test = not building test_bitcoin because fuzzing is
...

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