💬 l0rinc commented on pull request "doc: update translation generation cmake example":
(https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1935792472)
Thank you!
What about the MULTIPROCESS option? I cannot yet run this on my Mac or on Linux, unless I disable it.
(https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1935792472)
Thank you!
What about the MULTIPROCESS option? I cannot yet run this on my Mac or on Linux, unless I disable it.
💬 l0rinc commented on pull request "Ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-2624782947)
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 in case of some edge case).
I can confirm however that this does seem to solve the issue I had, reported in https://github.com/bitcoin/bitcoin/issues/31494 (i.e. reindex(-chainstate) after a partial IBD always enabling script verification).
I have added a `LogWarning("fScriptChecks is %s", fScript
...
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-2624782947)
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 in case of some edge case).
I can confirm however that this does seem to solve the issue I had, reported in https://github.com/bitcoin/bitcoin/issues/31494 (i.e. reindex(-chainstate) after a partial IBD always enabling script verification).
I have added a `LogWarning("fScriptChecks is %s", fScript
...
💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1935808313)
248ec2d2687fae47b63688e00b9ef18d4c0c9676 nit: when I run the linter (along with all other tests) on this commit it complains about trailing whitespace.
```diff
diff --git a/src/net.cpp b/src/net.cpp
index 8d0dd84d91..0228ddce57 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2068,7 +2068,7 @@ void CConnman::EventIOLoopCompletedForAllPeers()
DisconnectNodes();
NotifyNumConnectionsChanged();
}
-
+
Sock::EventsPerSock CConnman::GenerateWaitSockets(Span<CNode* const> nodes)
...
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1935808313)
248ec2d2687fae47b63688e00b9ef18d4c0c9676 nit: when I run the linter (along with all other tests) on this commit it complains about trailing whitespace.
```diff
diff --git a/src/net.cpp b/src/net.cpp
index 8d0dd84d91..0228ddce57 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2068,7 +2068,7 @@ void CConnman::EventIOLoopCompletedForAllPeers()
DisconnectNodes();
NotifyNumConnectionsChanged();
}
-
+
Sock::EventsPerSock CConnman::GenerateWaitSockets(Span<CNode* const> nodes)
...
💬 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