π¬ hebasto commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1935815953)
Thanks!
I do not really have an explanation for this change...
We definitely should honour user-provided `CMAKE_<LANG>_COMPILER_LAUNCHER` environment variables if any.
The appending has been restored.
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1935815953)
Thanks!
I do not really have an explanation for this change...
We definitely should honour user-provided `CMAKE_<LANG>_COMPILER_LAUNCHER` environment variables if any.
The appending has been restored.
π¬ stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1935834281)
The implicit βstd::filesystem::__cxx11::pathβ to βconst std::string&β conversion doesn't seem to cross-compile for `x86_64-w64-mingw32`:
```
[100%] Building CXX object src/kernel/CMakeFiles/kernel-bitcoin-chainstate.dir/bitcoin-chainstate.cpp.obj
/home/runner/work/py-bitcoinkernel/py-bitcoinkernel/depend/bitcoin/src/kernel/bitcoin-chainstate.cpp: In function βint main(int, char**)β:
/home/runner/work/py-bitcoinkernel/py-bitcoinkernel/depend/bitcoin/src/kernel/bitcoin-chainstate.cpp:164:64:
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1935834281)
The implicit βstd::filesystem::__cxx11::pathβ to βconst std::string&β conversion doesn't seem to cross-compile for `x86_64-w64-mingw32`:
```
[100%] Building CXX object src/kernel/CMakeFiles/kernel-bitcoin-chainstate.dir/bitcoin-chainstate.cpp.obj
/home/runner/work/py-bitcoinkernel/py-bitcoinkernel/depend/bitcoin/src/kernel/bitcoin-chainstate.cpp: In function βint main(int, char**)β:
/home/runner/work/py-bitcoinkernel/py-bitcoinkernel/depend/bitcoin/src/kernel/bitcoin-chainstate.cpp:164:64:
...
π¬ hebasto commented on pull request "doc: update translation generation cmake example":
(https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1935836999)
> What about the MULTIPROCESS option?
It does modify source list, for example:https://github.com/bitcoin/bitcoin/blob/809d7e763cc9bdfff3288860a1c530460c76ffff/src/qt/CMakeLists.txt#L244-L248
So, potentially disabling it could lead to missed translations.
> I cannot yet run this on my Mac or on Linux, unless I disable it.
I always build depends with `MULTIPROCESS=1` before configuring and building the `translate` target`.
(https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1935836999)
> What about the MULTIPROCESS option?
It does modify source list, for example:https://github.com/bitcoin/bitcoin/blob/809d7e763cc9bdfff3288860a1c530460c76ffff/src/qt/CMakeLists.txt#L244-L248
So, potentially disabling it could lead to missed translations.
> I cannot yet run this on my Mac or on Linux, unless I disable it.
I always build depends with `MULTIPROCESS=1` before configuring and building the `translate` target`.
π¬ l0rinc commented on pull request "doc: update translation generation cmake example":
(https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1935841590)
Thanks, I'll figure out how to make it work with MULTIPROCESS next time I need to do the translations :).
So is the PR correct as it is now?
(https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1935841590)
Thanks, I'll figure out how to make it work with MULTIPROCESS next time I need to do the translations :).
So is the PR correct as it is now?
π¬ TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1935842536)
Good catch, I will try add the tests and chainstate binary to the CI here too.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1935842536)
Good catch, I will try add the tests and chainstate binary to the CI here too.
π¬ Eunovo commented on pull request "Ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-2624877742)
> I'm not sure whether `LoadingBlocks` is the correct way to solve it or not (I would need someone else to opine on that, I can't tell if this can err on the side of not checking the scripts at all for some edge case).
I agree too. `LoadingBlocks()` is `true` when blocks are being reindexed or when new blocks are being loaded with `-loadblocks` . I intend to move away from `LoadingBlocks()` to something that's only true when blockfiles are being reindexed.
> I also checked that the new `
...
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-2624877742)
> I'm not sure whether `LoadingBlocks` is the correct way to solve it or not (I would need someone else to opine on that, I can't tell if this can err on the side of not checking the scripts at all for some edge case).
I agree too. `LoadingBlocks()` is `true` when blocks are being reindexed or when new blocks are being loaded with `-loadblocks` . I intend to move away from `LoadingBlocks()` to something that's only true when blockfiles are being reindexed.
> I also checked that the new `
...
π¬ Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2624897679)
It didn't work for the "test each commit" test, but that makes sense because the fix itself isn't introduced early enough. That should go away once the base PR is merged.
Looks like we're left with the tidy failure and https://github.com/chaincodelabs/libmultiprocess/issues/138.
(two tests are still running though...)
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2624897679)
It didn't work for the "test each commit" test, but that makes sense because the fix itself isn't introduced early enough. That should go away once the base PR is merged.
Looks like we're left with the tidy failure and https://github.com/chaincodelabs/libmultiprocess/issues/138.
(two tests are still running though...)
π¬ ryanofsky commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2624927160)
You mentioned the previous releases job was failing and now it is passing? Was this also the mptest bug?
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2624927160)
You mentioned the previous releases job was failing and now it is passing? Was this also the mptest bug?
π¬ ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2624942251)
I wonder if python test framework changes here are scaring reviewers from this PR? I potentially could drop the framework integration here and move it into a different PR. Maybe add just a more limited test here instead.
Would be good to know either way.
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2624942251)
I wonder if python test framework changes here are scaring reviewers from this PR? I potentially could drop the framework integration here and move it into a different PR. Maybe add just a more limited test here instead.
Would be good to know either way.
π TheCharlatan approved a pull request: "build: Enhance Ccache performance across worktrees and build trees"
(https://github.com/bitcoin/bitcoin/pull/30861#pullrequestreview-2584377516)
ACK aeb3977db51792d8ad22b0a5ff8fc5ff20d69a00
(https://github.com/bitcoin/bitcoin/pull/30861#pullrequestreview-2584377516)
ACK aeb3977db51792d8ad22b0a5ff8fc5ff20d69a00
π¬ sipa commented on issue "RFC: multiprocess binaries in 29.0":
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2624952074)
I'm wondering if everyone in this discussion is using the term "experimental" with the same connotation. I feel like in the past we have fairly regularly had "experimental" features, in the sense that we didn't want to commit to this feature being a stable interface, but not in the sense that it had a lower bar of review (for the purpose of code quality, review, ...).
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2624952074)
I'm wondering if everyone in this discussion is using the term "experimental" with the same connotation. I feel like in the past we have fairly regularly had "experimental" features, in the sense that we didn't want to commit to this feature being a stable interface, but not in the sense that it had a lower bar of review (for the purpose of code quality, review, ...).
π¬ Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2624956309)
> You mentioned the previous releases job was failing and now it is passing? Was this also the mptest bug?
No, it was having difficulty downloading capnp, maybe it was spurious.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2624956309)
> You mentioned the previous releases job was failing and now it is passing? Was this also the mptest bug?
No, it was having difficulty downloading capnp, maybe it was spurious.
π¬ m3dwards commented on issue "build: macOS fuzz instructions broken using latest macOS linker":
(https://github.com/bitcoin/bitcoin/issues/31049#issuecomment-2624979670)
I was able to compile with Clang 19.1.7 by using LLVM's linker `lld`.
```shell
brew install lld
cmake --preset=libfuzzer -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" -DAPPEND_LDFLAGS=-Wl,-no_warn_duplicate_libraries -DCMAKE_CXX_FLAGS="-fuse-ld=lld"
```
```shell
[100%] Linking CXX executable fuzz
[100%] Built target fuzz
```
A note that clang 19.1.7's sanitizers required a minimum OSX target of 14.7.
(https://github.com/bitcoin/bitcoin/issues/31049#issuecomment-2624979670)
I was able to compile with Clang 19.1.7 by using LLVM's linker `lld`.
```shell
brew install lld
cmake --preset=libfuzzer -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" -DAPPEND_LDFLAGS=-Wl,-no_warn_duplicate_libraries -DCMAKE_CXX_FLAGS="-fuse-ld=lld"
```
```shell
[100%] Linking CXX executable fuzz
[100%] Built target fuzz
```
A note that clang 19.1.7's sanitizers required a minimum OSX target of 14.7.
π¬ ryanofsky commented on issue "RFC: multiprocess binaries in 29.0":
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2625046815)
I can't speak for anyone else but I am using "experimental" to mean that the feature is new and has not had as much testing as other features because it is new. Therefore, it is not recommended for widespread use, but is recommended for use by people who want to experiment and help improve it.
> I feel like in the past we have fairly regularly had "experimental" features, in the sense that we didn't want to commit to this feature being a stable interface
Right this is not really what what I am
...
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2625046815)
I can't speak for anyone else but I am using "experimental" to mean that the feature is new and has not had as much testing as other features because it is new. Therefore, it is not recommended for widespread use, but is recommended for use by people who want to experiment and help improve it.
> I feel like in the past we have fairly regularly had "experimental" features, in the sense that we didn't want to commit to this feature being a stable interface
Right this is not really what what I am
...
π¬ achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2625046923)
@fanquake @pinheadmz Can one of you please do a build and make detached sigs for this PR for testing?
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2625046923)
@fanquake @pinheadmz Can one of you please do a build and make detached sigs for this PR for testing?
π¬ Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2625064735)
@ryanofsky getting a 403 again this time on a different job: https://cirrus-ci.com/task/6262892556713984?logs=ci#L1459
This might just be an availability issue?
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2625064735)
@ryanofsky getting a 403 again this time on a different job: https://cirrus-ci.com/task/6262892556713984?logs=ci#L1459
This might just be an availability issue?
π¬ davidgumberg commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2625065868)
utreACK https://github.com/bitcoin/bitcoin/commit/aeb3977db51792d8ad22b0a5ff8fc5ff20d69a00
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2625065868)
utreACK https://github.com/bitcoin/bitcoin/commit/aeb3977db51792d8ad22b0a5ff8fc5ff20d69a00
π¬ sr-gi commented on pull request "test: deduplicates p2p_tx_download constants":
(https://github.com/bitcoin/bitcoin/pull/31758#discussion_r1935967583)
I thought about this, but some of these are only used in this test.
I just realized that some of them were even duplicated amongst themselves. For instance, `INBOUND_PEER_TX_DELAY` and `NONPREF_PEER_TX_DELAY` refer to the same concept.
I'll do a second pass to further trim this
(https://github.com/bitcoin/bitcoin/pull/31758#discussion_r1935967583)
I thought about this, but some of these are only used in this test.
I just realized that some of them were even duplicated amongst themselves. For instance, `INBOUND_PEER_TX_DELAY` and `NONPREF_PEER_TX_DELAY` refer to the same concept.
I'll do a second pass to further trim this
π danielabrozzoni opened a pull request: "rpc: Ensure -debug=0/none behaves consistently with -nodebug"
(https://github.com/bitcoin/bitcoin/pull/31767)
Previously, -nodebug cleared all prior -debug configurations in the command line while allowing subsequent debug options to be applied.
However, -debug=0 and -debug=none completely disabled debugging, even for categories specified afterward.
This commit ensures consistency by making -debug=0 and -debug=none behave like -nodebug: they now clear previously set debug configurations but do not disable debugging for categories specified later.
See https://github.com/bitcoin/bitcoin/pull/30
...
(https://github.com/bitcoin/bitcoin/pull/31767)
Previously, -nodebug cleared all prior -debug configurations in the command line while allowing subsequent debug options to be applied.
However, -debug=0 and -debug=none completely disabled debugging, even for categories specified afterward.
This commit ensures consistency by making -debug=0 and -debug=none behave like -nodebug: they now clear previously set debug configurations but do not disable debugging for categories specified later.
See https://github.com/bitcoin/bitcoin/pull/30
...
π¬ cbrhex commented on something "":
(https://github.com/bitcoin/bitcoin/commit/edffb50b984ff1c6a4dfdc4ebdb84ca776eb0666#commitcomment-151961119)
0xFE03234A877C887764566
hexyden.com
(https://github.com/bitcoin/bitcoin/commit/edffb50b984ff1c6a4dfdc4ebdb84ca776eb0666#commitcomment-151961119)
0xFE03234A877C887764566
hexyden.com