💬 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

💬 yancyribbens commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2232042405)
Is it not true that when the max weight is encountered, the least valuable utxo is removed from the selection via the min-heap? Therefore the most valuable are returned?

💬 achow101 commented on pull request "test: Move `script_assets_tests` into its own suite":
(https://github.com/bitcoin/bitcoin/pull/31576#issuecomment-3120430867)
ACK c40dbbbf77078bb86a652ca151b472e7bef61ae0

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2232047661)
Thanks! Removed in a4084cb7554

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2232048386)
I have linked to the commit in e83c3718f2c and removed the dangling DANGER_DOCKER_BUILD_CACHE_HOST_DIR below

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2232048838)
echo also removed in e83c3718f2c

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

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2232051946)
6d252266e40 and 3e757e511a3 have had the `USE_CIRRUS_RUNNERS` variable removed as it is confusing and doesn't work on PR's to downstream forks as expected (which is partially what it is for).

760497ac393 gives some hints on what a fork should patch, should they wish to use Cirrus Runners on their own CI.

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