💬 maflcko commented on pull request "refactor: Simplify SipHash and fix related benchmarks":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1652865972)
One reason to assign the result is to give the compiler a hint that the result is uses (and should not be discarded). Otherwise, the compiler may optimize away the calculation (and the whole benchmark).
Another reason is to modify the value that is being hashed. Otherwise, the compiler may detect that the same value is hashed in every loop iteration and will only calculate it once, effectively optimizing away the calculation (and the whole benchmark). (While I don't think it matters here, in
...
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1652865972)
One reason to assign the result is to give the compiler a hint that the result is uses (and should not be discarded). Otherwise, the compiler may optimize away the calculation (and the whole benchmark).
Another reason is to modify the value that is being hashed. Otherwise, the compiler may detect that the same value is hashed in every loop iteration and will only calculate it once, effectively optimizing away the calculation (and the whole benchmark). (While I don't think it matters here, in
...
💬 paplorinc commented on pull request "refactor: Simplify SipHash and fix related benchmarks":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1652876883)
Yes, I understand the reason, that's why `doNotOptimizeAway` is needed in some cases.
But here it seems to produce the wrong result. I have validated it with a simple timed test, calculating a million different hashes with the two versions of the algorithm, which were giving different benchmarking speeds - and this new version resembles it more closely.
Also, we're not using this format in any other benchmarks I've seen.
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1652876883)
Yes, I understand the reason, that's why `doNotOptimizeAway` is needed in some cases.
But here it seems to produce the wrong result. I have validated it with a simple timed test, calculating a million different hashes with the two versions of the algorithm, which were giving different benchmarking speeds - and this new version resembles it more closely.
Also, we're not using this format in any other benchmarks I've seen.
👍 TheCharlatan approved a pull request: "guix: build with glibc 2.31"
(https://github.com/bitcoin/bitcoin/pull/29987#pullrequestreview-2138770843)
ACK b5fc6d46a3854c18f6e8dfc89540d24ef778caa2
(https://github.com/bitcoin/bitcoin/pull/29987#pullrequestreview-2138770843)
ACK b5fc6d46a3854c18f6e8dfc89540d24ef778caa2
💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652884524)
> Without this change, when there's no ARM machine, it remains pending forever.
I understand. However, my point is that this is a cosmetic thing. Who cares? The person that sets up a fork, enables cirrus on it, enables a worker pool on it, knowingly does not enable an arm worker on it, so the person should know that the arm task will not run. Whatever the icon in the UI displays should thus not matter.
However, this change may lead to problems. For example, does yaml allow duplicate keys?
...
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652884524)
> Without this change, when there's no ARM machine, it remains pending forever.
I understand. However, my point is that this is a cosmetic thing. Who cares? The person that sets up a fork, enables cirrus on it, enables a worker pool on it, knowingly does not enable an arm worker on it, so the person should know that the arm task will not run. Whatever the icon in the UI displays should thus not matter.
However, this change may lead to problems. For example, does yaml allow duplicate keys?
...
💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652899629)
> It seems like this commit is trying to be more efficient by not running CI twice for branch pushes to pull requests. I think it might also stop the "[ryanofsky/bitcoin] Run failed" emails I currently see when I push branches to my repository.
I'd say it is best to disable the CI in your normal fork, if you care about efficiency and not getting emails. Better, you could create a separate fork/repo with CI enabled, and only push to it if you *really* want to trigger the CI.
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652899629)
> It seems like this commit is trying to be more efficient by not running CI twice for branch pushes to pull requests. I think it might also stop the "[ryanofsky/bitcoin] Run failed" emails I currently see when I push branches to my repository.
I'd say it is best to disable the CI in your normal fork, if you care about efficiency and not getting emails. Better, you could create a separate fork/repo with CI enabled, and only push to it if you *really* want to trigger the CI.
💬 fanquake commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652914864)
> I understand. However, my point is that this is a cosmetic thing. Who cares? The person that sets up a fork, enables cirrus on it, enables a worker pool on it, knowingly does not enable an arm worker on it, so the person should know that the arm task will not run. Whatever the icon in the UI displays should thus not matter.
I agree with Marco. We seem to be spending a lot of time, and happy to take on some complexity just to accommodate use-cases *outside* this repository; for which, as far
...
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652914864)
> I understand. However, my point is that this is a cosmetic thing. Who cares? The person that sets up a fork, enables cirrus on it, enables a worker pool on it, knowingly does not enable an arm worker on it, so the person should know that the arm task will not run. Whatever the icon in the UI displays should thus not matter.
I agree with Marco. We seem to be spending a lot of time, and happy to take on some complexity just to accommodate use-cases *outside* this repository; for which, as far
...
💬 maflcko commented on pull request "refactor: Simplify SipHash and fix related benchmarks":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1652943209)
> But here it seems to produce the wrong result. I have validated it with a simple timed test
I am not sure what you are trying to say. If you want to claim that something is wrong, you will have to provide an explanation or at least steps to reproduce exactly. Otherwise there is nothing that can be done.
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1652943209)
> But here it seems to produce the wrong result. I have validated it with a simple timed test
I am not sure what you are trying to say. If you want to claim that something is wrong, you will have to provide an explanation or at least steps to reproduce exactly. Otherwise there is nothing that can be done.
💬 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`.