💬 laanwj commented on pull request "key: clear out secret data in `DecodeExtKey`":
(https://github.com/bitcoin/bitcoin/pull/31166#issuecomment-2446055905)
Yes, using secure_allocator is a better option if possible.

💬 ismaelsadeeq 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-2446067820)

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

<details>
<summary>Show diff</summary>

```diff
diff --git a/src/bench/rpc_blockchain.cpp b/src/bench/rpc_blockchain.cpp
index 7e3e2d8e48a..6d431685e66 100644
--- a/src/bench/rpc_blockchain.cpp
+++ b/src/bench/rpc_blockchain.cpp
@@ -45,7 +45,25 @@ struct TestBlo
...

💬 maflcko commented on issue "`Wunused-member-function` in test each commit":
(https://github.com/bitcoin/bitcoin/issues/31180#issuecomment-2446083130)
It may be trivial to suppress either way by using https://en.cppreference.com/w/cpp/language/attributes/maybe_unused in the commit?

It should also be trivial to remove the error completely from the CI task by specifying `Wno-error=unused-member-function`?

💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2446116915)
> > It would be good if ctest/Windows actually printed the exception and error message instead of silently hiding it.
>
> I'm not aware of the specific case discussed above, but exceptions in tests are generally printed as follows (with a patched code):

Maybe the log is truncated in CI and only the head is printed, as opposed to the tail, which would be more useful?

💬 hebasto commented on pull request "cmake: Add `FindZeroMQ` module":
(https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2446215480)
> We should also follow up with refactoring the libevent module, to more generically use CMake/pkg-config, rather than restricting the CMake usage to `vcpkg`. At that point, we'd likely be able to dump pkg-config for the depends path entirely.

Done in https://github.com/bitcoin/bitcoin/pull/31181.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1822182678)
This could be `ReattemptInitialBroadcast`, `BroadcastTransaction` (rpc/wallet), `ProcessValidTx`, or earlier upon handling `NetMsgType::TX`.

I think it's odd we have two calls to `RelayTransaction` in the two latter cases. I'd rather change this code, e.g. pass `already_relayed` (duplicating force) to `RelayTransaction`. Perhaps too much for this PR and we should do it elsewhere (especially now that the package stuff happens in-between). I understand why it was ok when `RelayTransaction` was
...

💬 hebasto commented on issue "build: use UCRT runtime for Windows (release) binaries":
(https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-2446405138)
FWIW, here is a way to cross-build UCRT-linked Windows binaries on Fedora 41:
```
$ sudo dnf install ucrt64-gcc-c++
$ make -C depends HOST=x86_64-w64-mingw32ucrt mingw32_CC=/usr/bin/x86_64-w64-mingw32ucrt-gcc mingw32_CXX=/usr/bin/x86_64-w64-mingw32ucrt-g++
$ cmake -B build --toolchain depends/x86_64-w64-mingw32ucrt/toolchain.cmake
$ cmake --build build
```

💬 hebasto commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2446416679)
> ... and suggest linking against UCRT instead.

I've built this branch on Fedora 41, linking against UCRT (see [this](https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-2446405138) workflow), but the issue persists.

📝 maflcko opened a pull request: "ci: Place datadirs for tests under tmpfs "
(https://github.com/bitcoin/bitcoin/pull/31182)

💬 laanwj commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1822298782)
DispatchMapPort is local and only called in one place; might as well roll it into `StartMapPort`?

💬 laanwj commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1822306060)
Maybe i'm missing something but the intent of `g_mapport_current` is hazy to me. It's turned on in `ThreadMapPort` while `ProcessPCP` is running, then turned off temporarily when `ProcessPCP` returns false.

- If we want to know if the mapport thread is running, `g_mapport_thread.joinable()` seems to suffice.
- If we want to know if mapport thread is supposed to be running, or stop it, there's `g_mapport_enabled`.

💬 fanquake commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#issuecomment-2446497505)
I think you could just remove the doc block being added here, and then this can go ahead (also rebase). Doesn't seem like it needs to be blocked on #30861, if that is why it was drafted.

🤔 hodlinator reviewed a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2404320459)
Concept ACK e53829d3952c6ed275507a66e77636aad67d106b

Cleanest attempt at increased compile time validation of format so far. When reviewing #31149 I had the gnawing feeling that more complete format string support would have reduced the diff, but pushed it away for expediency (an earlier attempt at more complete support was attempted in #30999).

💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822308585)
```suggestion
}
```

👍 laanwj approved a pull request: "test: Don't enforce BIP94 on regtest unless specified by arg"
(https://github.com/bitcoin/bitcoin/pull/31156#pullrequestreview-2404394664)
Code review ACK e60cecc8115d3b28be076792baa5e4ea26d353a6

💬 willcl-ark commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2446546723)
utACK 0de3e96e333090548a43e5e870c4cb8941d6baf1

Based on `git range-diff 38e24df...0de3e96e333090548a43e5e870c4cb8941d6baf1` .

The cmake tidy-up looks good to me, and I examined newly built binaries for probe and semaphore info. I didn't re-run the testing from my previous ACK.

💬 fanquake commented on pull request "cmake: Revamp `FindLibevent` module":
(https://github.com/bitcoin/bitcoin/pull/31181#issuecomment-2446546898)
Concept ACK - we don't have to restrict CMake usage to `vcpkg`, and this will let us make more cleanups.

💬 l0rinc commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822380551)
Found the PR thanks to @hodlinator, this is how I handled that case: https://github.com/bitcoin/bitcoin/pull/30999/files#diff-71badc1cc71ba46244f7841a088251bb294265f4fe9e662c0ad6b15be540eee4R60

We could a test case for the incomplete trailing number (or any other test that seems relevant here): https://github.com/bitcoin/bitcoin/pull/30999/files#diff-718d0d85269ec81595bd9f9181eea3a74b20244b07f14c546e3e07520b2b5f82R81

💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822392320)
Isn't that done here?
https://github.com/bitcoin/bitcoin/blob/ad37073f2e6ab1f39a59109692f84cc85809f53e/src/test/util_string_tests.cpp#L90

💬 l0rinc commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822395900)
It is indeed now, thanks for checking.