🤔 macgyver13 reviewed a pull request: "Silent Payments: Receiving"
(https://github.com/bitcoin/bitcoin/pull/32966#pullrequestreview-3056616801)
working on backup / restore testing I noticed a few issues.

💬 macgyver13 commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2231883781)
I think this change is breaking the ability to import sp descriptor with private keys defined for scan and spend
This is the warning I receive when using importdescriptors:
"Not all private keys provided. Some wallet functionality may return unexpected errors"

🚀 achow101 merged a pull request: "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs"
(https://github.com/bitcoin/bitcoin/pull/32845)

💬 jonatack commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2231906796)
- For users who don't follow changes closely, perhaps mention when legacy wallets became no longer supported
- Perhaps briefly remind users how to migrate legacy wallets to descriptor ones

🚀 achow101 merged a pull request: "wallet: Remove `upgradewallet` RPC"
(https://github.com/bitcoin/bitcoin/pull/32944)

👍 pinheadmz approved a pull request: "index: initial sync speedup, parallelize process"
(https://github.com/bitcoin/bitcoin/pull/26966#pullrequestreview-3052111555)
code review ACK a3ddbe6bde6c296948cd963f396b35bb6a906b22

Built and tested on macos/arm64 as well as Debian/x86. Left several questions and suggestions. I really like ThreadPool as a util and look forward to using it for http workers as well, to share the code.

I'm currently rebuilding block filter and tx indexes with this branch to compare against master, which after 48 hours was only about 70% complete...

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE
...

💬 pinheadmz commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2228839630)
d2138bef76ae7c25679571f9c07ecb44e99d4ecc

is this sleep to give tasks a chance to execute if the blocking breaks?

💬 pinheadmz commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2228819659)
d2138bef76ae7c25679571f9c07ecb44e99d4ecc

would there be any benefit here to incrementing the counter at the end of each blocking task and check that they executed properly when unblocked?

💬 pinheadmz commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2228891058)
b36010261921adc74e61bdcbc91ba6b7778bad9a

"must be in-between 1 and..." but you allow `0`

also nit, could drop the "in-" and say "number between 1 and ..."

💬 pinheadmz commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2228841403)
d2138bef76ae7c25679571f9c07ecb44e99d4ecc

Could anything be gained by checking the counter after each `ProcessTask()`?

💬 pinheadmz commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2229349665)
76aa1c2da9635b99e2fb8792b24b53f320b90624

Did you mean `CustomPostProcessBlocks()` here?

💬 pinheadmz commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2231639166)
77a14b37aaf5b1c1a5dbe28e900a3824e5732f3f

The corresponding line in the serial/legacy processing path asserts the block data (I don't care much about that) but I do think you should keep the variable for the filter type if ever one day a new filter index is added:

```cpp
BlockFilter filter(m_filter_type, *Assert(block.data), *Assert(block.undo_data));
```

💬 pinheadmz commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2231646277)
76aa1c2da9635b99e2fb8792b24b53f320b90624

I wonder if this condition should be a bit more attention-getting, since it *should* never execute right? Either log something or `Assume()` for the benefit of future index developers?

💬 pinheadmz commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2228814663)
d2138bef76ae7c25679571f9c07ecb44e99d4ecc

Why not just `+1` in each task like you do in the next test block?

💬 pinheadmz commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2229009128)
76aa1c2da9635b99e2fb8792b24b53f320b90624

Whats the benefit of defining this as a lambda instead of just moving the code inside `func_worker`? It doesn't seem to be called anywhere else ...?

💬 fjahr commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#issuecomment-3120241966)
utACK 936bd874ac1581de532317741f5ab26e875935b3

🤔 murchandamus reviewed a pull request: "test: add assertions to SRD max weight test"
(https://github.com/bitcoin/bitcoin/pull/33058#pullrequestreview-3056684210)
Yeah, as you noted, Single Random Draw is non-deterministic, so you would only get a predictable input set if there is only exactly one composition permitted. I don’t think that’s necessary to test what you want, though:

💬 murchandamus commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2231932809)
If the intent is to check that the result adheres to the `max_selection_weight` limitation, you could check for that directly:

```suggestion
BOOST_CHECK(res);
BOOST_CHECK(res->GetWeight() <= max_selection_weight);
```

💬 murchandamus commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2231936926)
What SRD does is a bit different:

```suggestion
// 3) Test that SRD result does not exceed the max weight
```

💬 stickies-v commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#issuecomment-3120264668)
Force-pushed from [122b432](https://github.com/bitcoin/bitcoin/commit/122b432b71e148cd85edcd74df741f99ac7f187c) to [137319b](https://github.com/bitcoin/bitcoin/commit/137319b5d6b0c4fa9e015608d6b43018f08db45c) to address silent merge conflict from https://github.com/bitcoin/bitcoin/pull/32845:
- replaced `self.MaybeArg<std::string>` with `self.MaybeArg<std::string_view>` in `wallet/rpc/wallet.cpp`
- updated `EnsureUniqueWalletName` signature to take `std::optional<std::string_view>` instead of
...