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

💬 maflcko commented on issue "ci: don't pass GIT_EXEC_PATH inside test container":
(https://github.com/bitcoin/bitcoin/issues/32935#issuecomment-3117377451)
@olegrok If you want, you can test https://github.com/bitcoin/bitcoin/pull/33002

💬 Sjors commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3117486035)
Original discussion: https://github.com/bitcoin/bitcoin/pull/26716#issuecomment-1356314359

> I reckon the lower memory usage is due to using clang instead of gcc.

`pyenv install` also honors `CC=clang` (tested by setting `CC=fkfkkf` which fails)

> it'd be less lines of code.

It removes a few lines in the installers, but adds some for `pyenv global` and `pyenv init`.

Using PyEnv directly does make it easier to use multiple Python versions, in case any of our linters would benefit f
...

📝 HowHsu opened a pull request: "truc: optimize the in package relation calculation"
(https://github.com/bitcoin/bitcoin/pull/33062)
In PackageTRUCChecks(), we calculate the in-package-parents for an in package tx, which results in the total time complexisity being O(n^2), n is the number of Txs in the package. Let's precompute the overall relationships between all transactions to make it be O(n).