💬 achow101 commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2501632607)
ACK b031b7910d62819443813b91705211c466933a53
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2501632607)
ACK b031b7910d62819443813b91705211c466933a53
💬 TheCharlatan commented on pull request "Miner: never create a template which exploits the timewarp bug":
(https://github.com/bitcoin/bitcoin/pull/31376#issuecomment-2501642097)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31376#issuecomment-2501642097)
Concept ACK
🚀 achow101 merged a pull request: "ci: Split out native fuzz jobs for macOS and windows (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31221)
(https://github.com/bitcoin/bitcoin/pull/31221)
💬 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
(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.
(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)
(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:
...
(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
...
(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.
(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)
...
(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.
(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
(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)
(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).
(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
(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
(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.
(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.
(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
(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.
(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.