🤔 hebasto reviewed a pull request: "depends: fix libevent `_WIN32_WINNT` usage"
(https://github.com/bitcoin/bitcoin/pull/32837#pullrequestreview-3054470510)
Post-merge ACK f5647c6c5ae85e9469cfc5df6fcac23752e1695a.

> > ACK [524dd98](https://github.com/bitcoin/bitcoin/commit/524dd98cb1cfa3a7917b94e13a91827842e7aba8), tested on macOS 15.5 (x64) using Homebrew's mingw-w64 13.0.0.
>
> Retracting my ACK basing on non-reproducibilty of the Windows builds.

I can confirm reproducibility across `x86_64`, `aarch64` and `riscv64` platforms:
```
71847e8121e14afe0ef221b40faa9b07d5ea948b5ab2082cb4d4e449e74eb87b guix-build-f5647c6c5ae8/output/dist-archi
...

💬 maflcko commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2230430411)
> I'm not against this change, I just don't fully understand why some of the `LogPrintf` weren't migrated, e.g.

thx, done httpserver. The others you mention are not init logs (those that happen only once during startup and shutdown), because they are called in a loop for as long as the program runs.

💬 maflcko commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2230431049)
thx, done

💬 maflcko commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2230431379)
thx, done

💬 maflcko commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2230469671)
> Thanks, will do this in the next push

I don't think you did?

📝 Sjors opened a pull request: "subprocess: always check result of close() "
(https://github.com/bitcoin/bitcoin/pull/33061)
Suggested in https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-3071393448

💬 maflcko commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3116865212)
review ACK d89c6fa4a71810cdb28395d4609632e1b22249b3 🤙

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK d89c6fa4a718
...

💬 Sjors commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-3116867038)
@tnndbtc I opened #33061 to do this

💬 Sjors commented on pull request "subprocess: always check result of close()":
(https://github.com/bitcoin/bitcoin/pull/33061#issuecomment-3116873333)
I suppose we should catch `OSError` and not crash if this ever happens...

🤔 maflcko reviewed a pull request: "subprocess: always check result of close()"
(https://github.com/bitcoin/bitcoin/pull/33061#pullrequestreview-3054567458)
This "fix" is just introducing other bugs down the line, no?

💬 maflcko commented on pull request "subprocess: always check result of close()":
(https://github.com/bitcoin/bitcoin/pull/33061#discussion_r2230497364)
(same below)

💬 maflcko commented on pull request "subprocess: always check result of close()":
(https://github.com/bitcoin/bitcoin/pull/33061#discussion_r2230496925)
returning early here will leak fds if the exception is caught and everything is tried again?

💬 maflcko commented on pull request "subprocess: always check result of close()":
(https://github.com/bitcoin/bitcoin/pull/33061#discussion_r2230494730)
This will just confusingly change the error message, claiming that the close failed, because the fd is not available? How would the fd be available if the open failed?

💬 fanquake commented on pull request "subprocess: always check result of close()":
(https://github.com/bitcoin/bitcoin/pull/33061#issuecomment-3116900258)
Have you also sent this change upstream? https://github.com/arun11299/cpp-subprocess

💬 Sjors commented on pull request "subprocess: always check result of close()":
(https://github.com/bitcoin/bitcoin/pull/33061#discussion_r2230509477)
Should we even call `close()` in that case? Alternatively I could add a `ignore_failure` argument to `util::close`.

💬 fanquake commented on pull request "subprocess: always check result of close()":
(https://github.com/bitcoin/bitcoin/pull/33061#issuecomment-3116902969)
cc @hebasto

💬 Sjors commented on pull request "subprocess: always check result of close()":
(https://github.com/bitcoin/bitcoin/pull/33061#issuecomment-3116904856)
> This "fix" is just introducing other bugs down the line, no?

That's possible, though in general it seems not ignoring errors is a good thing.

> Have you also sent this change upstream? https://github.com/arun11299/cpp-subprocess

Not yet, will do depending on feedback here. Marking as draft.

📝 Sjors converted_to_draft a pull request: "subprocess: always check result of close()"
(https://github.com/bitcoin/bitcoin/pull/33061)
Suggested in https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-3071393448

💬 willcl-ark commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3116914284)
Post-merge ACK 06ab3a394ade1e0d4fdb1fef636ff0d6bad71948

💬 maflcko commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2230521184)
fc4d563edb5feab273e5770f1b369cb6fe8a0c8c: Returning a constant seems confusing in the context of https://github.com/bitcoin/bitcoin/pull/32895