🤔 marcofleon reviewed a pull request: "allocators: Apply manual ASan poisoning to `PoolResource`"
(https://github.com/bitcoin/bitcoin/pull/32581#pullrequestreview-2993932032)
code review ACK ad132761fc49c38769c09653a265fdbc3b93eda5

Built with ASan and ran the unit tests to be sure. Also added a small use-after-free test in `pool_tests` that passes on master but not on this PR, as expected. Nice.

💬 furszy commented on pull request "wallet: remove dead code in legacy wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32758#discussion_r2190143690)
would keep the assertion. It would be bad if this is ever not the case and we end up resetting the pointer.

💬 l0rinc commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#issuecomment-3045279193)
That's my expectation, yes. Would you like me to measure it to sure?

🤔 furszy reviewed a pull request: "index: fix wrong assert of current_tip == m_best_block_index"
(https://github.com/bitcoin/bitcoin/pull/32878#pullrequestreview-2994003229)
I don't think the test is useful. The change is being tested against a `Sync()` mechanism introduced within the test itself and not the real one, which can change over time (see for example #26966 that changes it completely).
It would be better, and simpler for you, to share a patch that reproduces the issue (as @mzumsande mentioned, just adding a sleep() call and triggering the reorg is enough to reproduce it).

Other than that, concept ACK.

💬 maflcko commented on pull request "New SVG, Icons, PNGs and X PixMaps":
(https://github.com/bitcoin/bitcoin/pull/32891#issuecomment-3045287022)
The are optimized, see https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/optimize-pngs.py

💬 fanquake commented on pull request "depends: fix libevent `_WIN32_WINNT` usage":
(https://github.com/bitcoin/bitcoin/pull/32837#issuecomment-3045291987)
The cross-arch non-determinism here is in `bitcoin-qt.exe`.
x86_64
```bash
55c49e31615341a1f8e72cd4c7193a3052843a2a21cecaa42d1c374faf4e1600 bitcoin-5565d7d610dd/bin/bitcoin-cli.exe
c91ca56fd76dfddf120d1f92f692eff99f4f4e7ff64f2c486b44ce08c4ef930c bitcoin-5565d7d610dd/bin/bitcoin-qt.exe
7ddc376534a0811e5c6dd3a49cfffafef1c23be9e76f1e5586bfdbba25a2dbe5 bitcoin-5565d7d610dd/bin/bitcoin-tx.exe
38cd1c4bd75862ff5a58052027d006140761cb4162ec59eb90b0ed4e9e0ddc0e bitcoin-5565d7d610dd/bin/bitcoin-u
...

💬 maflcko commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#issuecomment-3045335544)
lgtm ACK 4207d9bf823bea9f94b484ebf3c9274ca781b245 🍄

<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: lgtm ACK 4207d9bf823bea9f
...

💬 maflcko commented on pull request "test, subprocess: Improve coverage report correctness":
(https://github.com/bitcoin/bitcoin/pull/30075#issuecomment-3045397135)
@hebasto are you still working on this?

💬 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_r2189541657)
In 0d24f0af7a3430dd8861d99390d902cde21f4275 "wallet: Add addhdkey RPC"

I believe this requires a release note?

💬 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_r2190005890)
In 10b677530e5fd9c502290acbcb9c706a2a368b5b "descriptor: Add unused(KEY) descriptor"

Perhaps we can update the doc of this function?
https://github.com/bitcoin/bitcoin/blob/e3f416dbf7633b2fb19c933e5508bd231cc7e9cf/src/script/descriptor.h#L111-L112

```diff
- /** Whether this descriptor will return one scriptPubKey or multiple (aka is or is not combo) */
+ /** Whether this descriptor will return at most one scriptPubKey or multiple (aka is or is not combo) */
```

Because in this c
...

💬 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_r2190133772)
In 10b677530e5fd9c502290acbcb9c706a2a368b5b "descriptor: Add unused(KEY) descriptor"

Nit because there already is a note below.

```diff
- // Apply the label if necessary, for descriptors that produce output scripts (i.e. not unused())
- // Note: we disable labels for ranged descriptors
+ // Apply the label if necessary
+ // Note: we disable labels for descriptors that are ranged or that don't produce output scripts (i.e. unused())
```

💬 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_r2190031968)
In 10b677530e5fd9c502290acbcb9c706a2a368b5b "descriptor: Add unused(KEY) descriptor"

For explicitness, loop can be replaced with an `Assume` on length being one.

```diff
- for (auto& pubkey : keys) {
- if (pubkey->IsRange()) {
- error = "unused(): key cannot be ranged";
- return {};
- }
- ret.emplace_back(std::make_unique<UnusedDescriptor>(std::move(pubkey)));
+ Assume(keys.size() == 1);
+ auto& pub
...

💬 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_r2190191964)
In 0d24f0af7a3430dd8861d99390d902cde21f4275 "wallet: Add addhdkey RPC"

Only reason being to restrict the call sites of `MakeNewKey` function that at the moment is being used only in one place barring the unit tests - straightaway lets us know the places of key generation in the wallet.

```diff
- CKey seed_key;
- seed_key.MakeNewKey(true);
+ CKey seed_key = GenerateRandomKey();
```

💬 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_r2190198179)
In https://github.com/bitcoin/bitcoin/commit/0d24f0af7a3430dd8861d99390d902cde21f4275 "wallet: Add addhdkey RPC"

Should we add a check for external signer wallet flag as well here?

💬 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_r2190215458)
In https://github.com/bitcoin/bitcoin/commit/0d24f0af7a3430dd8861d99390d902cde21f4275 "wallet: Add addhdkey RPC"

```diff
- expected_void_desc = descsum_create(f"unused({imp_xpub})")
+ expected_unused_desc = descsum_create(f"unused({imp_xpub})")
```

💬 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_r2190230878)
In https://github.com/bitcoin/bitcoin/commit/0d24f0af7a3430dd8861d99390d902cde21f4275 "wallet: Add addhdkey RPC"

I don't think this test is called and it fails upon being called because I don't believe the wallet is created without private keys.

💬 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_r2190214038)
In https://github.com/bitcoin/bitcoin/commit/0d24f0af7a3430dd8861d99390d902cde21f4275 "wallet: Add addhdkey RPC"

