💬 fanquake commented on pull request "Update libsecp256k1 subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/30334#issuecomment-2189118953)
> Depending on whether https://github.com/bitcoin-core/secp256k1/pull/1535 goes in, we could also pull that, but just a cleanup.
Incorporated.
(https://github.com/bitcoin/bitcoin/pull/30334#issuecomment-2189118953)
> Depending on whether https://github.com/bitcoin-core/secp256k1/pull/1535 goes in, we could also pull that, but just a cleanup.
Incorporated.
💬 maflcko commented on pull request "refactor: Simplify SipHash and fix related benchmarks":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1652946568)
Also, mixing "fixes" with "refactor" is wrong anyway. A commit should either be a fix or a refactor or a feature change. This is explained in the contrib guide in this repo.
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1652946568)
Also, mixing "fixes" with "refactor" is wrong anyway. A commit should either be a fix or a refactor or a feature change. This is explained in the contrib guide in this repo.
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652953447)
It's confusing for both the pull request author and reviewers. Only the fork maintainer will know what's going on, but they also need to carefully check each time that it's only the expected job that's stuck. A single green check, when all jobs pass, is more clear.
> does yaml allow duplicate keys
Not sure.
So the simplest solution for now is to drop 859e1c558fd9c8604fda361e465aa4eb4a676823 and make qemu "mandatory", which works for me.
> just to accommodate use-cases outside this re
...
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652953447)
It's confusing for both the pull request author and reviewers. Only the fork maintainer will know what's going on, but they also need to carefully check each time that it's only the expected job that's stuck. A single green check, when all jobs pass, is more clear.
> does yaml allow duplicate keys
Not sure.
So the simplest solution for now is to drop 859e1c558fd9c8604fda361e465aa4eb4a676823 and make qemu "mandatory", which works for me.
> just to accommodate use-cases outside this re
...
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2189134256)
Dropped the `NO_ARM` commit as well, see https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652953447
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2189134256)
Dropped the `NO_ARM` commit as well, see https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652953447
💬 TheCharlatan commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#discussion_r1652956075)
Could this just always be disabled?
(https://github.com/bitcoin/bitcoin/pull/29835#discussion_r1652956075)
Could this just always be disabled?
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652962963)
But this commit is dropped now.
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652962963)
But this commit is dropped now.
💬 paplorinc commented on pull request "refactor: Simplify SipHash and fix related benchmarks":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1652964172)
You're right, that's why it's still a draft. Thanks for the early input.
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1652964172)
You're right, that's why it's still a draft. Thanks for the early input.
👍 TheCharlatan approved a pull request: "Update libsecp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/30334#pullrequestreview-2138913915)
ACK cc58e958f341d2759fbabe5c9d8cc557e17d587f
(https://github.com/bitcoin/bitcoin/pull/30334#pullrequestreview-2138913915)
ACK cc58e958f341d2759fbabe5c9d8cc557e17d587f
🤔 real-or-random reviewed a pull request: "Update libsecp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/30334#pullrequestreview-2138914790)
Concept ACK -- this is a particularly harmless update as all changes are in the tests or in the build system or in comments
(https://github.com/bitcoin/bitcoin/pull/30334#pullrequestreview-2138914790)
Concept ACK -- this is a particularly harmless update as all changes are in the tests or in the build system or in comments
💬 fanquake commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2189167818)
> Maintainer note: NO_BRANCH=true must be set in Cirrus for bitcoin-core/gui before merging this. See https://cirrus-ci.com/github/bitcoin-core/gui -> Settings.
Can you rename this to something that actually explains what it's doing; `NO_BRANCH` is incredibly vauge. Why not `DONT_RUN_CI_ON_BRANCH_PUSH_ON_THIS_FORK` etc? Then someone looking at the settings in Cirrus will actually know what this is doing.
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2189167818)
> Maintainer note: NO_BRANCH=true must be set in Cirrus for bitcoin-core/gui before merging this. See https://cirrus-ci.com/github/bitcoin-core/gui -> Settings.
Can you rename this to something that actually explains what it's doing; `NO_BRANCH` is incredibly vauge. Why not `DONT_RUN_CI_ON_BRANCH_PUSH_ON_THIS_FORK` etc? Then someone looking at the settings in Cirrus will actually know what this is doing.
💬 fanquake commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#discussion_r1653002407)
Probably. Happy to just drop this, and it could always be added later.
(https://github.com/bitcoin/bitcoin/pull/29835#discussion_r1653002407)
Probably. Happy to just drop this, and it could always be added later.
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2189207922)
> a highly specialized process (I think we're only expecting a small minority of nodes to be running SV2?)
I'd like to see a lot more users mine, and do so with their own block template. In order to achieve that we have to lower the barrier to entry.
There's a generic case to be made for reducing the scope of Bitcoin Core, and not further increasing it. I think we still lack the tooling for this. Something like a plugin system, or a convenient way to ship a release tag plus custom function
...
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2189207922)
> a highly specialized process (I think we're only expecting a small minority of nodes to be running SV2?)
I'd like to see a lot more users mine, and do so with their own block template. In order to achieve that we have to lower the barrier to entry.
There's a generic case to be made for reducing the scope of Bitcoin Core, and not further increasing it. I think we still lack the tooling for this. Something like a plugin system, or a convenient way to ship a release tag plus custom function
...
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2189223926)
Renamed `NO_BRANCH` to `SKIP_BRANCH_PUSH`.
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2189223926)
Renamed `NO_BRANCH` to `SKIP_BRANCH_PUSH`.
👍 stickies-v approved a pull request: "contrib: Fixup verify-binaries OS platform parsing"
(https://github.com/bitcoin/bitcoin/pull/30147#pullrequestreview-2138611736)
ACK 4a85b154d0874f7bfa848ae8d7ab341eea754607 - nice work
(https://github.com/bitcoin/bitcoin/pull/30147#pullrequestreview-2138611736)
ACK 4a85b154d0874f7bfa848ae8d7ab341eea754607 - nice work
💬 stickies-v commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1652784235)
meta nit: although consistency is good, changing otherwise unaffected lines/functions just to do an unnecessary rename can be counterproductive, so this would have been fine to keep as is
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1652784235)
meta nit: although consistency is good, changing otherwise unaffected lines/functions just to do an unnecessary rename can be counterproductive, so this would have been fine to keep as is
👍 vasild approved a pull request: "net: Allow -proxy=[::1] on nodes with IPV6 lo only"
(https://github.com/bitcoin/bitcoin/pull/30245#pullrequestreview-2139044652)
ACK 60a753b7b38fbe89494f8df66f13a84f28af244b
Thank you!
(https://github.com/bitcoin/bitcoin/pull/30245#pullrequestreview-2139044652)
ACK 60a753b7b38fbe89494f8df66f13a84f28af244b
Thank you!
✅ fanquake closed a pull request: "util/system: Close non-std fds when execing slave processes"
(https://github.com/bitcoin/bitcoin/pull/22417)
(https://github.com/bitcoin/bitcoin/pull/22417)
💬 fanquake commented on pull request "util/system: Close non-std fds when execing slave processes":
(https://github.com/bitcoin/bitcoin/pull/22417#issuecomment-2189291497)
Closing, see above, and this is adding Boost Process code, which no-longer exists in this codebase.
(https://github.com/bitcoin/bitcoin/pull/22417#issuecomment-2189291497)
Closing, see above, and this is adding Boost Process code, which no-longer exists in this codebase.
💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2189293850)
`6d9083b249...45f4dbe484`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2189293850)
`6d9083b249...45f4dbe484`: rebase due to conflicts
📝 fanquake converted_to_draft a pull request: "fuzz: wallet, add target for Spend"
(https://github.com/bitcoin/bitcoin/pull/28236)
This PR adds fuzz coverage for `wallet/spend`.
Motivation: Issue [27272](https://github.com/bitcoin/bitcoin/issues/27272#issue-1628327906)
This PR adds Fuzz coverage to the whole concept of Creating a New Transaction as well as other sections of the Spend file. Because `CreateTransaction` is one of the most frequently used functions in the wallet codebase, merging this PR will significantly improve the wallet codebase's Fuzz testing!
I also used the `Singleton Class` concept for creati
...
(https://github.com/bitcoin/bitcoin/pull/28236)
This PR adds fuzz coverage for `wallet/spend`.
Motivation: Issue [27272](https://github.com/bitcoin/bitcoin/issues/27272#issue-1628327906)
This PR adds Fuzz coverage to the whole concept of Creating a New Transaction as well as other sections of the Spend file. Because `CreateTransaction` is one of the most frequently used functions in the wallet codebase, merging this PR will significantly improve the wallet codebase's Fuzz testing!
I also used the `Singleton Class` concept for creati
...