💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1836348991)
I don't think it is right to use mockable time here. Also, `GetTime` is deprecated, so what about `TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now())`?

Also, I think it would be better to switch `test_name / rand_str` to `rand_str / test_name`, so that all unit tests from the same time are bundled together. This would also be identical to the functional test runner.

💬 maflcko commented on pull request "doc: Fix missing comma in JSON example in REST-interface.md":
(https://github.com/bitcoin/bitcoin/pull/31259#issuecomment-2467792806)
lgtm ACK 5e3b444022c354ad4d69e3142f7bc98d723c9e29

💬 TheCharlatan commented on pull request "refactor: Drop deprecated space in operator""_mst":
(https://github.com/bitcoin/bitcoin/pull/31267#issuecomment-2467799611)
```
git grep -i 'operator"" '
src/test/fuzz/miniscript.cpp:using miniscript::operator"" _mst;
src/test/miniscript_tests.cpp:using miniscript::operator"" _mst;
```
Might want to correct those two imports as well?

💬 maflcko commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1836375257)
Looks like this coverage should be preserved?

See also https://corecheck.dev/bitcoin/bitcoin/pulls/31248

🚀 fanquake merged a pull request: "tracing: Only prepare tracepoint arguments when actually tracing"
(https://github.com/bitcoin/bitcoin/pull/26593)

💬 l0rinc commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1836449767)
I thought of that, but not sure how to make it work, I'm getting:
> note: non-constexpr function 'strchr' cannot be used in a constant expression

🚀 fanquake merged a pull request: "ci: make ctest stop on failure"
(https://github.com/bitcoin/bitcoin/pull/31257)

💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2467876464)
Thanks for the comments @vasild. I plan to address your comments soon, when I rebase on https://github.com/bitcoin/bitcoin/pull/26593.

🚀 fanquake merged a pull request: "scripted-diff: get rid of remaining "command" terminology in protocol.{h,cpp}"
(https://github.com/bitcoin/bitcoin/pull/31163)

🚀 fanquake merged a pull request: "doc: Fixup bitcoin-wallet manpage chain selection args"
(https://github.com/bitcoin/bitcoin/pull/31264)

💬 willcl-ark commented on pull request "doc: Fixup bitcoin-wallet manpage chain selection args":
(https://github.com/bitcoin/bitcoin/pull/31264#discussion_r1836491398)
We generally address these as and when files are touched (and folks remember).

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2467888419)
I ran a `reindex-chainstate` until 860k blocks, before and after this change, 2 runs per commit:

<details>
<summary>benchmark</summary>

```bash
hyperfine \
--runs 2 \
--export-json /mnt/my_storage/reindex-chainstate-obfuscation-overhead.json \
--parameter-list COMMIT 866f4fa521f6932162570d6531055cc007e3d0cd,3efc72ff7cbdfb788b23bf4346e29ba99362c120,08db1794647c37f966c525411f931a4a0f6b6119 \
--prepare 'git checkout {COMMIT} && git clean -fxd && git reset --hard && cmake -B build -
...

🤔 Abdulkbk reviewed a pull request: "test: Add combinerawtransaction test to rpc_createmultisig"
(https://github.com/bitcoin/bitcoin/pull/31249#pullrequestreview-2427016825)
ACK 83fab3212c91d91fc5502f940c901a07772ff747

📝 purpleKarrot opened a pull request: "cmake: add optional source files to bitcoin_crypto directly"
(https://github.com/bitcoin/bitcoin/pull/31268)
Avoid having many static libraries by adding the optional sources to the target `bitcoin_crypto` directly.
Set the necessary compile options at the source file level, rather than the target level.

👍 Abdulkbk approved a pull request: "doc: Fix missing comma in JSON example in REST-interface.md"
(https://github.com/bitcoin/bitcoin/pull/31259#pullrequestreview-2427038611)
ACK 5e3b444022c354ad4d69e3142f7bc98d723c9e29

🚀 fanquake merged a pull request: "doc: Fix missing comma in JSON example in REST-interface.md"
(https://github.com/bitcoin/bitcoin/pull/31259)

💬 willcl-ark commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#discussion_r1836522285)
I think you have a copy-paste error here. `x86` should be `arm`?

💬 BrandonOdiwuor commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#issuecomment-2467950878)
@laanwj I updated the PR to include the suggestions

💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1836536074)
> Yes, Assume() is some middle ground between assert() and the softer approach. Assume() + cap in release builds looks reasonable to me too.

I agree. I can address it.

> so in current master nothing but 23 or 0 should ever be passed.

So, we could adjust both addrman and connman fuzz targets to get 23 or 0 to use for `max_pct`. Sounds good?

💬 maflcko commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1836539245)
Not sure about silently ignoring those. The goal of the script is to generate the manpages of the releases, and fail if it can't.

Allowing third parties to generate their own manpages can be implemented behind an option, or not at all.