💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1867871199)
Via `subprocess`?

👋 hebasto's pull request is ready for review: "qa: Fix `wallet_multiwallet.py`"
(https://github.com/bitcoin/bitcoin/pull/31410)

💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1867881135)
What's the advantage of the comment stating exactly what the code already states?
We're already testing `\0` and non-trailing `=`, what else do these ones add?

💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1867882293)
I'll consider it if I need to touch again

💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1867886473)
You're suggesting an empty line here only, right?
I'll consider it if I need to edit again

💬 ryanofsky commented on pull request "rpc: Remove deprecated RPC arg names":
(https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2514813001)
First commit dropping verbosity|verbose seems ok, but I think the ability for different arguments names to exist in the same position is useful for #24963. #24963 is an old PR but Luke just updated it 2 weeks ago and I acked it in
https://github.com/bitcoin/bitcoin/pull/24963#pullrequestreview-2443280039, describing the benefits I think it offers.

Do you think letting multiple arguments occupy the same position makes implementing #29912 harder? I would think an automatically generated schem
...

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1867894322)
```bash
grep "std::string" bitcoin/src/test/coins_tests.cpp | wc -l
3
```

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1867899670)
I prefer `const` here, my IDE was also asking for them, I don't mind adding them here.
Being `noisy` when `a function doesn't contain any logic` doesn't sound like a blocker to me :)

💬 edilmedeiros commented on pull request "rpc: Remove deprecated RPC arg names":
(https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2514827383)
Concept ACK.
This incrementally reduces complexity and is an easy fix for those that may have something broken because of it.


What I don't like is to have RPC methods with different parameter names doing the same thing.
For instance:
```
getblock "blockhash" ( verbosity )
getblockheader "blockhash" ( verbose )
```

Maybe it would be good to clean up the RPC API before fixing #29912 (which seems to be done in the foreseeable future)?

👍 ryanofsky approved a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2476055822)
Code review ACK 50cce20013c97e1257cb9f4c9319701a09b0a196. Looks good! Thanks for the followups.

Since last review, coins.h commits were reorganized to minimize diffs, but only overall change was to drop redundant `inline` keywords. In `coins_tests.cpp` a lot of smaller changes were made like adding `const` to amount parameters (which is not great but ok), using more brace initialization, changing CheckAddCoin parameter order, and fixing up some comments.

💬 hebasto commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2514900738)
> I use `uv`

[This](https://github.com/astral-sh/uv) one?

💬 willcl-ark commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2514904974)
Yes, and I highly recommend it!

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1867966922)
This issue is a non-nit for me as a general pattern as it will affect diffs & reviews going forward.

We've got a ratio of 5/212 or roughly 1:40 for this codebase. I'm surprised that the IDE is encouraging that.

What is you rationale for not applying it to `AddFlags`? Did the specific version of your IDE not tell you to do so? Does your IDE have a documented rationale?

If there is a clear rationale, ideally with a statistical sample of % bugs discovered from changing to `const` value typ
...

💬 mzumsande commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1867973096)
the state could already be invalid before entering the loop (BIP30 check a few lines above).

👍 ryanofsky approved a pull request: "util: Improve documentation and negation of args"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2476098921)
Code review ACK 95a0104f2e9869799db84add108ae8c57b56d360. Looks good! Thanks for all your work on this breaking the changes down and making them simple.

Since last review, more log newlines are removed, -norpccookiefile code is reorganized a little without changing behavior, combine_logs.py assert is improved, feature_config_args.py more consistently stops nodes after tests, and using -noconf now triggers an info log instead of a warning.

✅ maflcko closed a pull request: "rpc: Remove deprecated RPC arg names"
(https://github.com/bitcoin/bitcoin/pull/31412)

💬 maflcko commented on pull request "rpc: Remove deprecated RPC arg names":
(https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2514938972)
> Do you think letting multiple arguments occupy the same position makes implementing #29912 harder? I would think an automatically generated schema could just ignore deprecated aliases and only show the current non-deprecated way of calling the API.

I agree, but it is still annoying to have the for-loop bloat everywhere internally in this repo for no real use-case. If this is needed for https://github.com/bitcoin/bitcoin/pull/24963, then I guess there is a use-case and I can close it for now
...

💬 instagibbs commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1867984833)
glossed over that above change for bip30 checks, thanks

💬 maaku commented on issue "macOS App Notarization & Stapling":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2514948561)
I’m not sure how what you are explaining is in way different than the normal expected behavior? Code signing is done by the developer, and the notarization is done is done by Apple. I’ve been doing this third-party submission to Apple for a while now, so I can make stapled bitcoin binaries for my own use.No part of the security argument relies on the person submitting the app for authorization to Apple being the original developer, and Gatekeeper has always been a “trust Apple” solution.On Dec 3
...

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1867992733)
We can unify these in a separate PR, I don't mind, I added these since the linters were complaining - and I was fine with either option since it doesn't affect the body of the function signal-to-noise-ratio-wise.