💬 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

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

The rpc doesn't crash on exceptions, no?



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

Yes, but it depends on the context. As mentioned above, most of the places intentionally and correctly ignore the errors, as I understand it? Blindly throwing exceptions without looking at the context doesn't seem the right approach to me.

💬 Sjors commented on pull request "subprocess: always check result of close()":
(https://github.com/bitcoin/bitcoin/pull/33061#discussion_r2230546363)
Good point. So IIUC `cleanup_fds` makes up to three `close` calls, and we want to do them all even if one fails. This could be handled with an `ignore_failure` and throwing at the end. Though that does add complexity.

🚀 fanquake merged a pull request: "fuzz: Make process_message(s) more deterministic"
(https://github.com/bitcoin/bitcoin/pull/32822)

💬 maflcko commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3117031567)
lgtm ACK e71990fabc53c53d1524ea00851e0d9a67a8f86f

CI: https://cirrus-ci.com/task/5023354685489152?logs=build#L1722

```
[15:02:47.597] #10 36.72 ++ cat /.python-version
[15:02:47.813] #10 36.72 + PYTHON_VERSION=3.10
[15:02:47.813] #10 36.72 ++ python-build --definitions
[15:02:47.813] #10 36.72 ++ grep '^3.10'
[15:02:47.813] #10 36.72 ++ tail -1
[15:02:47.813] #10 36.73 + PYTHON_PATCH_VERSION=3.10.18
[15:02:47.813] #10 36.73 + env CC=clang python-build 3.10.18 /python_build
[15:02:4
...