👍 fanquake approved a pull request: "cmake: Revamp `FindLibevent` module"
(https://github.com/bitcoin/bitcoin/pull/31181#pullrequestreview-2427529805)
ACK 5a96767e3f531ba9e8a676eec47727421f9f589f

💬 hodlinator commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2468421728)
> > Disallow -noconf since a config file is required
>
> But it's not. Seems like `-noconf` should work to ignore the config file.

As mentioned in the commit message for 2d65e6ae0756b87e82dbafd032308e8687ec7941:

> ArgsManager::GetConfigFilePath() asserts that a path is always set. Explicitly negating it is therefore invalid."
>
> Previously, -noconf would result in an ifstream being opened to the ".bitcoin"-directory (not a file) in ArgsManager::ReadConfigFiles(), also circumventing t
...

💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1836842757)
> 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())?

Yeah sure. This is usually called before mocking the time, but agree to change it.

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

That won’t work as you expect. T
...

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2468440155)
Rebased for renaming `TRACE5` -> `TRACEPOINT`.

💬 sipa commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2468443487)
Concept ACK. If it's possible to have differently-compiled compilation units without needing separate libraries for each, then this does seem like a cleaner solution. I suspect we perhaps ended up in this state because in autotools separate libraries were the only way to achieve this?

💬 l0rinc commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2468446882)
Concept ACK

👍 rkrux approved a pull request: "doc: correct typos"
(https://github.com/bitcoin/bitcoin/pull/31271#pullrequestreview-2427587276)
ACK 726cbee9553b25bedfef70cfd5be9f1eeec8a30d

🚀 fanquake merged a pull request: "cmake: Revamp `FindLibevent` module"
(https://github.com/bitcoin/bitcoin/pull/31181)

💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2468455953)
re-ACK 6c7b3bf0ce7a6238457dd44ee1d5d744b3663a52

💬 furszy commented on pull request "test: report detailed msg during utf8 response decoding error":
(https://github.com/bitcoin/bitcoin/pull/31251#discussion_r1836872860)
sure, done as suggested. Thanks

💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2468489703)
Force-pushed squashing and addressing https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1836765925.

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#issuecomment-2468489763)
re-crACK 9edbe5a48fe9a2c0d364882fcb7d44ae6aac6d24

💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1836877155)
Done.

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1836877971)
minor nit: I don't think this one needed to be renamed to `pair`. `self` still makes sense here.

💬 hodlinator commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2468495898)
Added commit which now guards against passing directory paths as `-conf` values as per https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2468421728.

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1836880690)
yeah, but it's more consistent this way (and method name already mentions self)

💬 achow101 commented on pull request "doc: Add missing 'blank=true' option in offline-signing-tutorial.md":
(https://github.com/bitcoin/bitcoin/pull/31237#issuecomment-2468501470)
Post merge -0

`disable_private_keys` already implies `blank` so there isn't really a point in specifying `blank` explicitly. I would have preferred to remove the earlier text that says both options would be set.

📝 maflcko opened a pull request: "ci: Bump valgrind tasks to clang-18"
(https://github.com/bitcoin/bitcoin/pull/31273)
This is the default, see https://packages.ubuntu.com/noble/clang, so likely more widely used. Thus, it seems better to use `clang-18`, instead of `clang-16` in the valgrind CI tasks.

💬 fanquake commented on pull request "doc: Add missing 'blank=true' option in offline-signing-tutorial.md":
(https://github.com/bitcoin/bitcoin/pull/31237#issuecomment-2468510059)
I guess if we decide to do that, we can also update the other guides that are doing this redundantly, i.e: https://github.com/bitcoin/bitcoin/blob/master/doc/multisig-tutorial.md#13-create-the-multisig-wallet.

💬 hodlinator commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2468514467)
cr-ACK ee1128ead846698db5e5633f193883837f2fbc64

`git range-diff master 0440c40 ee1128e`:

- Only [comment corrections](https://github.com/bitcoin/bitcoin/pull/31172#discussion_r1834418655.) since my last Windows 11 run.