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

💬 Sjors commented on pull request "subprocess: always check result of close()":
(https://github.com/bitcoin/bitcoin/pull/33061#issuecomment-3117063759)
Having gone through the code a bit more, it seems that throwing `OSError` is problematic for at least two reasons:

1. It can obfuscate other errors, since `close` is often called as part of cleanup
2. It makes errors out of things that are almost never an actual problem
3. If we upstream the change, it's a breaking change, requiring callers to catch and handle `OSError` in more scenarios

A better approach is probably to log (a narrow subset of) `close()` failures. There's no logging func
...

👍 rkrux approved a pull request: "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor"
(https://github.com/bitcoin/bitcoin/pull/29136#pullrequestreview-3054647308)
ACK 439734e

Thanks for incorporating suggestions; TIL about the for-else pattern in Python.

```bash
git range-diff 6100108...439734e
```

💬 rkrux commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2230555778)
In 71b4900f85eb18498ca93ef2eca64fe68ad350f5 "wallet: Add addhdkey RPC"

Nit if retouched:
```diff
- assert False, "Did not find HD key with no descriptors"
+ assert False, "Did not find any HD key with unused descriptor"
```

💬 rkrux commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2230556493)
Ah right! Fine to keep it as-is in the parsing flow.

Via `addhdkey` RPC though, I don't suppose there is a way to get _multipath_ keys added? (as only one hdkey can be generated or imported)

💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3117088226)
> The current failing CI

Should be fixed in my (just now) pushed changes for the base and sending PRs.

🤔 maflcko reviewed a pull request: "wallet: Remove wallet version and several legacy related functions"
(https://github.com/bitcoin/bitcoin/pull/32977#pullrequestreview-3054772912)
review ACK 8462d84c596315d1979d26b964657079ed5bc09b 🥙

<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 8462d84c5963
...

💬 maflcko commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2230677812)
nit in 269c43bdb04768954e59119363afdfa061849ed0: I haven't reviewed this, and it could make sense to add a test for this.

💬 maflcko commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2230683627)
nit in f5f643a7cd5ab251d2491661318553fd7463acaf: No need to cast the value?

💬 maflcko commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2230645828)
nit in fc4d563edb5feab273e5770f1b369cb6fe8a0c8c: This is the minversion, so could write:

```cpp
const int latest_legacy_wallet_minversion{169900};
```

💬 maflcko commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2230695414)
nit in dbf8dbf4b1792686be70050c3b19e477769d142b: Why pass an empty array?

💬 maflcko commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2230684594)
nit in f5f643a7cd5ab251d2491661318553fd7463acaf: Forgot to remove the function in the cpp file?

💬 stickies-v commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#issuecomment-3117179644)
re-ACK face8123fdc10549676c6679ee3225c178a7f30c

💬 fanquake commented on pull request "p2p: rename GetAddresses -> GetAddressesUnsafe":
(https://github.com/bitcoin/bitcoin/pull/32994#issuecomment-3117182153)
cc @mzumsande