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

💬 fanquake commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3117039847)
The PR description and commit message will need re-writing, to explain the motivation, as the current motivaiton is just user error (updating python and not reinstalling dependencies).

💬 fanquake commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3117046059)
> But why are we cloning the PyEnv repo, but then not just using pyenv install 3.10? Then the workaround wouldn't be needed.

If we are going to change anything here, then we might as well swap to this. Given the original motivation is unclear, and it'd be less lines of code.

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3117061381)
I've fixed all known errors. The current failing CI is unrelated.