💬 ryanofsky commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2624717951)
re: https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2623801480

Thanks I started looking into all of these but for "unable to find executable" errors, I think fix should be:

```diff
--- a/src/ipc/CMakeLists.txt
+++ b/src/ipc/CMakeLists.txt
@@ -9,6 +9,8 @@ if (NOT WITH_LIBMULTIPROCESS)
# build rules can find the libmultiprocess include directory and avoid
# "error: Import failed: /mp/proxy.capnp" errors from capnp.
set(MP_INCLUDE_DIR "${MP_INCLUDE_DIR}" PARENT_SCOPE
...

💬 hebasto commented on pull request "doc: update translation generation cmake example":
(https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1935753074)
> Couldn't disabling lead to missed translations?

A build option might affect the resulting translations if it changes the list of source files and headers containing translatable strings. However, this is not the case for `WITH_USDT`. It only controls the `ENABLE_TRACING` macro. Additionally, both the `extract_strings_qt.py` script and Qt's `lupdate` tool process raw sources (not preprocessed ones).

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2624734398)
I pushed that patch to see if it helps and/or reveals new issues.

💬 hebasto commented on issue "RFC: multiprocess binaries in 29.0":
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2624746006)
> I think there should be a released but experimental state...

FWIW, libsecp256k1 did release "experimental" modules.

💬 l0rinc commented on pull request "RFC: optimize `CheckBlock` input checks (duplicate detection & nulls)":
(https://github.com/bitcoin/bitcoin/pull/31682#issuecomment-2624773641)
> by using a faster comparison operator and doing direct comparisons for relatively small input sizes.

Quadratic comparison would likely be faster than sorting for very small input sizes (e.g. <10 I guess), but it would complicate the code and testing considerably for a case that is likely not representative. I also thought of that but I want the code to be faster while not being more difficult to comprehend. But feel free to challenge my bias by pushing a different PR with those changes and
...

💬 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.

💬 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
...

💬 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)
...

💬 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.

💬 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:
...

💬 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`.

💬 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?

💬 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.

💬 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 `
...

💬 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...)

💬 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?

💬 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.

👍 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

💬 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, ...).

💬 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.