💬 laanwj commented on pull request "ci: Use `ninja` to build in macOS native CI job":
(https://github.com/bitcoin/bitcoin/pull/30915#issuecomment-2357962161)
ACK d01b85bfecbcf16ea16f90e2ade7537bf582269f

Good to test this in the CI, i prefer building using ninja.

💬 pablomartin4btc commented on pull request "Fix both display issues for IPv6 proxy setup and reachable networks in Options Dialog (UI only, no functionality impact)":
(https://github.com/bitcoin-core/gui/pull/836#discussion_r1764753872)
I'll drop the changes on `src/qt/optionsdialog.cpp` as suggested by @vasild, since the approach is incorrect.

💬 maflcko commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1764760058)
> Definitely not a dumb question, this change results in an extra copy (only a move is happening on this line, but there is a copy inside the `getBlock()` function, but a copy is also removed later on due to adding std::move in `GenerateBlock`, so the net number of copies should be the same.

I don't understand this comment. This line is part of the `getblocktemplate` RPC, which is a real RPC (possibly used in production). Whereas the `GenerateBlock` function is a test-only function for the te
...

💬 rkrux commented on issue "28.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2358014932)
@hodlinator Thanks for following the guide and reviewing it thoroughly. I agree with all your suggestions and have accepted them. The diagram is a nice touch. :)

💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1764780151)
Marking as unresolved so we don't lose track of it.

💬 maflcko commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1764801712)
(I edited my comment, because it seems that the block is fully serialized into json anyway, which should be way more expensive than a copy, so probably fine, but I haven't benchmarked this)

💬 marcofleon commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2358175361)
```
operator(): Assertion `!txdownload_impl.RecentRejectsFilter().contains(package.back()->GetWitnessHash().ToUint256())' failed.
```

`ZT8AAAD/////ZaOjaf/y/////f39/f39odAq0KMAZWU+qOHh4eHdzMsrZTKgWuPMyytlcaBaZWVl
MaBaK8vczGUx/wwAUlrdzMsrZTGgWuPMyytlcaBa2WVl//////8xoFrczMsrZTEAUlrdzMsrZTGg
WuMrZTH/DABS/////w==`

[txdownload_impl_crash.txt](https://github.com/user-attachments/files/17042820/txdownload_impl_crash.txt)

Child is still in reject filter? Still (slowly and steadily) working t
...

👍 l0rinc approved a pull request: "log: Use ConstevalFormatString"
(https://github.com/bitcoin/bitcoin/pull/30889#pullrequestreview-2312360142)
ACK facbcd4cef8890ae18976fb53b67ea56b3c04454

💬 l0rinc commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764863855)
Should this always be info?
Some `WalletLogPrintf` usages seem like [warnings](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/receive.cpp#L233-L234)), or [errors](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/backup.cpp#L600).

💬 l0rinc commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764855872)
nit: maybe we could dogfood this and use:
```suggestion
log_msg = tfm::format("Error \"%s\" while formatting log message: %s", fmterr.what(), fmt.fmt);
```

💬 l0rinc commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764878600)
clever workaround

💬 l0rinc commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764885813)
is there a conceptual difference between `Args` (used here) and `Params` (used in `WalletLogPrintf`)?

💬 maflcko commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#discussion_r1764891407)
nit: Now that `HAVE_BUILD_INFO` is removed, this can be placed under the "normal" includes, no? Also, a more descriptive name could be used, while touching this? Maybe `build_info.h` or `build_git_info.h`, as it may only contain the git commit or tag?

💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764902520)
No. It seems fragile to fall back to tinyformat when a tinyformat error occurred. Even if unlikley, that just seems like it could lead to another tfm exception, rendering this whole code useless in the first place.

💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764903584)
A refactor is not allowed to change behavior. If you want to change behavior, a separate issue or pull request seems more appropriate.

📝 l0rinc opened a pull request: "test: generalize HasReason and use it in FailFmtWithError"
(https://github.com/bitcoin/bitcoin/pull/30921)
Standardized boost exception checking in recent tests introduced in https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756493521 by extending `HasReason` to accept `const char*` through `string_view`.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1764904714)
Pushed this change in https://github.com/bitcoin/bitcoin/pull/30921

💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764906085)
No

💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764908089)
Added "This refactor does not change behavior of the compiled executables." to the pull description and applied the label.

💬 l0rinc commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764909660)
I understand (and somewhat share) the reasoning, but doesn't this mean that we don't trust `tfm::format` even in a fixed case?
E.g. isn't it possible after this call for `log_msg` to contain dangling format specifiers, which would be passed on to `LogInfo` anyway?