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

💬 glozow commented on pull request "p2p: never check tx rejections by txid":
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3120300985)
cc @darosior

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2231981895)
non-TRUC transactions shouldn't have TRUC children. I think it would be a bug if we set this variable for a transaction that isn't TRUC

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2231994092)
> [d2138be](https://github.com/bitcoin/bitcoin/commit/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?

It's subtle but we're doing the same with the wait here. The `blocking_tasks` vector contains the futures whose promises are set only when the worker finishes executing the task (meaning it has run all its code).

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2232002054)
> [d2138be](https://github.com/bitcoin/bitcoin/commit/d2138bef76ae7c25679571f9c07ecb44e99d4ecc)
>
> is this sleep to give tasks a chance to execute if the blocking breaks?

Good eye.
IIRC, I added it to wait until the workers actually get blocked. Otherwise, the thread pool queue size would be greater than the expected value (because it did not consumed the blocking tasks).
Still, we could remove this by adding some `ready_promises`, as did in test 2. A bit more code but less fragile.

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2232007294)
> [d2138be](https://github.com/bitcoin/bitcoin/commit/d2138bef76ae7c25679571f9c07ecb44e99d4ecc)
>
> Could anything be gained by checking the counter after each `ProcessTask()`?

don't think so. Could maybe improve it by making the task return something and checking the futures' promises. But I'm not totally convinced it will add much value.

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2232008180)
Good catch!, fixed.

🤔 cedwies reviewed a pull request: "wallet: Set descriptor cache upgraded flag for migrated wallets"
(https://github.com/bitcoin/bitcoin/pull/33031#pullrequestreview-3056796711)
- Unit tests (test_bitcoin) passed in 91.67 s (macOS Debug build)

questions/observations:
1. why set WALLET_FLAG_DESCRIPTORS and WALLET_FLAG_LAST_HARDENED_XPUB_CACHED in a single db write?
2. If the xpub cache flag was already true, could re-setting it trigger any unexpected behavior?
3. nit: please update the comment above the SetWalletFlagWithDB call to mention that it now also caches the last-hardened xpub.

ACK 4e5faab

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2232015151)
> [76aa1c2](https://github.com/bitcoin/bitcoin/commit/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 ...?

It was like that at first, but I found it harder to follow and wanted to simplify `func_worker` as much as possible.
You can try moving it back in and will surely see what I'm referring to.

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2232021760)
good catch!, fixed.

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2232035213)
> [76aa1c2](https://github.com/bitcoin/bitcoin/commit/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?

Hmm, or we could remove this line and call `CustomAppend` by default. Then it would be up to the child index to decide whether it needs sequential ordering during parallel sync or not. This way, the tx and BIP
...

🤔 dr8931240-ux reviewed a pull request: "kernel: chainparams updates for 26.x"
(https://github.com/bitcoin/bitcoin/pull/28591#pullrequestreview-3056829484)
Complete

💬 darosior commented on pull request "p2p: never check tx rejections by txid":
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3120416280)
Concept ACK