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

📝 furszy opened a pull request: "[WIP] descriptors: do not return top-level only funcs as sub descriptors"
(https://github.com/bitcoin/bitcoin/pull/28067)
Linked to #28057.
Under WIP because I'm still exploring the topic. But pushing it to gather more opinions and insights.

Currently, the `InferScript` function returns an invalid descriptor when it tries to infer a p2sh-p2pkh script whose pubkey is not known by the wallet.

This behavior occurs because the inference process bypasses the `pkh` subscript when the pubkey is not contained by the wallet (no pubkey provider), interpreting it as a `sh(addr(ADDR))` descriptor. Then, the failure aris
...

💬 MarcoFalke commented on pull request "[WIP] descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1631097368)
> For reviewers, would recommend to start by examining the functional test to understand the context and the circumstances on which this can result in a fatal error (e.g. during the migration process).

The added functional test doesn't crash, no?

👍 jamesob approved a pull request: "test: indexes, fix on error infinite loop"
(https://github.com/bitcoin/bitcoin/pull/28044#pullrequestreview-1524633487)
ACK 89ba890

Built/tested locally. Seems like a commonsense way to assert-fail instead of causing an infinite loop on CI machines.

💬 MarcoFalke commented on pull request "test: indexes, fix on error infinite loop":
(https://github.com/bitcoin/bitcoin/pull/28044#issuecomment-1631104387)
lgtm ACK 89ba8905f5c68ae29412f9c4010314c5a113c234

👍 ryanofsky approved a pull request: "test: indexes, fix on error infinite loop"
(https://github.com/bitcoin/bitcoin/pull/28044#pullrequestreview-1524643965)
Code review ACK 89ba8905f5c68ae29412f9c4010314c5a113c234. Just comment update since last review