💬 willcl-ark commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1630664257)
It's a tNACK from me currently @ c3b0f137a9 as I can no longer get this functionality to work in practice on Ubuntu 23.04 with Tor 0.4.7.13.

The send of the data in `vSocks5Init` seems to be failing, meaning the proxy cannot be used: https://github.com/pinheadmz/bitcoin/blob/c3b0f137a9ccd894f2f1d287ccdab26310b3aaf2/src/netbase.cpp#L372-L375

I haven't yet totally ruled out issues with Tor, but I do get responses from the unix socket about it not being an HTTP proxy if I try to send an HTTP
...

💬 recursive-rat4 commented on issue "Creating a Wallet Feature Guidelines Document":
(https://github.com/bitcoin/bitcoin/issues/28062#issuecomment-1630685387)
I'd rather see it more useful with an opposite list of what is in scope, for example:
- The Wallet should improve an user experience when that does not compromise the security
- The Wallet must use the Bitcoin P2P network when an interaction with the outside world is desired
- The Wallet should implement new features that got standardized as BIPs

💬 naumenkogs commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1630685413)
Approach ACK

📝 MarcoFalke opened a pull request: "fuzz: Flatten all FUZZ_TARGET macros into one"
(https://github.com/bitcoin/bitcoin/pull/28065)
The `FUZZ_TARGET` macros have many issues:
* The developer will have to pick the right macro to pass the wanted option.
* Adding a new option requires doubling the number of existing macros in the worst case.

Fix all issues by using only a single macro.

This refactor does not change behavior.

💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1259659543)
nit: (This is my fault)

Not really a fan adding more macros, where each new option will cause doubling all existing macros. Currently there are 3, in this pull there are 6, and with the next option we'll have 12 to 16 macros?

At least for the existing options, which only need to be known at runtime, an options struct can be used.

See https://github.com/bitcoin/bitcoin/pull/28065 . Feel free to ignore/NACK.

💬 willcl-ark commented on issue "Bitcoin core hidden services link down/doesnt work":
(https://github.com/bitcoin/bitcoin/issues/28054#issuecomment-1630813890)
Thanks for the report @TNTBOMBOM.

I am going to close this issue here as it's more appropriate for the bitcoincore.org repo, and I've taken the liberty of opening an equivalent issue over there on your behalf: https://github.com/bitcoin-core/bitcoincore.org/issues/985

✅ willcl-ark closed an issue: "Bitcoin core hidden services link down/doesnt work"
(https://github.com/bitcoin/bitcoin/issues/28054)

👍 ryanofsky approved a pull request: "refactor: Move stopafterblockimport option out of blockstorage"
(https://github.com/bitcoin/bitcoin/pull/28053#pullrequestreview-1524282976)
Code review ACK 462390c85f1174b3cf83dfb0f74922049e5cb8af. Just has been rebased and is a simpler change after #27607

💬 willcl-ark commented on pull request "init: adding check for : for -torcontrol flag":
(https://github.com/bitcoin/bitcoin/pull/28018#issuecomment-1630852725)
Unrelated to the implementation here, but in general I disagree with most of Luke's comment:

> Port isn't required (the default is 9051).

The way this is currently documented, both host and port should be required:

```

The option is `torcontrol` and should require `host::port` if specified. If we used independant options `torcontrolhost` and `torcontrolport` then we could reasonably have defaults for both, and make both optional, but having _half of a single option_ be optional seem
...

✅ willcl-ark closed a pull request: "init: adding check for : for -torcontrol flag"
(https://github.com/bitcoin/bitcoin/pull/28018)

📝 willcl-ark reopened a pull request: "init: adding check for : for -torcontrol flag"
(https://github.com/bitcoin/bitcoin/pull/28018)
right now for -torcontrol you can pass in any string as long as it doesn't have a : but that is invalid. I added an additional check before calling SplitHostPort to see if there is a :


closes https://github.com/bitcoin/bitcoin/issues/23589
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
firs
...

💬 sipa commented on pull request "fuzz: Flatten all FUZZ_TARGET macros into one":
(https://github.com/bitcoin/bitcoin/pull/28065#issuecomment-1630859123)
Concept ACK

💬 willcl-ark commented on pull request "init: adding check for : for -torcontrol flag":
(https://github.com/bitcoin/bitcoin/pull/28018#issuecomment-1630861898)
Unrelated to the actual implementation in this PR, but I disagree with some of Luke's points:

> Port isn't required (the default is 9051).

The way this is currently documented, both host and port should be required. However, it does align with many of our other options to have a default port, and if that makes sense here too then the help for `torcontrol` arg should be updated to show port as optional as a first step.

> Furthermore, even "1" is a valid value (it would be the same as 0
...

🚀 ryanofsky merged a pull request: "refactor: Move stopafterblockimport option out of blockstorage"
(https://github.com/bitcoin/bitcoin/pull/28053)

💬 theStack commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1259760752)
magic number elimination nit:
```suggestion
while (bytes >= POLY1305_BLOCK_SIZE) {
```

🤔 theStack reviewed a pull request: "Make poly1305 support incremental computation + modernize"
(https://github.com/bitcoin/bitcoin/pull/27993#pullrequestreview-1524312375)
Concept ACK

Verified that the poly1305 implementation introduced in 4abc8465f74e390e0f887a5a0be9550d3d179791 matches poly1305-donna (used https://github.com/openbsd/src/blob/master/sys/crypto/poly1305.c as a reference, which is again based on Andrew Moon's implementation with minor adaptions). Still planning to verify the test vectors with another implementation, likely PyCryptodome.

🤔 dergoegge reviewed a pull request: "fuzz: Flatten all FUZZ_TARGET macros into one"
(https://github.com/bitcoin/bitcoin/pull/28065#pullrequestreview-1524385898)
Concept ACK

> Adding a new option requires doubling the number of existing macros in the worst case.

I've run into this before and it is indeed pretty annoying.

💬 dergoegge commented on pull request "fuzz: Flatten all FUZZ_TARGET macros into one":
(https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1259806682)
nit: would be cool if we could avoid these empty braces somehow

📝 MarcoFalke opened a pull request: " fuzz: Generate process_message targets individually "
(https://github.com/bitcoin/bitcoin/pull/28066)
Now that `LIMIT_TO_MESSAGE_TYPE` is a runtime setting after commit 927b001502a74a7224f04cfe6ffddc9a59409ba1, it shouldn't hurt to also generate each message type individually. Something similar was done for the `rpc` target in commit cf4da5ec29f9e8cd6cc6577e5ecbd87174edba62.

💬 MarcoFalke commented on pull request "fuzz: Flatten all FUZZ_TARGET macros into one":
(https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1259825732)
Yes, it is possible. For example:

```diff
diff --git a/src/test/fuzz/blockfilter.cpp b/src/test/fuzz/blockfilter.cpp
index aa06af549a..3adc114515 100644
--- a/src/test/fuzz/blockfilter.cpp
+++ b/src/test/fuzz/blockfilter.cpp
@@ -12,7 +12,7 @@
#include <string>
#include <vector>

-FUZZ_TARGET(blockfilter, {})
+FUZZ_TARGET(blockfilter)
{
FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
const std::optional<BlockFilter> block_filter = ConsumeDeseri
...