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

💬 marcofleon commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3120296633)
Crash in the `txorphan` target:
```bash
/src/test/fuzz/txorphan.cpp:145 operator(): Assertion `orphanage->TotalOrphanUsage() == total_bytes_start' failed.
```
[txorphan_crash.txt](https://github.com/user-attachments/files/21437145/txorphan_crash.txt)

The `LimitOrphans` in `AddTx` ends up decreasing `m_unique_orphan_usage` in this case. If we only add an announcer, it makes sense that memory usage shouldn't increase, but it could be decreased if a transaction is completely removed from the
...

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2231978461)
> [d2138be](https://github.com/bitcoin/bitcoin/commit/d2138bef76ae7c25679571f9c07ecb44e99d4ecc)
>
> Why not just `+1` in each task like you do in the next test block?

Hmm, good question. I was probably not only testing that all tasks were executed, but also that each of them was executed only once. Or… maybe I just wanted to mention Gauss somewhere in our code.
I did this one 3 years ago so.. it is hard to know. But it doesn't hurt to have it.

📝 glozow opened a pull request: "p2p: never check tx rejections by txid"
(https://github.com/bitcoin/bitcoin/pull/33066)
Main motivation of this PR is to enable #32379 without creating a censorship problem where the attacker sends a witness-stripped version of a transaction to cause us to reject its child. It does this by removing all queries to the rejection caches by txid.

TLDR, this PR removes the logic that stops us from attempting orphan resolution when parents are found in the rejection filter. It removes the rejection cache filtering part of all txid-based tx requests. In practice, this seems to only be
...