💬 kallewoof commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1893281157)
Makes no sense to me. I read through the entire thread in https://github.com/bitcoin/bitcoin/pull/18267 where this was added, and it looks like I put it there right at the end before it was merged, so it may have slipped under the radar.
Anyway, my suggestion: remove the duplication. Try the tests. If the tests fail, we know this at least has a purpose.
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1893281157)
Makes no sense to me. I read through the entire thread in https://github.com/bitcoin/bitcoin/pull/18267 where this was added, and it looks like I put it there right at the end before it was merged, so it may have slipped under the radar.
Anyway, my suggestion: remove the duplication. Try the tests. If the tests fail, we know this at least has a purpose.
👋 Sjors's pull request is ready for review: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283)
(https://github.com/bitcoin/bitcoin/pull/31283)
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2556096604)
#31325 landed so this is ready for review.
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2556096604)
#31325 landed so this is ready for review.
💬 Sjors commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1893337657)
Oh I get it now. There's two nodes for each signet, six in total. So this is fine.
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1893337657)
Oh I get it now. There's two nodes for each signet, six in total. So this is fine.
💬 Sjors commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1893340846)
Thanks for the adding the helper. But now it's inconsistent with the `check_getblockchaininfo` helper. No strong preference for which style to use.
Maybe make it `def check_getmininginfo(self, node_idx, signet_idx):` and `check_getblockchaininfo(self, node_idx, signet_idx) ` and put them above `run_test`.
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1893340846)
Thanks for the adding the helper. But now it's inconsistent with the `check_getblockchaininfo` helper. No strong preference for which style to use.
Maybe make it `def check_getmininginfo(self, node_idx, signet_idx):` and `check_getblockchaininfo(self, node_idx, signet_idx) ` and put them above `run_test`.
💬 kallewoof commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1893345947)
3 sets of pairs of nodes where each pair shares their set of args? OK, that sounds reasonable.
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1893345947)
3 sets of pairs of nodes where each pair shares their set of args? OK, that sounds reasonable.
💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1893346331)
Strange, I have an Ubuntu 24 desktop machine at home with comes with Wayland by default. And it was complaining about libxcb missing. It's possible that I accidentally uninstalled it at some point though.
https://releases.ubuntu.com/noble/ubuntu-24.04.1-desktop-amd64.manifest
It has `libwayland-cursor0` but not `libxcb-cursor0`, so maybe it's a matter of looking for that alternative name?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1893346331)
Strange, I have an Ubuntu 24 desktop machine at home with comes with Wayland by default. And it was complaining about libxcb missing. It's possible that I accidentally uninstalled it at some point though.
https://releases.ubuntu.com/noble/ubuntu-24.04.1-desktop-amd64.manifest
It has `libwayland-cursor0` but not `libxcb-cursor0`, so maybe it's a matter of looking for that alternative name?
💬 Sjors commented on issue "Stratum v2 via IPC Mining Interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-2556161334)
Updated description to hint at some new interface methods I plan to propose in the future: `createFutureBlock()`, `waitNewPowBlock()` and `verifyBlock()`.
However for now let's focus on the last interface method that the Template Provider needs, namely `waitNext()`. And on all the multiprocess changes.
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-2556161334)
Updated description to hint at some new interface methods I plan to propose in the future: `createFutureBlock()`, `waitNewPowBlock()` and `verifyBlock()`.
However for now let's focus on the last interface method that the Template Provider needs, namely `waitNext()`. And on all the multiprocess changes.
💬 Sjors commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2556186288)
@gitgab19 and I did a bit of benchmarking for (3) on a S19k Pro. We sent the machine a new job template every second. The proxy software was modified to keep the share difficulty very low.
The time between share roughly varied from 2-16ms. Presumably following a Poison distribution, but we didn't analyze that.
With `clean_jobs` [0] enabled we found that between sending a new template and getting the first share typically 22-65ms went by.
With `clean_jobs` disabled the delay was 49-65
...
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2556186288)
@gitgab19 and I did a bit of benchmarking for (3) on a S19k Pro. We sent the machine a new job template every second. The proxy software was modified to keep the share difficulty very low.
The time between share roughly varied from 2-16ms. Presumably following a Poison distribution, but we didn't analyze that.
With `clean_jobs` [0] enabled we found that between sending a new template and getting the first share typically 22-65ms went by.
With `clean_jobs` disabled the delay was 49-65
...
👍 rkrux approved a pull request: "wallet: migration, avoid loading legacy wallet after failure when BDB isn't compiled"
(https://github.com/bitcoin/bitcoin/pull/31451#pullrequestreview-2516718199)
ACK 589ed1a8eafe1daed379ebb383602c8f220aef19
(https://github.com/bitcoin/bitcoin/pull/31451#pullrequestreview-2516718199)
ACK 589ed1a8eafe1daed379ebb383602c8f220aef19
🤔 hebasto reviewed a pull request: "test: Embed univalue json tests in binary"
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2516946599)
Approach ACK fabda022536c7a000a575bbb05059d27d29b5cca.
Related: https://github.com/bitcoin/bitcoin/pull/30901 improves handling of data files in build scripts.
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2516946599)
Approach ACK fabda022536c7a000a575bbb05059d27d29b5cca.
Related: https://github.com/bitcoin/bitcoin/pull/30901 improves handling of data files in build scripts.
💬 hebasto commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893581463)
style nit: Follow surrounding style:
```suggestion
test/unitester.cpp
)
```
?
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893581463)
style nit: Follow surrounding style:
```suggestion
test/unitester.cpp
)
```
?
💬 hebasto commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893578552)
nit: Reorder?
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893578552)
nit: Reorder?
💬 hebasto commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893578057)
Drop this one?
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893578057)
Drop this one?
📝 0xB10C opened a pull request: "ci: optionally use local docker build cache"
(https://github.com/bitcoin/bitcoin/pull/31545)
By setting `DOCKER_BUILD_CACHE_HOST_DIR`, the task-specific docker images built during the CI run can be cached. This allows, for example, ephemeral CI runners to reuse the docker images (or layers of it) from earlier runs, by persisting the image cache before the ephemeral CI runner is shut down. The cache keyed by `CONTAINER_NAME`.
As `--cache-to` doesn't remove old cache files, the existing cache is removed after a successful `docker build` and the newly cached image is moved to it's locat
...
(https://github.com/bitcoin/bitcoin/pull/31545)
By setting `DOCKER_BUILD_CACHE_HOST_DIR`, the task-specific docker images built during the CI run can be cached. This allows, for example, ephemeral CI runners to reuse the docker images (or layers of it) from earlier runs, by persisting the image cache before the ephemeral CI runner is shut down. The cache keyed by `CONTAINER_NAME`.
As `--cache-to` doesn't remove old cache files, the existing cache is removed after a successful `docker build` and the newly cached image is moved to it's locat
...
💬 ArisAthena commented on issue "build: broken CMake *flags output":
(https://github.com/bitcoin/bitcoin/issues/31482#issuecomment-2556481051)
Sir, It looks like there's a problem with the compiler flags, particularly the redundancy in the -Werror and -pthread options.
Why not see to it that the flags are correctly formatted and compatible with the compiler version being used. Specifically, checking the $<$<COMPILE_LANG_AND_ID:CUDA,NVIDIA>:SHELL:-Xcompiler -pthread> and $<$<AND:$<NOT:$<COMPILE_LANG_AND_ID:CUDA,NVIDIA>>,$<NOT:$<COMPILE_LANGUAGE:Swift>>>:-pthread> parts for redundancy?
Are the flags compatible with the compiler ve
...
(https://github.com/bitcoin/bitcoin/issues/31482#issuecomment-2556481051)
Sir, It looks like there's a problem with the compiler flags, particularly the redundancy in the -Werror and -pthread options.
Why not see to it that the flags are correctly formatted and compatible with the compiler version being used. Specifically, checking the $<$<COMPILE_LANG_AND_ID:CUDA,NVIDIA>:SHELL:-Xcompiler -pthread> and $<$<AND:$<NOT:$<COMPILE_LANG_AND_ID:CUDA,NVIDIA>>,$<NOT:$<COMPILE_LANGUAGE:Swift>>>:-pthread> parts for redundancy?
Are the flags compatible with the compiler ve
...
💬 vasild commented on pull request "build: enable libc++ and libstdc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1893621353)
Extended the PR description, see the <sup>1</sup> specifically. I wouldn't call it a "downgrade" - it is less checks, but an "upgrade" from performance point of view. All these settings provide varying tradeoffs between performance and amount of checks. Downgrade for one is upgrade for the other.
This is analogous to the `-O` flag. Assuming the default is `-O1` (if no `-O...` is provided). If the user explicitly provides `-O2` that will use `-O2` instead of the default `-O1`. I wouldn't call
...
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1893621353)
Extended the PR description, see the <sup>1</sup> specifically. I wouldn't call it a "downgrade" - it is less checks, but an "upgrade" from performance point of view. All these settings provide varying tradeoffs between performance and amount of checks. Downgrade for one is upgrade for the other.
This is analogous to the `-O` flag. Assuming the default is `-O1` (if no `-O...` is provided). If the user explicitly provides `-O2` that will use `-O2` instead of the default `-O1`. I wouldn't call
...
💬 vasild commented on pull request "build: enable libc++ and libstdc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1893625169)
I removed the `_LIBCPP_ABI_BOUNDED*` options from CMake when `ENABLE_HARDENING=ON` because they caused linking failures for the multiprocess task because libmultiprocess was compiled without them, i.e. ABI incompatibility issues. So, don't enable them on users when `ENABLE_HARDENING=ON`.
> It would be good to have them in a CI task
Right, I changed a few CI tasks to use them, enabling them manually, outside of what CMake does by default, see the second commit and the PR description.
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1893625169)
I removed the `_LIBCPP_ABI_BOUNDED*` options from CMake when `ENABLE_HARDENING=ON` because they caused linking failures for the multiprocess task because libmultiprocess was compiled without them, i.e. ABI incompatibility issues. So, don't enable them on users when `ENABLE_HARDENING=ON`.
> It would be good to have them in a CI task
Right, I changed a few CI tasks to use them, enabling them manually, outside of what CMake does by default, see the second commit and the PR description.
💬 maflcko commented on pull request "build: enable libc++ and libstdc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1893634079)
Why would the other tasks not break the ABI? Just because CI is green doesn't mean the change is correct. Instead of repeating what the changes are doing in the description, it would be more helpful to reviewers to explain why the changes are correct.
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1893634079)
Why would the other tasks not break the ABI? Just because CI is green doesn't mean the change is correct. Instead of repeating what the changes are doing in the description, it would be more helpful to reviewers to explain why the changes are correct.
💬 0xB10C commented on pull request "ci: optionally use local docker build cache":
(https://github.com/bitcoin/bitcoin/pull/31545#issuecomment-2556532955)
Some additional context:
I've been testing and using this with ephemeral cirrus runners I've been working on. See a run of the initial commit of this PR here: https://cirrus-ci.com/build/6017527349772288. If this PR is merged, these could be used on bitcoin/bitcoin after a while - once people have rebased their PRs to include this change. Old branches, for example, 28.x backports won't be able to use this (unless backported) but the docker images on old branches might generally be different a
...
(https://github.com/bitcoin/bitcoin/pull/31545#issuecomment-2556532955)
Some additional context:
I've been testing and using this with ephemeral cirrus runners I've been working on. See a run of the initial commit of this PR here: https://cirrus-ci.com/build/6017527349772288. If this PR is merged, these could be used on bitcoin/bitcoin after a while - once people have rebased their PRs to include this change. Old branches, for example, 28.x backports won't be able to use this (unless backported) but the docker images on old branches might generally be different a
...