💬 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

⚠️ fanquake opened an issue: "cmake: searching across directories for config files"
(https://github.com/bitcoin/bitcoin/issues/32938)
This can be recreated on at least macOS, due to #32707.

If there are two checkouts of the source, the build in one directory, can fail to configure, because of files in the other directory:
```bash
git clone https://github.com/bitcoin/bitcoin/ bitcoin_a
git clone https://github.com/bitcoin/bitcoin/ bitcoin_b

cd bitcoin_a
make -C depends HOST=x86_64-w64-mingw32
# fails to build libevent (#32707)
cd ../bitcoin_b
# fails to configure, because of the libevent config files (for a different target)
...

💬 maflcko commented on issue "ci: don't pass GIT_EXEC_PATH inside test container":
(https://github.com/bitcoin/bitcoin/issues/32935#issuecomment-3057091584)
I think the possible solutions here are:

* Keep everything as-is and just let people figure out that they need to follow the docs exactly.
* Rename all env vars used internally in the ci to have some prefix like `CI_...` and then filter for those and only pass them. (easy-ish change, but still a breaking change)
* Wrap the ci invocation into an arg-parser (larger re-write, also a breaking change)
* something else?

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3057186819)
Updated 1a87ed000e09b10c173ba87d4bf2f79bf63abfa5 -> 2910315d1d8a3879f4e0bd50bf90870b642bfede ([`pr/mine.26`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.26) -> [`pr/mine.27`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.27), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.26..pr/mine.27)) fixing unused python import lint error https://cirrus-ci.com/task/5421333066022912

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3057188134)
ACK 2910315d1d8a3879f4e0bd50bf90870b642bfede

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3057258422)
[Latest push](https://github.com/bitcoin/bitcoin/compare/e036ba4ddaeb24a2361c356f7f270ee60bfff984..64fbca986cbaf3990d15d26bf6336fa8116f1d56) contains (as usual, the second push is just a changeless rebase):
* `Obfuscation` constructor only accepts fixed-size span now - and is explicit
* fuzz test changes were moved earlier to fix per-commit tests and we're using `ConsumeFixedLengthByteVector` now instead of either a uint64_t or random length vector
* `Obfuscation::SIZE_BYTES` was originally n
...

🚀 fanquake merged a pull request: "bench: Avoid tmp files in pwd"
(https://github.com/bitcoin/bitcoin/pull/32890)

👍 Sjors approved a pull request: "ipc: Handle unclean shutdowns better"
(https://github.com/bitcoin/bitcoin/pull/32345#pullrequestreview-3005455760)
utACK 0a9a1940782fad8dc3494c4aacee0a06c030eb5c

💬 Sjors commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2197581718)
In f8fd3959d51f6f80b428828c4fd965e06a8fa19e _ipc: Use EventLoopRef instead of addClient/removeClient_: if you have to retouch it would be good to document `m_loop` and `m_loop_ref`.

📝 l0rinc opened a pull request: "refactor: avoid double hashing in `SourceLocationHasher`"
(https://github.com/bitcoin/bitcoin/pull/32939)
Feeds the file name string directly into the hasher instead of pre-hashing it first. The resulting hash changes of course, but that doesn't affect us since it's only stored in-memory.

Fixes: https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2188644325

A few other related nits were also addressed here:
* Comment was redundant, `CSipHasher` is used in other hashers all over the codebase
* `static_cast<size_t>` skewed the whole line, changed to shorter functional-style cast
* struc
...