🚀 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

💬 brunoerg commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1259974561)
I think `--perl-regexp` won't work on MacOS.

See (MacOS 13.0 (M1)):
```sh
➜ bitcoin-core-dev git:(28066-marco) ✗ ./test/fuzz/test_runner.py corpus process_message -g
1 of 168 detected fuzz target(s) selected: process_message
Generating corpus to corpus
grep: unrecognized option `--perl-regexp'
usage: grep [-abcdDEFGHhIiJLlMmnOopqRSsUVvwXxZz] [-A num] [-B num] [-C[num]]
[-e pattern] [-f file] [--binary-files=value] [--color=when]
[--context[=num]] [--directories=action
...

🤔 vasild reviewed a pull request: "net: disconnect inside AttemptToEvictConnection"
(https://github.com/bitcoin/bitcoin/pull/27912#pullrequestreview-1524221824)
I am not sure yet what would be the best approach to resolve the issues below.

One way would be to hold `m_nodes_disconnected_mutex` for the entire iteration of the `m_nodes_disconnected` list. But this would mean to call `DeleteNode()` and thus `PeerManagerImpl::FinalizeNode()` under that mutex. The latter locks `cs_main` :-(