👍 hodlinator approved a pull request: "test: add global time to place exec tests within the same dir"
(https://github.com/bitcoin/bitcoin/pull/31291#pullrequestreview-2437371463)
ACK b7f40e8094e00c13a6c847340645b16704fed63b

Ran `ctest -j 20 --test-dir build` and `build/src/test/test_bitcoin` and inspected activity with `lsof |grep "/tmp.*bitcoin`. The former will create a new process and hence *time* directory for every test, but run them in parallel. The latter will run all tests sequentially in-process, resulting in only one new *time* directory.

💬 hodlinator commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2477602934)
@ryanofsky thanks for the review! Updated to cover negation for all users of `AbsPathForConfigVal`.

👍 ryanofsky approved a pull request: "Add destroy to BlockTemplate schema"
(https://github.com/bitcoin/bitcoin/pull/31288#pullrequestreview-2437526746)
Code review ACK 5f7dc0dbe64eb38afc74e9c4cba489382ea373bd

> @ryanofsky can you further clarify what the difference in behavior is with an without a `destroy` method?

I think it probably doesn't matter too much in this case, since the only thing the destroy method will currently do is free memory. But this does help future proof the interface, in case we want the destructor to do more things in the future, or if we add memory limits and want to give clients a way to ensure that previous bloc
...

💬 ryanofsky commented on pull request "Add destroy to BlockTemplate schema":
(https://github.com/bitcoin/bitcoin/pull/31288#discussion_r1843099829)
In commit "Add destroy to BlockTemplate schema" (5f7dc0dbe64eb38afc74e9c4cba489382ea373bd)

Would suggest making destroy the first method because it affect semantics of the object as a whole, and thats where I placed destroy in all the other interfaces. It is fine to keep the number @9 for it, or renumber if you prefer.

💬 ZKcash-IrishGov commented on something "":
(https://github.com/bitcoin/bitcoin/commit/0328dcdcfcb56dc8918697716d7686be048ad0b3#r149113901)
https://explorer.electriccash.global/

👍 ryanofsky approved a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2437555054)
Code review ACK 2c15a858ca7b3f14eaebf4b39134cd0e9c559273. No changes since the last review other than rebase.

I do think it would simplify this PR and make review more meaningful if #31072 could be merged first. This PR is combining application code changes which require looking at surrounding code and seeing if the changes fit in, with translation class changes which require thinking about API design. I think it's harder to evaluate both things clearly when they are bundled in the same PR. I
...

💬 luke-jr commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1843126749)
Seems trivial to fix properly? https://github.com/luke-jr/bitcoin/commit/56981318da5dc3145e9f244ad0584ef05c815f6b

💬 luke-jr commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2477853828)
Any reason not to scope the new variables? https://github.com/luke-jr/bitcoin/commit/851d354fc4f94695aabd12d21e470dc90d267b93

👍 tdb3 approved a pull request: "refactor: Avoid std::string format strings"
(https://github.com/bitcoin/bitcoin/pull/31287#pullrequestreview-2437567836)
code review and light test ACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d

Quick checks:
- Viewed the `-printpriority` description with `bitcoind -help -help-debug`
- Induced test failure in `txrequest_tests` to see the string.

💬 ryanofsky commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#issuecomment-2477861319)
Updated 46c55ab427c0771cacaed7a20612ffe2cc3543db -> 1e9ea7de0b5ee6d3179930d4fc93c8e276e525bd ([`pr/bfmt.3`](https://github.com/ryanofsky/bitcoin/commits/pr/bfmt.3) -> [`pr/bfmt.4`](https://github.com/ryanofsky/bitcoin/commits/pr/bfmt.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bfmt.3..pr/bfmt.4)) with no changes. Just updated commit messages to be more accurate now that #31174 is merged.

💬 ryanofsky commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2477864531)
> Just repeating my "note to maintainers" from the description: like with Cirrus, `SKIP_BRANCH_PUSH=true` needs to be set for the GUI repo to maintain existing behaviour.
>
> Probably [here](https://github.com/bitcoin-core/gui/settings/variables/actions).

Can confirm this was set at the time this PR was merged.

💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-2477872950)
@rkrux thanks for the heads up. I didn't expect changes to build system would affect this. I'll rebase asap, also want to bring in achow's [#22838](https://github.com/bitcoin/bitcoin/pull/22838)

🤔 ryanofsky reviewed a pull request: "util: Improve documentation and negation of args"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2437584042)
Code review 77c8a538b7164f8b65576600eb4668ac160f6b14. Thanks for the updates! This all looks great, my only substantial comments are about the RPC commit which I think is more complicated than it needs to be and may have a bug where it may allow an empty auth string to be accepted if -norpccookiefile is specified, or if there is an initialization failure.

It may be nice to add python functional tests to exercise some of the new behaviors. It would be nontrivial work to add test coverage for a
...

💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1843140875)
In commit "args: Support -norpccookiefile" (cf3e4edc803bf950f280b24c5283a2838c86faf6)

