💬 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

💬 MarcoFalke commented on pull request "Small refactoring of iterator type declaration":
(https://github.com/bitcoin/bitcoin/pull/28073#issuecomment-1634198701)
Closing for now. If you are looking for something else to work on, see:


## Getting started to contribute to Bitcoin Core

### Setting up your development environment

New developers are very welcome and needed. There are a lot of open issues of any difficulty waiting to be fixed. However, before you start contributing, familiarize yourself with the Bitcoin Core build system and tests. Refer to the documentation in the repository on how to build Bitcoin Core and how to run the unit and f
...

✅ MarcoFalke closed a pull request: "Small refactoring of iterator type declaration"
(https://github.com/bitcoin/bitcoin/pull/28073)

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1634208153)
> I find it slightly unsatisfying that we can't break any 8-byte repetitive pattern.

I just find it difficult to think about without a use-case. The only thing it may affect is that it makes on-disk compression harder, but 8-byte XOR already makes that hard, so I don't think this is an argument (for or against) either?

💬 fanquake commented on issue "valgrind: Syscall param ppoll(ufds.events) points to uninitialised byte(s)":
(https://github.com/bitcoin/bitcoin/issues/28072#issuecomment-1634233433)
Opened a bug report with Valgrind: https://bugs.kde.org/show_bug.cgi?id=472219

👍 theStack approved a pull request: "Make poly1305 support incremental computation + modernize"
(https://github.com/bitcoin/bitcoin/pull/27993#pullrequestreview-1528432509)
ACK 4e5c933f6a40c07d1c68115f7979b89a5b2ba580

💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#issuecomment-1634237988)
@MarcoFalke You may be interested in the `Span<std::byte>` usage here.