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

💬 sipa commented on pull request "[WIP] descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#discussion_r1260003229)
Coding style nit: if you have more than a single line `if` statement, you must use braces and indentation.

💬 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-1631222345)
> On the other hand, maybe we should just permit `addr()` and `raw()` inside sh/wsh/tr. There is discussion about that in #24114 too.

Nice, I was asking myself the same question.
I actually started implementing this as a custom, sort of dummy, pubkey provider which had no knowledge of its keys, only containing the key ids, inside the `PKHDescriptor`. So it mapped to the original `sh(pkh(key_id))`. But.. I ended up preferring this approach for the "controversy" that the other one could had.

...

💬 MarcoFalke commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#discussion_r1260062800)
thx, fixed

🤔 mzumsande reviewed a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1524806674)
Concept ACK

💬 mzumsande commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1260065112)
Just wanted to note that this proposal has some history, see Issue #13477 and the two closed PRs: #13490 and #13713.

In particular, comment https://github.com/bitcoin/bitcoin/pull/13490#issuecomment-398069237 still seems relevant:
The expectation that -stopatheight stops exactly at the specified height relies on `ActivateBestChainStep()` only connecting a single block in each invocation, which is currently the case, but more
of a coincidence due to not wanting to lock `cs_main` for too lon
...

🤔 MarcoFalke reviewed a pull request: "Add support for "partial" fuzzers that indicate usefulness"
(https://github.com/bitcoin/bitcoin/pull/27552#pullrequestreview-1524835460)
lgtm, but would be good to test this before merge

💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260091062)
I think this is intentional to collect fuzz inputs that fail `SanityCheckASMap` into the qa-assets directory.

💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260083660)
This will discard all cases where `miniscript::FromScript` fails? This seems undesirable, because then someone can change the code to add undefined behavior or a crash in code paths that return an error.

💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260084752)
Same?

💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260089383)
style nit: May be better to use a switch-case to avoid missing a case, when a new value is added (unlikely)?

💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260087559)
```suggestion
UNINTERESTING,
```

Style nit: Missing comma to avoid having to touch this line again if a new value is added (unlikely).