I think having two separate accesses to the -rpccookiefile in different functions is fragile since `GetAuthCookieFile` will not do the right thing if called in a different context. It is also not taking advantage of GetPathArg's built in handling of negation. I think a change like:

```diff
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -86,6 +86,7 @@ static const char* const COOKIEAUTH_FILE = "
...

💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1843188789)
In commit "args: Properly support -noconf and catch directories" (77c8a538b7164f8b65576600eb4668ac160f6b14)

It doesn't seem good to try to open a file with an empty path. This behavior doesn't seem well defined and the call will unnecessarily be passed down to the os when it shouldn't be. I think it would also clarify the code and avoid the need to add a second IsArgNegated call if this checked for an empty path. Would suggest:

```c++
const auto conf_path{GetConfigFilePath()};
st
...

💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1843179490)
In commit "args: Support -norpccookiefile" (cf3e4edc803bf950f280b24c5283a2838c86faf6)

The changes to `httprpc.cpp` seem too complicated and I don't think they are safe because now that an empty `strRPCUserColonPass` string is allowed by `RPCAuthorized`, the `TimingResistantEqual` function can return true when the client provides an empty auth string.

I also think changing `strRPCUserColonPass` to `std::optional` makes state more complicated and code more confusing to think about, even as i
...

👍 rkrux approved a pull request: "refactor: Avoid std::string format strings"
(https://github.com/bitcoin/bitcoin/pull/31287#pullrequestreview-2437724875)
tACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d

Make and unit tests successful. Checked the `printpriority` arg message in help.

```
-printpriority
Log transaction fee rate in BTC/kvB when mining blocks (default: 0)
```

💬 maflcko commented on issue "valgrind: Conditional jump or move depends on uninitialised value(s)":
(https://github.com/bitcoin/bitcoin/issues/29635#issuecomment-2478073203)
The current CI with clang-16 seems to fail on arm64 anyway?

```
/ci_container_base/ci/scratch/build-aarch64-unknown-linux-gnu/src/test/test_bitcoin -t wallet_tests
```

```
==7187== Conditional jump or move depends on uninitialised value(s)
==7187== at 0xD363B4: wallet::MakeDatabase(fs::path const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) (./wallet/walletdb.cpp:1447)
==7187== by 0xCF6DCB: wallet::MakeWalletDatabase(std::__cxx11::basic_string<char,
...

💬 maflcko commented on pull request "test: Fix RANDOM_CTX_SEED use with parallel tests":
(https://github.com/bitcoin/bitcoin/pull/30737#issuecomment-2478167460)
If you want you can just open a new pull, it will have to be reviewed from scratch anyway.



> Are you still against passing through RANDOM_CTX_SEED in the test_runner.py? In that case I can drop that commit.

Yes, adding code like `fuzz_env['RANDOM_CTX_SEED'] = random_seed` to the fuzz runner also seems to go in the wrong direction a bit. The goal of the fuzz tests is to be fully deterministic and stable, within a single run and across different runs. If the fuzz randomness is seeded wit
...

💬 maflcko commented on pull request "Add `contrib/justfile` containing useful development workflow commands.":
(https://github.com/bitcoin/bitcoin/pull/31292#discussion_r1843352532)
Not sure about adding this. It seems more indirections to type `just configure -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' ..etc...` than just `cmake -B build -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' ...etc...` directly.

In the end, I think for this to be useful, the developer or user would have to write the justfile themselves. Trying to offer one (even if it is just an example) is almost guaranteed to only find use by a single person.