💬 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` :-(

💬 vasild commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#discussion_r1259711622)
This can end up with a double-free if two threads concurrently execute it.

1. Thread1 makes a copy of `m_nodes_disconnected` and releases `m_nodes_disconnected_mutex`
2. Thread2 does the same
3. Thread1 starts iterating on its own copy and calls `DeleteDisconnectedNode()` on the first element which calls `DeleteNode()` which calls `delete pnode;`
4. Thread2 does the same on its own copy, a second `delete` on the same `CNode` object.

💬 vasild commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#discussion_r1259704670)
This need not be `RecursiveMutex`?

```suggestion
mutable Mutex m_nodes_disconnected_mutex;
```

💬 vasild commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#discussion_r1259898235)
This could be called concurrently by two threads for the same `CNode`.

💬 vasild commented on pull request "net: disconnect inside AttemptToEvictConnection":
(https://github.com/bitcoin/bitcoin/pull/27912#discussion_r1259736855)
Same double-free as above (even though when this code in `StopNodes()` is executed, then the other threads that could access `m_nodes_disconnected` should have been exited by `StopThreads()` already, but better not rely on that).

💬 furszy commented on pull request "[WIP] descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1631128450)
> > 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, does it?

Yeah. Without the fix commit, the migration process in the test will pass "successfully" and then, at the wallet restart verification, the wallet loading process will throw the "Unrecognized descriptor found" error. Which denotes that t
...

💬 sipa commented on pull request "[WIP] descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1631134852)
This is clearly an issue; we should not be inferring descriptors that the code itself doesn't accept back.

On the other hand, maybe we should just permit `addr()` and `raw()` inside sh/wsh/tr. There is discussion about that in #24114 too.

🚀 ryanofsky merged a pull request: "test: indexes, fix on error infinite loop"
(https://github.com/bitcoin/bitcoin/pull/28044)