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

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755179261)
This part `Coins whose output index is 4 (mod 5) have GetCoin() always succeed after being spent` is due to us not erasing these coins with this condition on line 169 below. Since we are not yet not returning spent coins (`// TODO GetCoin shouldn't return spent coins`), we shouldn't remove this line of the comment.

💬 sipa commented on pull request "build: Add more cmake presets":
(https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344249910)
@ryanofsky I picked the name to match the same thing in [libsecp256k1](https://github.com/bitcoin-core/secp256k1/blob/v0.5.1/CMakePresets.json#L6), but I'm okay with "enable-all" or "all" as well.

💬 maflcko commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1755195956)
Is there a reason to extract those function? I think it would be better to not extract them, because:

* The diff would be smaller
* They are only used and called once
* There shouldn't be a reason for them to be called from somewhere else internally, because `operator<<` is available internally and externally
* If someone wanted to use them externally (for whatever reason), they'd have to be moved again, because they are private right now.

Seems easier to extract them and possibly make
...