👍 theStack approved a pull request: "test: remove scanning check on `wallet_importdescriptors`"
(https://github.com/bitcoin/bitcoin/pull/31893#pullrequestreview-2623718174)
ACK 405dd0e647e4ed0402320917a06128eb69504655

💬 jonatack commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1959835437)
```suggestion
{"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The path to the directory of the wallet to be loaded, either absolute or relative to the \"wallets\" directory. The \"wallets\" directory is set by the -walletdir option and defaults to the \"wallets\" folder within the data directory."},
```

- be more clear with the distinction between the wallet directory and the wallets directory
- legacy wallet loading will soon be disabled: https://github.com/bitc
...

💬 maflcko commented on pull request "wip: Split fuzz binary (take 2)":
(https://github.com/bitcoin/bitcoin/pull/30882#issuecomment-2665846375)
The changes here could even be useful for afl++ on their own, but I haven't tried that either, as I am using libfuzzer.

💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2665861435)
re-ACK 6f0405177f89377ed272bae42becec1c75b8943c.

Lightly reviewed the code changes since my previous review. It's nice that `ExecCommand` is now a bit shorter, as a result of moving some helper functionality to `src/util/exec.h`. The Python changes are also easier to follow now thanks to splitting the commit.

I made a guix build for 6f0405, but didn't try including the IPC binaries with #31802 since there's too many build system changes there. That will be easier to test after #31741 lands
...

💬 maflcko commented on pull request "build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2665863991)
No opinion about the changes here, but if they are done, shouldn't the deterministic guix release build also set `DWCONFIGURE_ERROR=ON`? If there are any errors, it seems better to explicitly silence them.

💬 dergoegge commented on pull request "wip: Split fuzz binary (take 2)":
(https://github.com/bitcoin/bitcoin/pull/30882#issuecomment-2665867574)
> It could still make sense to try a FI run on this branch to see if it will help.

I've tried in the past but I kept running out of memory on a 128GB machine (see https://github.com/ossf/fuzz-introspector/issues/1742). I don't know how much more memory is needed currently. The maintainer indicated that they'd look at it but doesn't look like they had time for this so far.

> The changes here could even be useful for afl++ on their own, but I haven't tried that either, as I am using libfuzze
...

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959857461)
I guess it could also crash on a non-existing block hash? That's not unlikely to happen.

💬 yancyribbens commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665872160)
Hmm ok. I thought this was a simple win since there's no need to have 4 for loops here. Will keep it open for a bit longer to see if there is any other justification either way.

💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959862391)
Done.

💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959862589)
Nice, done.

💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959863364)
Done. I changed `pubkeys` to `keys`.

💬 sipa commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665881117)
Performance doesn't matter for tests.

✅ maflcko closed a pull request: "refactor: remove redundant `for` constructs"
(https://github.com/bitcoin/bitcoin/pull/31891)

💬 maflcko commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665881793)
> Will keep it open for a bit longer to see if there is any other justification either way.

The burden to explain and motivate a change is on the pull request author. If there was a need to have changes without motivation, I am sure an LLM can generate them en mass.

💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2665882208)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959636120, https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959639872 and https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959643968.

It adds 2 more test cases and fix a comment. Thanks, @rkrux.

💬 maflcko commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665883746)
Thank you for your contribution. While this stylistic change can make sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:

* Time spent on review
* Accidental introduction of bugs
* (Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer time or introduce bugs.

Generall
...

💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2665894022)
rebased

💬 hebasto commented on pull request "doc: update dev note examples for CMake":
(https://github.com/bitcoin/bitcoin/pull/30739#issuecomment-2665898730)
> On the command line, invoking the underlying commands explicitly is both easier to understand and faster to type:
>
> ```shell
> mkdir build
> cd build
> cmake -GNinja ..
> ninja
> ```

I would advise against this approach, as it may inadvertently lead to the use of an incorrect build tool.

For instance, on macOS, when GNU Make is installed via Homebrew, two build tools become available:
```
% which -a make
/usr/bin/make
hebasto@mini bitcoin % make --version
...

💬 yancyribbens commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665909290)
> The burden to explain and motivate a change is on the pull request author. If there was a need to have changes without motivation, I am sure an LLM can generate them en mass.

The author was paged above and just thought he had justification or maybe it was just an oversight.

👍 TheCharlatan approved a pull request: "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree"
(https://github.com/bitcoin/bitcoin/pull/31379#pullrequestreview-2623814063)
ACK c4c5cf174883cb53256e869f0d1673e29576a97c

I think I prefer this over passing through `SECP256K1_APPEND_FLAGS`. This is a distinct project after all, and having to configure all its subtrees/subprojects with separate flags seems cumbersome. It would also be inconsistent, since the method introduced here for libsecp already works fine for e.g. leveldb.