💬 Eunovo commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#issuecomment-2480940195)
> > I believe you can get more fine-grained benchmarks for this using the `rpc_blockchain` benchmarks.
>
> Yes, I can, but I’ll need to add verbosity levels 1 and 2, where we can see some performance improvements.
>
> Show diff
> Benchmark Results
>
> On **master**:
>
> ns/op op/s err% total benchmark
> 190,342 5,254 2.3% 0.01 `BlockToJsonVerbose1`
> 34,812,292 28.73 1.0% 0.38 `BlockToJsonVerbose2`
> 34,457,167 29.02 1.0% 0.38 `BlockToJsonVerbose3`
> On **this PR**:
>
> ns/op
...

📝 PastaPastaPasta opened a pull request: "refactor: spanify DecodeBits, use constexpr std::array instead of vector"
(https://github.com/bitcoin/bitcoin/pull/31302)
Should generally prefer using std::span over const std::vector&, and generally should use constexpr std::array instead of the const std::vector as used here. This allows the interface to be more flexible (thought the value is low here) and allows it to be more constexpr.

Brief investigation was made into spanifing other interfaces in this file; however, the usage of the specialization std::vector<bool> made that incompatible with trivial spanification

💬 TheCharlatan commented on pull request "Add `contrib/justfile` containing useful development workflow commands.":
(https://github.com/bitcoin/bitcoin/pull/31292#discussion_r1845340819)
We already have the cmake presets for making it a bit easier to get started and the user can bring their own `CmakeUserPresets.json`. It is seldom that I am still typing out a full cmake command.

💬 hebasto commented on pull request "depends: Update minimum required CMake version":
(https://github.com/bitcoin/bitcoin/pull/31300#issuecomment-2481070101)
My Guix build:
```
x86_64
bb75dc6ee5cde4b85294a4c61948e4ae1d5ef5d011e671b494333f207c07c774 guix-build-b5a12350faf0/output/aarch64-linux-gnu/SHA256SUMS.part
6bc0af6d7d9c76ef1c65ba631bdcd84f482f30ec3961460dfaf0e2721843f802 guix-build-b5a12350faf0/output/aarch64-linux-gnu/bitcoin-b5a12350faf0-aarch64-linux-gnu-debug.tar.gz
fa27f50aa27fd5a95200340612515a79e17df57cc65faceeeb0df4f31e5d4592 guix-build-b5a12350faf0/output/aarch64-linux-gnu/bitcoin-b5a12350faf0-aarch64-linux-gnu.tar.gz
c79b56bc5
...

⚠️ hebasto opened an issue: "MSVC 17.12.0 internal compiler error "
(https://github.com/bitcoin/bitcoin/issues/31303)
The [latest version](https://learn.microsoft.com/en-us/visualstudio/releases/2022/release-notes#17.12.0) of MSVC causes an internal compiler error in `src/test/fuzz/utxo_snapshot.cpp` for both "Release" and "Debug" configurations:
```
< snip >
utxo_snapshot.cpp
C:\Users\hebasto\source\repos\bitcoin\src\test\fuzz\utxo_snapshot.cpp(45,1): error C1001: Internal compiler error. [C:\Users\hebasto\source\repos\bitcoin\build-static\src\test\fuzz\fuzz.vcxproj]
C:\Users\hebasto\source\repos\bitcoi
...

💬 flack commented on pull request "refactor: covert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1845405810)
I guess this should be updated, too

💬 hebasto commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2481165163)
cc @sipsorcery @davidgumberg @hodlinator

💬 maflcko commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2481173636)
It would be good to report this upstream, as requested in the error message. Possibly with a minimal reproducer, if someone on Windows can reduce it further.

💬 maflcko commented on pull request "depends: Update minimum required CMake version":
(https://github.com/bitcoin/bitcoin/pull/31300#issuecomment-2481174794)
Is there a reason to pick 3.10, when someone couldn't compile Bitcoin Core later on anyway with that version? See https://github.com/bitcoin/bitcoin/blob/ccc2d3abcd39c64a78e366f3e4794de729155e9e/CMakeLists.txt#L10

Also, according to https://github.com/bitcoin/bitcoin/issues/31299#issuecomment-2480631513 3.13 may be required anyway?

💬 pinheadmz commented on pull request "refactor: spanify DecodeBits, use constexpr std::array instead of vector":
(https://github.com/bitcoin/bitcoin/pull/31302#issuecomment-2481186853)
I think we try to avoid unnecessary refactors for "general preference". If there's no performance improvement with these changes it's likely to get ignored by reviewers.

👍 l0rinc approved a pull request: "refactor: Clean up messy strformat and bilingual_str usages"
(https://github.com/bitcoin/bitcoin/pull/31072#pullrequestreview-2440916482)
ACK 85df2fbf26c73f97f85797868155247c11a4ccd6

I have basically redone the change locally to understand it better and mostly left questions or nits, I'm fine with merging as is

💬 l0rinc commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1845404225)
I think I'm missing some important detail here, but I see that `#define COPYRIGHT_HOLDERS_FINAL "The Bitcoin Core developers"` is basically `#define COPYRIGHT_HOLDERS "The %s developers"` substituted into `#define COPYRIGHT_HOLDERS_SUBSTITUTION "Bitcoin Core"`, could we maybe simplify to
```C++
const auto copyright_devs = _(COPYRIGHT_HOLDERS_FINAL).translated;
```
or would that prevent translation?

💬 l0rinc commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1845361924)
nit: it seems to me both start with str and optionally extend with the details, if available.
Maybe we could tell this story with the code as well:
```C++
auto suffix = details.empty() ? bilingual_str{} : Untranslated(strprintf(":\n%s", MakeUnorderedList(details)));
return InitError(str + suffix);
```

💬 l0rinc commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1845399972)
I don't fully understand why this couldn't be an `Untranslated(strprintf` (is it because `bilingual_str user_error` doesn't have an `std::ostream` operator defined?), but if you touch again, consider simplifying to `+ ErrorString(rename_result)` (or extending `Result` with an error returning method as well, but that's likely outside the scope of this PR), i.e.
```C++
if (auto rename_result = m_snapshot_chainstate->InvalidateCoinsDBOnDisk(); !rename_result) {
user_error += Untranslated("\n
...

💬 l0rinc commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1845406983)
👍, it's obvious that the `Section` name is `m_name`, not `m_file`.

nit: it seems to me in other cases we're using `%d` instead of `%i` (if you edit again, consider simplifying to `%d` which seems to have fewer surprises): https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L986

💬 l0rinc commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1845409666)
what is the reason for these still being false positives?

💬 l0rinc commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1845404992)
👍, a lot cleaner this way

💬 l0rinc commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1845409859)
nit: it's simple enough to review, but if you edit again consider a scripted diff

💬 maflcko commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#issuecomment-2481187627)
> > Considering that CI is failing immediately after a walletprocesspsbt call, and that RPC is being changed in this PR, I'm inclined to think that it's probably not a CI issue.
>
> Yes, there was a new issue introduced by the added commits too. But there was also a CI issue: it failed to merge into master before building.

Not sure what failure you are referring to, but looking at https://github.com/bitcoin/bitcoin/actions/runs/11875971339/job/33093790745, it looks like the problem was tha
...

💬 maflcko commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1845412111)
I don't think you'd want to translate the name of the program? For reference, I've also split up the common refactor commits into https://github.com/bitcoin/bitcoin/pull/31295, as they are needed by two pull requests.

💬 maflcko commented on pull request "wallet: Translate [default wallet] string in progress messages":
(https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2481190404)
What are the steps to test this with sqlite? If it isn't needed after the bdb removal, it can probably be skipped?