💬 stickies-v commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793707636)
```suggestion
QMessageBox::information(m_parent_widget, tr("Restore wallet message"), QString::fromStdString("Wallet restored successfully \n"));
```

💬 stickies-v commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793744937)
This function is currently unused, and it seems incorrectly implemented as it doesn't compile when actually used:

```
In file included from ../../src/clientversion.cpp:9:
../../src/util/translation.h:42:48: error: too many arguments to function call, expected 0, have 1
std::string translate() { return translate(original.fmt); }
~~~~~~~~~ ^~~~~~~~~~~~
../../src/util/translation.h:106:42: note: in instantiation of member function 'util::bilingual_fm
...

💬 stickies-v commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793473436)
typo nit
```suggestion
/** Type to denote whether an original string literal is translatable */
```

💬 stickies-v commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793829614)
nit: I would prefer templating this on an enum `{Translateable,Untranslateable}` for readability over the non-obvious `<{true,false}>` boolean

💬 kevkevinpal commented on pull request "bench: Adds a benchmark for CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/29745#issuecomment-2402804271)
> Closing for now due to inactivity since the day it was opened. Leave a comment below if you want this re-opened, or open a new pull.

ya sorry about that I can leave it up for grabs if anyone else wants to look at it otherwise I can reopen when I have a more finalized PR open, again sorry for leaving it open but inactive

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402903171)
@Sjors

I cannot reproduce the issue on macOS Intel w/o Xcode you mentioned in:
- https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388568119
- https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2396235386

On my MacBook Pro (2019, Intel) + macOS 15.0.1, no Xcode, I can successfully build 86837bc75c4c1282fc652dfd755273db956222db as follows:
```
% brew list
==> Formulae
cmake ninja pkg-config

==> Casks
% make -j 16 -C depends NO_WALLET=1 NO_UPNP=1 NO_ZMQ=1
% cm
...

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402950941)
> They'd have to be cleared on all machines. Let me known if you want that to happen.

At some point, we may have to do this anyway.

> If you are just looking for a temporary workaround for testing, you can pick a different qt version, like `6.7.1`.

Not really, as it'll break patches.

> Or you can temporarily use a new sources volume, similar to the diff in [#30851 (comment)](https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2337816419).

Thanks for a hint. I've added a t
...

👍 itornaza approved a pull request: "build: Bump minimum supported macOS to 12.0"
(https://github.com/bitcoin/bitcoin/pull/31048#pullrequestreview-2357857323)
Concept ACK

🤔 mzumsande reviewed a pull request: "init: Some small chainstate load improvements"
(https://github.com/bitcoin/bitcoin/pull/31046#pullrequestreview-2357858524)
Code Review / lightly tested ACK 31cc5006c3de4dd6a1f7a238684163956604df45

👍 itornaza approved a pull request: "doc: update IBD requirements in doc/README.md"
(https://github.com/bitcoin/bitcoin/pull/30992#pullrequestreview-2357871419)
ACK 36a6d4b0078ebb39ed082c866bf49214a2a01241

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402990346)
The issue with two copies of patches has been resolved. See:
- https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1791797379
- https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1791799818

Additionally, a few other comments have been addressed.

---

In the meantime, Qt 6.8.0 has been [released](https://www.qt.io/blog/qt-6.8-released). However, it [breaks](https://cirrus-ci.com/task/5888564838268928) cross-compiling for Windows on Debian Bookworm, which uses [GCC 12.2](htt
...

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1793963952)
Fixed in https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402990346.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1793964163)
Fixed in https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402990346.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1793964598)
Addressed in https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402990346.

💬 ryanofsky commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793837788)
re: https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793533834

Thanks, it's a good point that Untranslated(strprintf) is not equivalent to strprintf(Untranslated) in all cases. But I think those cases are basically broken because they are substituting non-english string fragments inside english strings.

I created a branch which removes these (4) cases and adds a scripted-diff to remove the remaining strprintf(Untranslated) uses. This allows a lot of simplifications to this PR:

...

💬 ryanofsky commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793913422)
re: https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793403826

> I don't think this is possible with a simple diff touching only lines that are already touched in this pull request. If you disagree, I am happy to accept any commit that compiles. I am also happy to review any pull request that achieves this and is split up from this pull request.

I see what you mean: it's not possible to introduce a util::detail namespace because it would conflict with the util::detail in the hex_
...

💬 ryanofsky commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793960605)
re: https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793488009

> in [aaaa4fb](https://github.com/bitcoin/bitcoin/commit/aaaa4fb20156b4375d92e1eca4acc90a425a1896): seems more straightforward to just extend `ConstevalStringLiteral` instead of creating a new `util::Original`? This would 1) minimize the diff and 2) I find `ConstevalStringLiteral` a much more helpful name than `Original`.

I think this is only a question of naming since `Original` class is replacing `ConstevalStringLit
...

💬 ryanofsky commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793914825)
re: https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793544349

> Thanks, added unrelated change to fix comment.

Thanks, comment makes sense. I knew there was probably some reason for doing it that way but couldn't figure out what it was.

💬 ryanofsky commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793951571)
re: https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793744937

This is a good catch, but it seems better to just drop the template method instead of trying to fix it up and use it. It just seems to add complexity and it doesn't seem like there is another use-case for it.

💬 ryanofsky commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793948085)
> nit: I would prefer templating this on an enum `{Translateable,Untranslateable}` for readability over the non-obvious `<{true,false}>` boolean

Note: with changes in suggested branch it should no longer be necessary for this to be templated.