💬 hebasto commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2344111410)
> Just tested the same on a Fedora box (ccache 4.10.2) and ended up with `Hits: 447 / 528 (84.66%)`.

What if you `ccache --zero-stats` after configuring for a new build tree, just before building?

On Fedora, `ccache` masquerades as a compiler, so all compilation during configuration can affect the final hit rate.

👍 ryanofsky approved a pull request: "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors"
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2297822100)
Code review ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7. Looks good!

Since last review, just renamed push methods to append, and applied a few other suggested tweaks

💬 m3dwards commented on pull request "ci: parse TEST_RUNNER_EXTRA into an array":
(https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2344112521)
@maflcko no apology needed, apologies I introduced a bug!

I will work out what's going on and propose a fix PR ASAP.

Thanks for the logs.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755133612)
I think using positional args may work for `*`, but I haven't checked it.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755134264)
For tinyformat, almost any format specifier type is valid. `%` as well: https://godbolt.org/z/f8zWsP6nT

Thanks, added the tests and a comment!

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755135236)
Thanks, added the tests!

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755135557)
Thanks, added a comment!

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755136016)
Thanks, added a comment!

💬 ryanofsky commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1755134235)
In commit "chain: simplify `deleteRwSettings` code and improve it's doc" (84663291275248fd52da644b0c2566bbf9cc780b)

Might be good to say in commit message that this change is a refactoring and does not change behavior. Could have "refactor:" prefix in the title.

👍 ryanofsky approved a pull request: "interfaces: #30697 follow ups"
(https://github.com/bitcoin/bitcoin/pull/30828#pullrequestreview-2297841266)
Code review ACK 84663291275248fd52da644b0c2566bbf9cc780b. Looks good, thanks for taking suggestions and applying them to the right commits. Only changes since last review were documentation improvements and simplifying delete method.

💬 ryanofsky commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1755133114)
In commit "chain: dont check for null settings value in `overwriteRwSetting`" (f8d91f49c7091102138fcb123850399e8fadfbc7)

Might be good to say in commit message that this change is a refactoring and does not change behavior. Could have "refactor:" prefix in the title.

💬 ismaelsadeeq commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1755152446)
thanks will do if there is need to retouch.

💬 ismaelsadeeq commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1755154932)
I will do it when their is need to retouch.

💬 maflcko commented on pull request "Add more cmake presets":
(https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344163054)
review ACK 1171871f63261e0c06d34baf216a54bfa747d75f

Seems fine to add recommended presets for common stuff like development or fuzzing.

Nit: Add `build:` prefix to pull request title and remove "CI" mention from the pull request description?

💬 sipa commented on pull request "Add more cmake presets":
(https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344184241)
> Nit: Add `build:` prefix to pull request title and remove "CI" mention from the pull request description?

Done.

💬 tdb3 commented on issue "28.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2344216440)
The guide is looking great!
I dry ran it through the `2. V3 Transactions TRUC` section so far. Love how the guide explains the "why" and "what" is being done before steps are executed.

Some minor suggestions below:

### `1. Testnet4 Support`
- The user starts bitcoind with testnet4, but `2. V3 Transactions TRUC`, uses regtest. Could add a statement telling the user to stop the nodes and something in the beginning of the next section to start for regtest.


### `2. V3 Transactions TRU
...

👍 ryanofsky approved a pull request: "build: Add more cmake presets"
(https://github.com/bitcoin/bitcoin/pull/30871#pullrequestreview-2297996310)
Code review ACK f15e817811e328423ea489870ead089128a6ef8c. This change is much needed to simplify my command line.

Could consider the name "all" or "enable-all" for the preset, and "build_all" for the directory name, but current "dev-mode" and "build_dev_mode" also seems ok.

👍 pablomartin4btc approved a pull request: "build: Fix `ENABLE_WALLET` option"
(https://github.com/bitcoin/bitcoin/pull/30867#pullrequestreview-2297998037)
tACK 0037d53d1a21ec8a5a97a83ab716d68030446021

Also checked building depends with `NO_WALLET`.

👍 ryanofsky approved a pull request: "util: Use consteval checked format string in FatalErrorf, LogConnectFailure"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2298007577)
Code review ACK fa26462e95291652b4021d91b014655f678149e8. Just new tests and comments since last review

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755184675)
I don't think we should be changing all these lines in this commit just for updating the style. It muddies the git blame.