💬 willcl-ark commented on issue "Feature Request":
(https://github.com/bitcoin/bitcoin/issues/31380#issuecomment-2503536436)
@dexizer7799
Thanks for taking the time to submit these feature suggestions.
While these are interesting proposals, we need to keep our issue tracker focused on specific, actionable items. These suggestions cover multiple substantial protocol changes that would each require extensive design, discussion, and review processes.
Pull requests that implment any of these features would be welcome for review here, and these would allow the community to properly evaluate the specific implement
...
(https://github.com/bitcoin/bitcoin/issues/31380#issuecomment-2503536436)
@dexizer7799
Thanks for taking the time to submit these feature suggestions.
While these are interesting proposals, we need to keep our issue tracker focused on specific, actionable items. These suggestions cover multiple substantial protocol changes that would each require extensive design, discussion, and review processes.
Pull requests that implment any of these features would be welcome for review here, and these would allow the community to properly evaluate the specific implement
...
🚀 fanquake merged a pull request: "guix: swap `moreutils` for just `sponge`"
(https://github.com/bitcoin/bitcoin/pull/31323)
(https://github.com/bitcoin/bitcoin/pull/31323)
💬 dergoegge commented on pull request "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree":
(https://github.com/bitcoin/bitcoin/pull/31379#issuecomment-2503562852)
> Yes, passing sanitizers flags works on the master branch.
Our MSan ci job [passes C/CXXFLAGS to depends](https://github.com/bitcoin/bitcoin/blob/efdb49afb9e24962958ad4445458ad8da253183b/ci/test/00_setup_env_native_msan.sh#L18) (including non-"-fsanitize=" flags e.g. `-fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls`), why do those flags propagate to secp but the `APPEND_CFLAGS` flags do not? Would it make sense to use the same mechanism for both?
(I'm very confused by all the d
...
(https://github.com/bitcoin/bitcoin/pull/31379#issuecomment-2503562852)
> Yes, passing sanitizers flags works on the master branch.
Our MSan ci job [passes C/CXXFLAGS to depends](https://github.com/bitcoin/bitcoin/blob/efdb49afb9e24962958ad4445458ad8da253183b/ci/test/00_setup_env_native_msan.sh#L18) (including non-"-fsanitize=" flags e.g. `-fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls`), why do those flags propagate to secp but the `APPEND_CFLAGS` flags do not? Would it make sense to use the same mechanism for both?
(I'm very confused by all the d
...
💬 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-2503596845)
It occurred to me that the local cache needs to be keyed by e.g. `${CONTAINER_NAME}` since different tasks build different images. This wasn't clear to me from the build cache documentation at first. Setting `CI_IMAGE_BUILD_EXTRA_ARGS="--cache-to type=local,dest=/cache/docker/${CONTAINER_NAME}"` doesn't work, as bash does not expand variables in variables. I'll mark this as draft for now until I find a solution.
> However, the images are quite large (especially msan), so my worry would be tha
...
(https://github.com/bitcoin/bitcoin/pull/31377#issuecomment-2503596845)
It occurred to me that the local cache needs to be keyed by e.g. `${CONTAINER_NAME}` since different tasks build different images. This wasn't clear to me from the build cache documentation at first. Setting `CI_IMAGE_BUILD_EXTRA_ARGS="--cache-to type=local,dest=/cache/docker/${CONTAINER_NAME}"` doesn't work, as bash does not expand variables in variables. I'll mark this as draft for now until I find a solution.
> However, the images are quite large (especially msan), so my worry would be tha
...
📝 0xB10C converted_to_draft 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.
💬 maflcko 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-2503617213)
> It occurred to me that the local cache needs to be keyed by e.g. `${CONTAINER_NAME}` since different tasks build different images.
Are you sure? The final layers should be different for each task, so should never collide. (Possibly early layers may be shared, if they happen to be the same). If this was an issue, shouldn't the CI normally fail when running several tasks on the same machine in the same user account?
> After the build, overwrite `/path/to/docker/build/cache/${CONTAINER_
...
(https://github.com/bitcoin/bitcoin/pull/31377#issuecomment-2503617213)
> It occurred to me that the local cache needs to be keyed by e.g. `${CONTAINER_NAME}` since different tasks build different images.
Are you sure? The final layers should be different for each task, so should never collide. (Possibly early layers may be shared, if they happen to be the same). If this was an issue, shouldn't the CI normally fail when running several tasks on the same machine in the same user account?
> After the build, overwrite `/path/to/docker/build/cache/${CONTAINER_
...
💬 0xB10C commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1860486568)
> (unless they are switched to @0xB10C's workers, which are running as root?)
the runner setup I'm working on explicitly **doesn't** run as root and is far from finished :)
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1860486568)
> (unless they are switched to @0xB10C's workers, which are running as root?)
the runner setup I'm working on explicitly **doesn't** run as root and is far from finished :)
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1860526331)
> the runner setup I'm working on explicitly **doesn't** run as root
Are you sure, because the current CI (in this run) is run in a user account (not root), and gives a permission error. The same CI in your run does not give a permission error, so there seems to be a difference.
The only thing I see is that you are using docker, which IIRC is running rootful by default.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1860526331)
> the runner setup I'm working on explicitly **doesn't** run as root
Are you sure, because the current CI (in this run) is run in a user account (not root), and gives a permission error. The same CI in your run does not give a permission error, so there seems to be a difference.
The only thing I see is that you are using docker, which IIRC is running rootful by default.
👍 scgbckbone approved a pull request: "test: Add `leaf_version` parameter to `taproot_tree_helper()`"
(https://github.com/bitcoin/bitcoin/pull/29371#pullrequestreview-2464807260)
ACK
(https://github.com/bitcoin/bitcoin/pull/29371#pullrequestreview-2464807260)
ACK
📝 sky-coderay opened a pull request: "Update init.md"
(https://github.com/bitcoin/bitcoin/pull/31381)
###Motivation for the patch:
The change corrects a spelling error in the documentation, where "behaviour" was mistakenly used instead of the correct American English spelling "behavior." Since Bitcoin Core's official documentation follows American English conventions, this fix ensures consistency and accuracy, making the documentation clearer and more professional for users and developers.
(https://github.com/bitcoin/bitcoin/pull/31381)
###Motivation for the patch:
The change corrects a spelling error in the documentation, where "behaviour" was mistakenly used instead of the correct American English spelling "behavior." Since Bitcoin Core's official documentation follows American English conventions, this fix ensures consistency and accuracy, making the documentation clearer and more professional for users and developers.
💬 willcl-ark commented on pull request "Update init.md":
(https://github.com/bitcoin/bitcoin/pull/31381#issuecomment-2503726337)
Please don't contribute these AI-generated fixes here in the future, thank you.
(https://github.com/bitcoin/bitcoin/pull/31381#issuecomment-2503726337)
Please don't contribute these AI-generated fixes here in the future, thank you.
💬 fanquake commented on pull request "cmake: Check `-Wno-*` compiler options for `leveldb` target":
(https://github.com/bitcoin/bitcoin/pull/31366#issuecomment-2503734949)
In Autotools we filtered out the unwanted flags, ifor CMake, the `-Wno-*` variants were added (but in a way that didn't quite work), and this PR fixes their usage by adding checks for the flags. However that means we duplicate checks that we do for the same flags earlier in the buildsystem, and we end up with something like:
`clang++ ... -Wconditional-uninitialized -Wsuggest-override ... -Wno-conditional-uninitialized -Wno-suggest-override`
when building leveldb. Wondering if we could have
...
(https://github.com/bitcoin/bitcoin/pull/31366#issuecomment-2503734949)
In Autotools we filtered out the unwanted flags, ifor CMake, the `-Wno-*` variants were added (but in a way that didn't quite work), and this PR fixes their usage by adding checks for the flags. However that means we duplicate checks that we do for the same flags earlier in the buildsystem, and we end up with something like:
`clang++ ... -Wconditional-uninitialized -Wsuggest-override ... -Wno-conditional-uninitialized -Wno-suggest-override`
when building leveldb. Wondering if we could have
...
💬 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-2503777192)
> > It occurred to me that the local cache needs to be keyed by e.g. `${CONTAINER_NAME}` since different tasks build different images.
>
> Are you sure? The final layers should be different for each task, so should never collide. (Possibly early layers may be shared, if they happen to be the same). If this was an issue, shouldn't the CI normally fail when running several tasks on the same machine in the same user account?
I'm not sure if we are talking about the same thing. I mean the `doc
...
(https://github.com/bitcoin/bitcoin/pull/31377#issuecomment-2503777192)
> > It occurred to me that the local cache needs to be keyed by e.g. `${CONTAINER_NAME}` since different tasks build different images.
>
> Are you sure? The final layers should be different for each task, so should never collide. (Possibly early layers may be shared, if they happen to be the same). If this was an issue, shouldn't the CI normally fail when running several tasks on the same machine in the same user account?
I'm not sure if we are talking about the same thing. I mean the `doc
...
🤔 willcl-ark reviewed a pull request: "RFC: build: support for pre-compiled headers."
(https://github.com/bitcoin/bitcoin/pull/31053#pullrequestreview-2462100101)
Took a first pass over this. It seems to work for me locally, both with and without `ccache`.
I see both clang and gcc running faster with this change than merge-base @ 5fb94550638 (with clang being much faster than gcc in both cases):
```log
Summary
env CCACHE_DISABLE=1 CC=clang CXX=clang++ just build (commit = 3cec3508a79c08db5b97e93bc4225ccdd7d32ad8) ran
1.32 ± 0.02 times faster than env CCACHE_DISABLE=1 CC=clang CXX=clang++ just build (commit = 5fb94550638)
1.43 ± 0.00 ti
...
(https://github.com/bitcoin/bitcoin/pull/31053#pullrequestreview-2462100101)
Took a first pass over this. It seems to work for me locally, both with and without `ccache`.
I see both clang and gcc running faster with this change than merge-base @ 5fb94550638 (with clang being much faster than gcc in both cases):
```log
Summary
env CCACHE_DISABLE=1 CC=clang CXX=clang++ just build (commit = 3cec3508a79c08db5b97e93bc4225ccdd7d32ad8) ran
1.32 ± 0.02 times faster than env CCACHE_DISABLE=1 CC=clang CXX=clang++ just build (commit = 5fb94550638)
1.43 ± 0.00 ti
...
💬 willcl-ark commented on pull request "RFC: build: support for pre-compiled headers.":
(https://github.com/bitcoin/bitcoin/pull/31053#discussion_r1858875514)
They also give [GCC flags](https://ccache.dev/manual/latest.html#_precompiled_headers). Perhaps here we can therefore use something like:
```patch
diff --git a/cmake/ccache.cmake b/cmake/ccache.cmake
index 08f078e627b..cb7059884fa 100644
--- a/cmake/ccache.cmake
+++ b/cmake/ccache.cmake
@@ -35,7 +35,12 @@ if(NOT MSVC)
if(NOT CCACHE_TEST_RESULT)
list(APPEND CMAKE_CXX_COMPILER_LAUNCHER sloppiness=pch_defines,time_macros)
# According to the ccache docs clang requ
...
(https://github.com/bitcoin/bitcoin/pull/31053#discussion_r1858875514)
They also give [GCC flags](https://ccache.dev/manual/latest.html#_precompiled_headers). Perhaps here we can therefore use something like:
```patch
diff --git a/cmake/ccache.cmake b/cmake/ccache.cmake
index 08f078e627b..cb7059884fa 100644
--- a/cmake/ccache.cmake
+++ b/cmake/ccache.cmake
@@ -35,7 +35,12 @@ if(NOT MSVC)
if(NOT CCACHE_TEST_RESULT)
list(APPEND CMAKE_CXX_COMPILER_LAUNCHER sloppiness=pch_defines,time_macros)
# According to the ccache docs clang requ
...
💬 hebasto commented on pull request "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree":
(https://github.com/bitcoin/bitcoin/pull/31379#issuecomment-2503808995)
> > Yes, passing sanitizers flags works on the master branch.
>
> Our MSan ci job [passes C/CXXFLAGS to depends](https://github.com/bitcoin/bitcoin/blob/efdb49afb9e24962958ad4445458ad8da253183b/ci/test/00_setup_env_native_msan.sh#L18) (including non-"-fsanitize=" flags e.g. `-fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls`), why do those flags propagate to secp but the `APPEND_CFLAGS` flags do not?
Flags set in depends are propagated via the toolchain file.
> Would it make se
...
(https://github.com/bitcoin/bitcoin/pull/31379#issuecomment-2503808995)
> > Yes, passing sanitizers flags works on the master branch.
>
> Our MSan ci job [passes C/CXXFLAGS to depends](https://github.com/bitcoin/bitcoin/blob/efdb49afb9e24962958ad4445458ad8da253183b/ci/test/00_setup_env_native_msan.sh#L18) (including non-"-fsanitize=" flags e.g. `-fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls`), why do those flags propagate to secp but the `APPEND_CFLAGS` flags do not?
Flags set in depends are propagated via the toolchain file.
> Would it make se
...
💬 hebasto commented on pull request "cmake: Check `-Wno-*` compiler options for `leveldb` target":
(https://github.com/bitcoin/bitcoin/pull/31366#issuecomment-2503838418)
> However that means we duplicate checks that we do for the same flags earlier in the buildsystem...
Checks are cached. You can observe only a single instance of:
```
-- Performing Test CXX_SUPPORTS__WSUGGEST_OVERRIDE
-- Performing Test CXX_SUPPORTS__WSUGGEST_OVERRIDE - Success
```
in the configuration output.
(https://github.com/bitcoin/bitcoin/pull/31366#issuecomment-2503838418)
> However that means we duplicate checks that we do for the same flags earlier in the buildsystem...
Checks are cached. You can observe only a single instance of:
```
-- Performing Test CXX_SUPPORTS__WSUGGEST_OVERRIDE
-- Performing Test CXX_SUPPORTS__WSUGGEST_OVERRIDE - Success
```
in the configuration output.
💬 fanquake commented on pull request "cmake: Check `-Wno-*` compiler options for `leveldb` target":
(https://github.com/bitcoin/bitcoin/pull/31366#issuecomment-2503841439)
The code is duplicated.
(https://github.com/bitcoin/bitcoin/pull/31366#issuecomment-2503841439)
The code is duplicated.
📝 brunoerg converted_to_draft a pull request: "fuzz: wallet: add target for spkm migration"
(https://github.com/bitcoin/bitcoin/pull/29694)
This PR adds a fuzz target for `ScriptPubKeyMan` migration. It creates a `LegacyDataSPKM` which can have some keys/scripts/etc, and then migrate it to descriptor. I tried to keep it as compatible as possible with future legacy wallet removal.
(https://github.com/bitcoin/bitcoin/pull/29694)
This PR adds a fuzz target for `ScriptPubKeyMan` migration. It creates a `LegacyDataSPKM` which can have some keys/scripts/etc, and then migrate it to descriptor. I tried to keep it as compatible as possible with future legacy wallet removal.