💬 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.
📝 TheCharlatan opened a pull request: "kernel: Flush in ChainstateManager destructor"
(https://github.com/bitcoin/bitcoin/pull/31382)
This simplifies the shutdown code a bit and allows for getting rid of the `goto` in `bitcoin-chainstate` as well as making it a bit easier to use for future users of the kernel library.
Moving the second flush out of the Shutdown function into the ChainstateManager destructor, should be safe since there should be no new events created after the first flush. Tweak the chainman tests to actually exercise this new tear down behaviour.
(https://github.com/bitcoin/bitcoin/pull/31382)
This simplifies the shutdown code a bit and allows for getting rid of the `goto` in `bitcoin-chainstate` as well as making it a bit easier to use for future users of the kernel library.
Moving the second flush out of the Shutdown function into the ChainstateManager destructor, should be safe since there should be no new events created after the first flush. Tweak the chainman tests to actually exercise this new tear down behaviour.
💬 maflcko commented on pull request "cmake: Check `-Wno-*` compiler options for `leveldb` target":
(https://github.com/bitcoin/bitcoin/pull/31366#issuecomment-2503864322)
Personally I think it is fine append `-Wno...` in 5 lines of code. I'd presume filtering would require even more code?
(https://github.com/bitcoin/bitcoin/pull/31366#issuecomment-2503864322)
Personally I think it is fine append `-Wno...` in 5 lines of code. I'd presume filtering would require even more code?
💬 fanquake commented on pull request "cmake: Check `-Wno-*` compiler options for `leveldb` target":
(https://github.com/bitcoin/bitcoin/pull/31366#issuecomment-2503869092)
> I'd presume filtering would require even more code?
I don't see why it'd be more than 1 line.
(https://github.com/bitcoin/bitcoin/pull/31366#issuecomment-2503869092)
> I'd presume filtering would require even more code?
I don't see why it'd be more than 1 line.
💬 0xB10C commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1860668423)
I'm using [rootless-docker](https://docs.docker.com/engine/security/rootless/) which runs dockerd as a user account. Inside the container, you're root and can tcpdump on the containers `eth0` interface, but you can't* e.g. mount and edit the hosts `/etc/passwd` like you can't with the user account.
*until someone finds a vuln in rootless-docker
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1860668423)
I'm using [rootless-docker](https://docs.docker.com/engine/security/rootless/) which runs dockerd as a user account. Inside the container, you're root and can tcpdump on the containers `eth0` interface, but you can't* e.g. mount and edit the hosts `/etc/passwd` like you can't with the user account.
*until someone finds a vuln in rootless-docker
📝 maflcko opened a pull request: "test: Add missing node.setmocktime(self.mocktime) to p2p_ibd_stalling.py"
(https://github.com/bitcoin/bitcoin/pull/31383)
This was forgotten by myself in commit fa5b58ea01fac1adb6336b8b6b5217193295c695.
This time, there is a diff to test, which fails on current master and passes with this pull request.
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index e503a68382..16438ebd08 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -112,9 +112,9 @@ static_assert(MAX_BLOCKTXN_DEPTH <= MIN_BLOCKS_TO_KEEP, "MAX_BLOCKTXN_DEPTH too
* want to make this a per-peer adaptive
...
(https://github.com/bitcoin/bitcoin/pull/31383)
This was forgotten by myself in commit fa5b58ea01fac1adb6336b8b6b5217193295c695.
This time, there is a diff to test, which fails on current master and passes with this pull request.
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index e503a68382..16438ebd08 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -112,9 +112,9 @@ static_assert(MAX_BLOCKTXN_DEPTH <= MIN_BLOCKS_TO_KEEP, "MAX_BLOCKTXN_DEPTH too
* want to make this a per-peer adaptive
...
📝 TheCharlatan converted_to_draft a pull request: "kernel: Flush in ChainstateManager destructor"
(https://github.com/bitcoin/bitcoin/pull/31382)
This simplifies the shutdown code a bit and allows for getting rid of the `goto` in `bitcoin-chainstate` as well as making it a bit easier to use for future users of the kernel library.
Moving the second flush out of the Shutdown function into the ChainstateManager destructor, should be safe since there should be no new events created after the first flush. Tweak the chainman tests to actually exercise this new tear down behaviour.
(https://github.com/bitcoin/bitcoin/pull/31382)
This simplifies the shutdown code a bit and allows for getting rid of the `goto` in `bitcoin-chainstate` as well as making it a bit easier to use for future users of the kernel library.
Moving the second flush out of the Shutdown function into the ChainstateManager destructor, should be safe since there should be no new events created after the first flush. Tweak the chainman tests to actually exercise this new tear down behaviour.
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1860714243)
It may be that docker rootless has a different capabilities set, compared to podman. (Can be checked with `capsh --print`).
In any case, my preference would be to explicitly list the required caps, instead of relying on a vendor default.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1860714243)
It may be that docker rootless has a different capabilities set, compared to podman. (Can be checked with `capsh --print`).
In any case, my preference would be to explicitly list the required caps, instead of relying on a vendor default.