💬 rkrux commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1959793328)
Nit: The Windows example could be more generic in case of absolute path.

```
HelpExampleCli("loadwallet", "\"DriveLetter:\\path\\to\\walletname\\\"")
```
or
```
HelpExampleCli("loadwallet", "\"C:\\path\\to\\walletname\\\"")
```

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2665802465)
Rebased due to the conflict with the merged bitcoin/bitcoin#31268.

💬 l0rinc commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1959806856)
Please resolve the comment, seems it's not strictly related to the issue

💬 dergoegge commented on pull request "wip: Split fuzz binary (take 2)":
(https://github.com/bitcoin/bitcoin/pull/30882#issuecomment-2665824563)
The full report (https://storage.googleapis.com/oss-fuzz-introspector/bitcoin-core/inspector-report/20250216/fuzz_report.html) still seems blocked on our dynamic harness look up. Maybe I'm looking at the wrong thing.

💬 maflcko commented on pull request "wip: Split fuzz binary (take 2)":
(https://github.com/bitcoin/bitcoin/pull/30882#issuecomment-2665838423)
No it is the right thing. I just wanted to leave a comment to say that FI will now fallback to the light version by default, so the build is already passing, but the result didn't look like it could be used.

It could still make sense to try a FI run on this branch to see if it will help.

💬 NicolaLS commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#issuecomment-2665841137)
[Preview](https://github.com/NicolaLS/bitcoin/blob/doc-followup/doc/dependencies.md)

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