Shouldn't the descriptor checksum be added?

💬 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_r2190224703)
In https://github.com/bitcoin/bitcoin/commit/0d24f0af7a3430dd8861d99390d902cde21f4275 "wallet: Add addhdkey RPC"

There's a redundant RPC.
```diff
- wallet.listdescriptors()
- for d in wallet.listdescriptors()["descriptors"]:
+ descs = wallet.listdescriptors()
+ for d in descs["descriptors"]:
```

💬 kevkevinpal commented on pull request "threading: use correct mutex name in reverse_lock fatal error messages":
(https://github.com/bitcoin/bitcoin/pull/32829#issuecomment-3045441605)
> Done in #32888

awesome going to squash [41f64bd](https://github.com/bitcoin/bitcoin/pull/32829/commits/41f64bd636f580851ef9aa27d56dc45885ace0c7) and [85c2848](https://github.com/bitcoin/bitcoin/pull/32829/commits/85c2848eb575f4abaa81fdd4e8f3b2048693dd98)

💬 1BitcoinBoWP1FZ4xwTNkq6XksKidmgYYw commented on pull request "New SVG, Icons, PNGs and X PixMaps":
(https://github.com/bitcoin/bitcoin/pull/32891#issuecomment-3045444299)
@maflcko You mean this commit? https://github.com/bitcoin/bitcoin/commit/42f6a0c2b946af2cce86b721b349d35c4e21ce88
Yes the PNGs was optimized but not correctly, that's why you can see in my commit for example that my bitcoin.png is -256 KB (16%) smaller than the current file in the repo. https://github.com/bitcoin/bitcoin/pull/32891/files#diff-5e43a57fa6d8a58177b80b3d7d608e6fffcda2a75078da2aa47bb4e447b44f81 also the script only optimized PNG files unlike my commit that optimized all bitcoin log
...