💬 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

💬 marcofleon commented on pull request "[29.x] final changes for v29.1rc1":
(https://github.com/bitcoin/bitcoin/pull/33056#issuecomment-3117319768)
nice, ACK 4c2d285b706058ece7092d03f0e5ad1112c2097f

💬 fanquake commented on pull request "rpc: use CScheduler for relocking wallet and remove RPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3117350070)
Reproduced locally (and consistently) on aarch64 and x86_64. Wondered why this isn't happening in the TSAN CI, it's because of `DEBUG_LOCKORDER`. If you drop that, then the CI will reproduce:

<details><summary>more details</summary>

```bash
134/143 Test #107: transaction_tests .................... Passed 19.46 sec
135/143 Test #116: txvalidationcache_tests .............. Passed 21.00 sec
136/143 Test #140: wallet_tests ......................... Passed 18.79 sec
137/143 Test
...

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

🚀 fanquake merged a pull request: "log: [refactor] Use info level for init logs"
(https://github.com/bitcoin/bitcoin/pull/32967)