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

💬 sipa commented on pull request "fuzz: Flatten all FUZZ_TARGET macros into one":
(https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1259833732)
I believe that before C++20 this actually requires at least one argument after name, technically speaking (it's been a widely-adopted extension to allow that, however).

💬 MarcoFalke commented on pull request "fuzz: Flatten all FUZZ_TARGET macros into one":
(https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1259840541)
Currently all call sites do pass at least one token. However, https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1259825732 wouldn't, I guess.

💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1259844908)
Fixed. At some point I decided to eliminate the constant entirely, but then went back to trying to match the original implementation as much as possible.

💬 dergoegge commented on pull request "fuzz: Flatten all FUZZ_TARGET macros into one":
(https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1259868745)
Yes, I think that'd be nice, but also don't have a super strong preference

💬 sipa commented on pull request "fuzz: Flatten all FUZZ_TARGET macros into one":
(https://github.com/bitcoin/bitcoin/pull/28065#discussion_r1259870822)
Oh, yes, I commented incorrectly. This was meant as a response to that comment.

💬 MarcoFalke commented on pull request "fuzz: Flatten all FUZZ_TARGET macros into one":
(https://github.com/bitcoin/bitcoin/pull/28065#issuecomment-1631008261)
Thanks for the comments. Fixed everything.