💬 achow101 commented on pull request "interpreter: Use the same type for SignatureHash in the definition":
(https://github.com/bitcoin/bitcoin/pull/31365#issuecomment-2501660248)
ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f

💬 achow101 commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1859064957)
In c32104f5449bbea89a3fde5b99c97f7de6914829 "gen-manpages: Update default build path"

The documentation states that this script can work from any directory for in-tree builds, but this change would break that (although we do not currently do in-tree builds). It further states that `BUILDDIR` should be set for out-of-tree builds. Either the documentation should be updated, or this default should not be changed. I prefer the latter.

🚀 achow101 merged a pull request: "interpreter: Use the same type for SignatureHash in the definition"
(https://github.com/bitcoin/bitcoin/pull/31365)

💬 darosior commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1859092261)
I do not have the context nor the bug report behind this fix, but #10146 is clearly preventing a crash on accessing `block->vtx[0]` when an empty block is submitted through `submitblock`. In addition the introduction of the check for a valid coinbase transaction smells like preventing https://gnusha.org/url/?https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-February/016697.html but i can't think of how it could be exploited.

Typically you would think of an attack along these lines:
...

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1859111798)
Yes, that's right. In the current revision, if the set is full and the given transaction has some ancestors, the children will be flooded while the parents will be reconciled, which may cause orphans.

I think we could handle this at the same level that we handled `fanout_with_ancestors`:

- Fanout to peers that have ancestors and have a full set
- Fanout to peers that have the least amount of ancestors

However, this will still be an issue in some cases, given we only select a subset of
...

📝 0xB10C opened a pull request: "ci: allow passing extra args to docker build with CI_IMAGE_BUILD_EXTRA_ARGS"
(https://github.com/bitcoin/bitcoin/pull/31377)
By setting `CI_IMAGE_BUILD_EXTRA_ARGS`, a CI runner can influence the docker build of the CI container image. This allows to, for example, pass `--cache-to` and `--cache-from` to the build, which is useful for ephemeral, self-hosted CI runners.

💬 0xB10C commented on pull request "ci: allow passing extra args to docker build with CI_IMAGE_BUILD_EXTRA_ARGS":
(https://github.com/bitcoin/bitcoin/pull/31377#issuecomment-2501746096)

I've been looking into setting up my own ephemeral CI runners. Ideally, these would cache the CI container images that we build in the CI. By setting `CI_IMAGE_BUILD_EXTRA_ARGS` to `--cache-to type=local,dest=/cache/docker,mode=max --cache-from type=local,src=/cache/docker` (see https://docs.docker.com/build/cache/backends/local/) I can cache the images (and reusable layers of the images) for `docker build`. This is both nice for a faster build (the `docker build` in the MSan task takes > 1h)
...

🤔 furszy reviewed a pull request: "fuzz: wallet: add target for spkm migration"
(https://github.com/bitcoin/bitcoin/pull/29694#pullrequestreview-2462527071)
Would be good to ensure that #31374 can be replicated here.

💬 achow101 commented on pull request "refactor: Fix remaining clang-tidy performance-inefficient-vector errors":
(https://github.com/bitcoin/bitcoin/pull/31305#issuecomment-2501795762)
ACK 11f3bc229ccd4b20191855fb1df882cfa6145264

🚀 achow101 merged a pull request: "refactor: Fix remaining clang-tidy performance-inefficient-vector errors"
(https://github.com/bitcoin/bitcoin/pull/31305)

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1859182705)
I think I'm not following here. We do apply the delayed set both on addition (`AddToSet`), deletion (`RemoveFromSet`), and when counting parents (this bit of code is part of the parent sorting).

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1859187630)
Sure. This also implies dropping the bench for it, which honestly I do not think is as useful anymore now that we do not have a cache

💬 achow101 commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#issuecomment-2501899041)
ACK 3305972f7bfd78181566e4297891c2dd7cae0f2b

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#discussion_r1859205128)
Done.

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#discussion_r1859205308)
Done.

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2501906569)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/29694#discussion_r1828422802

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2501907189)
> Would be good to ensure that https://github.com/bitcoin/bitcoin/pull/31374 can be replicated here.

Since this target is only for spkm migration, I don't think it can be replicated here.

🤔 ismaelsadeeq reviewed a pull request: "validation: fetch block inputs on parallel threads ~17% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-2462618817)
Concept ACK
This is nice. Although I have not yet benchmarked this branch, I also like @furszy's idea of having a general-purpose thread pool.

I just have one test improvement comment, question and a nit after first pass of the PR

💬 ismaelsadeeq commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1859189838)
In 3c201bcffc1d7e382e8afa9a88750a4c261c1cf8 "tests: add inputfetcher tests"
You can set this up in `InputFetcherTest` so that you don't have to repeat it in the rest of the tests.

💬 ismaelsadeeq commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1859192217)
In 2349ac7d6071746a80223358bce0d5e556b277d7 "bench: add inputfetcher bench"
How did you select this batch size?