⚠️ cyjseagull opened an issue: "Why bitcoin use so many RecursiveMutex"
(https://github.com/bitcoin/bitcoin/issues/32934)
### Please describe the feature you'd like to see added.

I have noticed that in the Bitcoin source code, almost all thread-safe sections use `RecursiveGuard`.
As we all know, `RecursiveGuard` is **exclusive**, which can **impact system performance and concurrency**.

At the same time, I have also noticed [another issue](https://github.com/bitcoin/bitcoin/issues/19303) specifically tracking the replacement of `RecursiveMutex` with `Mutex`, which would help reduce some of the locking overhead.


...

👍 fanquake approved a pull request: "test: Add missing convert_to_json_for_cli"
(https://github.com/bitcoin/bitcoin/pull/32932#pullrequestreview-3004869160)
ACK fa0528479d5e37833fa66395c94d4611aa9270f6

🚀 fanquake merged a pull request: "test: Add missing convert_to_json_for_cli"
(https://github.com/bitcoin/bitcoin/pull/32932)

💬 fanquake commented on pull request "qa: Avoid knock-on exception in assert_start_raises_init_error":
(https://github.com/bitcoin/bitcoin/pull/32929#issuecomment-3056708522)
(Test failure here was fixed in #32932)

💬 maflcko commented on pull request "clang-format: align brace-after-struct and *-class formatting":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2197233259)
in 7c9b3e1eae8f206753457149f1b1c837f6627d6d: How to reproduce the commit on non-macos? I don't have an apple, so I tried:

```
# clang-format-16 --version
Ubuntu clang-format version 16.0.6 (23ubuntu4)
```

However, the command:

```
clang-format-16 -dump-config > .clang-format
```

produces a different result:

```diff
# git diff -U0
diff --git a/src/.clang-format b/src/.clang-format
index 2f3f96ae2e..0822db5d11 100644
--- a/src/.clang-format
+++ b/src/.clang-format
@@ -0
...

⚠️ olegrok opened an issue: "ci: don't pass GIT_EXEC_PATH inside test container"
(https://github.com/bitcoin/bitcoin/issues/32935)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

https://github.com/bitcoin/bitcoin/blob/83ae7802fe14af7eed9edd830f51a3269cc2bd23/ci/test/02_run_container.sh#L15C22-L15C24

If host system is not ubuntu it could set GIT_EXEC_PATH to /usr/libexec/git-core (it should be /usr/lib/git-core) that causes `git: 'remote-https' is not a git command. See 'git --help'.` error.

You need to change it to something like:
```bash
python3 -c '
import o
...

💬 l0rinc commented on pull request "clang-format: align brace-after-struct and *-class formatting":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2197286287)
> How to reproduce the commit on non-macos

Please see the commit messages - should I move the instructions to the PR description as well?

> produces a different result

I think you have dumped the defaults that don't load our config first, which are indeed different from what we have (hence our own config).
```bash
$ clang-format-16 --version
Ubuntu clang-format version 16.0.6 (23ubuntu4)
$ clang-format-16 -dump-config | grep AfterClass
AfterClass: false
$ clang-format-16 -d
...

💬 maflcko commented on issue "ci: don't pass GIT_EXEC_PATH inside test container":
(https://github.com/bitcoin/bitcoin/issues/32935#issuecomment-3056903564)
> ### Steps to reproduce
>
> Run CI script on e.g. awslinux and try to clone something inside build scripts

You'll have to include the full and exact steps to reproduce.

Did you use https://github.com/bitcoin/bitcoin/blob/c4f90900b55f1a77fa68b627f73a494fab0ed98d/ci/README.md#L46 ?



> exclude

I don't think a list with excludes is maintainable. It is never going to be complete anyway. (see e.g. https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586828644, or any linux system that s
...

⚠️ hebasto opened an issue: "SegFault in `coinstatsindex_tests`"
(https://github.com/bitcoin/bitcoin/issues/32936)
System NetBSD 10.1, GCC 14.2.0, using system packages:
```
29/141 Test #32: coinstatsindex_tests .................***Exception: SegFault 1.20 sec
```

Full log is available [here](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/16185521919/job/45690339085).

💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2197391680)
> So, I think, the error should say: "Both blockhashes and descriptors arrays must be specified", and the `.empty()` check should be removed for both.

I think I agree with that. It seems that the RPC works fine (did not properly inspect the call graph/code) with empty blockhashes and descriptors (modulo not calling `.get_array()`):

```
% cli getdescriptoractivity '[]' '[]'
{
"activity": [
]
}
```

I don't think we need to protect the user against providing empty arrays if it is
...

💬 maflcko commented on pull request "clang-format: align brace-after-struct and *-class formatting":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2197393675)
> `clang-format-16 -dump-config > .clang-format`

to answer my own question, I guess redirecting `>` is the problem here. It works fine when redirecting to a tmp file first: `( clang-format-16 -dump-config > tmp ) && mv tmp ./.clang-format`

📝 fanquake opened a pull request: "Enable `-Werror=dev` in CI & Guix"
(https://github.com/bitcoin/bitcoin/pull/32937)
Pass `-Werror=dev` in the CI, Guix and the `dev-mode` preset.

https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-Werror:
> Make developer warnings errors.
> Make warnings that are meant for the author of the CMakeLists.txt files errors. By default this will also turn on deprecated warnings as errors.

Pulled out of #32865.

👍 maflcko approved a pull request: "clang-format: align brace-after-struct and *-class formatting"
(https://github.com/bitcoin/bitcoin/pull/32813#pullrequestreview-3005160856)
I haven't checked if this fixes the issues with your IDE, but I guess it makes sense either way to get the clang-format config up to date. clang-16 is the minimum required for about a year now, so every developer should have access to clang-format-16 in some way or another.

I've recreated the commits on Ubuntu and reviewed the diff:

review ACK 94364a7d5403d9d480ebc065f8709c6dd21e543f 🥙

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secr
...

💬 hebasto commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2197410784)
> > Reintroducing this block + your patch will only do something if we also reintroduce the custom warning handling (`configure_warnings`
>
> Yes, that was my suggestion: Replace the commit with a trivial one-line patch.

I support @maflcko's suggestion.

💬 hebasto commented on pull request "Enable `-Werror=dev` in CI & Guix":
(https://github.com/bitcoin/bitcoin/pull/32937#issuecomment-3056995951)
Concept ACK.

💬 hebasto commented on issue "SegFault in `coinstatsindex_tests`":
(https://github.com/bitcoin/bitcoin/issues/32936#issuecomment-3057030276)
The issue seems intermittent. Maybe an OOM in the container.

💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2197447487)
"adding tech debt" was probably too strong of a term, because yes just a different function signature would address my immediate concerns, even if it looks like this functionality can be streamlined in future PRs.

```cpp
/**
* Ensures exactly one wallet name is specified across the endpoint and wallet_arg. Throws
* `RPC_INVALID_PARAMETER` if no or multiple wallet names are specified.
*/
std::string EnsureUniqueWalletName(const JSONRPCRequest& request, const std::string* wallet_arg);
...

💬 olegrok commented on issue "ci: don't pass GIT_EXEC_PATH inside test container":
(https://github.com/bitcoin/bitcoin/issues/32935#issuecomment-3057060249)
> Did you use?

No. I hoped that everything will works fine out-of-box. So here I just highlight possible issue. It was really hard to debug it.

> I don't think a list with excludes is maintainable.

That's true. So feel free to close this issue if you don't think anything actually can't be improved here.

💬 maflcko commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2197460325)
Yeah, thanks @stickies-v. Sorry for missing this. Obviously commit 529d2ce24570aae426ce19e87cb22f124dd09470 is not quite right. It should either be dropped, because there isn't really any problem, or the already existing RPCArg::Optional logic should be used.

👍 stickies-v approved a pull request: "log: Properly log warnings with warn loglevel in addrdb"
(https://github.com/bitcoin/bitcoin/pull/32933#pullrequestreview-3005268716)
ACK fa894b0f3e13dcc55fd42cec2c56d4aa2115194d