💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2231837660)
Why is it safe to assume that the transaction doesn't already have a child here?

💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2231847269)
added these and `serialize.h`, hope that'll do. (never got iwyu running locally and the tidy job didn't cover the new file for some reason).

💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2231849239)
yes, I think the part with the "three items" is still correct, it talks about the values of the map.
I replaced the part about the key from blockfilterindex.cpp by a reference to db_key.h

💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2231851121)
Done - I added a comment to the file, using parts from `blockfilterindex.cpp`.
I agree that `db_key.h` is nicer, changed it to that.

💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#issuecomment-3120065955)
[03b67a2](https://github.com/bitcoin/bitcoin/commit/03b67a27d013e55891df69b1aef9425574b88f5e) to [936bd87](https://github.com/bitcoin/bitcoin/commit/936bd874ac1581de532317741f5ab26e875935b3):
Addressed feedback by @fjahr.

💬 achow101 commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#issuecomment-3120080986)
ACK c5c1960f9350d6315cadbdc95fface5f85f25806

💬 achow101 commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3120084517)
ACK d89c6fa4a71810cdb28395d4609632e1b22249b3

💬 macgyver13 commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2231890541)
Shouldn't the coin_type be different for regtest/signet? /352h/1h instead of /352h/0h

🤔 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));
```