💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2501448936)
@achow101 ready for merge?
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2501448936)
@achow101 ready for merge?
💬 yancyribbens commented on pull request "Add coin-grinder example test":
(https://github.com/bitcoin/bitcoin/pull/31352#issuecomment-2501552639)
> If you simply are reproducing the example code, so I tend to concept NACK on adding this to the test, the example is only an example.
That's fair. I wanted a way to follow the example code to trace values during various assignments and conditions. I figured since I wrote the test it might be useful to add, although if not, I can still use it and we can close the PR.
(https://github.com/bitcoin/bitcoin/pull/31352#issuecomment-2501552639)
> If you simply are reproducing the example code, so I tend to concept NACK on adding this to the test, the example is only an example.
That's fair. I wanted a way to follow the example code to trace values during various assignments and conditions. I figured since I wrote the test it might be useful to add, although if not, I can still use it and we can close the PR.
📝 ryanofsky opened a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375)
Intended to make bitcoin command line features more discoverable and allow installing new multiprocess binaries in libexec/ instead of bin/ so they don't cause confusion.
Idea and implementation of this were discussed in https://github.com/bitcoin/bitcoin/issues/30983
(https://github.com/bitcoin/bitcoin/pull/31375)
Intended to make bitcoin command line features more discoverable and allow installing new multiprocess binaries in libexec/ instead of bin/ so they don't cause confusion.
Idea and implementation of this were discussed in https://github.com/bitcoin/bitcoin/issues/30983
💬 achow101 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1859024481)
`blockhash` should be omitted too.
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1859024481)
`blockhash` should be omitted too.
📝 darosior opened a pull request: "Miner: never create a template which exploits the timewarp bug"
(https://github.com/bitcoin/bitcoin/pull/31376)
This check was introduced in #30681 but only enabled for testnet4. To avoid potentially creating an invalid block template if a soft fork to fix the timewarp attack were to activate in the future, we should have this check on all networks. It also seems wise for our miner to not support it whether or not a soft fork activates to fix it at the consensus level.
(https://github.com/bitcoin/bitcoin/pull/31376)
This check was introduced in #30681 but only enabled for testnet4. To avoid potentially creating an invalid block template if a soft fork to fix the timewarp attack were to activate in the future, we should have this check on all networks. It also seems wise for our miner to not support it whether or not a soft fork activates to fix it at the consensus level.
💬 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