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

🤔 l0rinc requested changes to a pull request: "refactor: covert ContainsNoNUL to ContainsNUL"
(https://github.com/bitcoin/bitcoin/pull/31301#pullrequestreview-2440971933)
While [refactor-only PRs are often frowned upon](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md?plain=1#L61) (since the benefit is small, but the risk is big), I think this is small enoughfor us to consider.

You could simplify further by changing the occurrences via https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#scripted-diffs (making sure all commit compile)

Note that the `utxo_snapshot.cpp(45,1): error C1001: Internal compiler error` seems un
...

💬 l0rinc commented on pull request "refactor: covert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1845411957)
We should be able to simplify further:
```suggestion
return str.find('\0') != std::string_view::npos;
```

💬 l0rinc commented on pull request "refactor: covert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1845412066)
`[[nodiscard]]` doesn't make a lot of sense here, since the method doesn't have any side-effect

💬 l0rinc commented on pull request "refactor: spanify DecodeBits, use constexpr std::array instead of vector":
(https://github.com/bitcoin/bitcoin/pull/31302#issuecomment-2481195565)
approach NACK, we should only modify this when the affected area is already changed and tested well enough (+ whole build is red, also indicating this wasn't the case). The risk of breaking something is a lot greater while the benefit is likely very small. These should usually be bundled with other related changes, having a concrete purpose (e.g. tests are covering something that cannot happen in reality, new feature being introduced, result is undeniably safer code, etc).

⚠️ mraksoll4 opened an issue: "Bitcoin 28.0 ver. Compiler bug at fuzz when compiling using vs 2022."
(https://github.com/bitcoin/bitcoin/issues/31304)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

compile bug at fuzz , and warning about wildcards.

### Expected behaviour

i exepect success build , only fuzz problem.

### Steps to reproduce

i use this docs

https://github.com/bitcoin/bitcoin/tree/v28.0/build_msvc

and use

Microsoft Visual Studio Community 2022 (64-bit) - Current
Version 17.12.0


### Relevant log output

```
H:\bitcoin-28.0\src\test\fuzz\utxo_snapshot.cpp
...

💬 l0rinc commented on issue "Bitcoin 28.0 ver. Compiler bug at fuzz when compiling using vs 2022.":
(https://github.com/bitcoin/bitcoin/issues/31304#issuecomment-2481232758)
Seems like a duplicate of https://github.com/bitcoin/bitcoin/issues/31303