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

📝 maflcko opened a pull request: "rpc: Remove deprecated dummy alias for listtransactions::label"
(https://github.com/bitcoin/bitcoin/pull/31413)
The RPC arg is not a dummy, but a label, so offering an undocumented alias is confusing at best, if not entirely unused.

Fix it by removing the deprecated alias.

💬 bigspider commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2514970384)
Thanks @Sjors for testing the app!

I just updated it with a new version (`Bitcoin Test Musig v2.4.0-rc`):
- Some bug fixes for more complex policies. Testing is still not extensive, but it should work for all combinations of musig and miniscript (with the due limits to the policy size; currently, at most 5 keys for MuSig2).
- If a psbt is sent where _only MuSig2 round 1_ is executed (no signatures returned), the app will return pubnonces _with no user interaction_. Note that if there are ot
...

🤔 hodlinator reviewed a pull request: "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2475934654)
Code reviewed f2fd1f7c043a2782cb2bf3c9fe7e2f94c17728b5

```
₿ git range-diff master 57caa96 f2fd1f7
₿ git show e314bb7e00 > old
₿ git show 91a8fde051 > new
₿ meld old new
```

Thanks for using more `constexpr` `std::array`s and clearer `sizeof`s!

Nice that block data could be compressed to such a large extent.

nit: Would prefer the *\*.cmake* changes where broken out into their own commit, keeping only *src/bench/CMakeLists.txt* as part of the benchmark change.

💬 hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1867876958)
nit: Clearer name and source of `sizeof` expression.
```suggestion
for (constexpr auto chunk_size = sizeof(key); write.size() >= chunk_size; write = write.subspan(chunk_size)) {
XorInt(write, key, chunk_size);
```

💬 hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1867892424)
Missing newline at EOF in latest version.