💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2624601401)
`266ac32673...7866c736c8`: rebase due to conflicts

💬 TheCharlatan commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1935678768)
Why is this now overriding the variable instead of appending?

💬 ryanofsky commented on issue "RFC: multiprocess binaries in 29.0":
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2624659370)
Thanks again for clarifying your concerns @darosior. Hopefully Sjors post added background and answered the specific questions you asked. I just wanted to respond to two things:

> First of all why is the default position that we should ship a new binary on short notice right before feature freeze

I think you can be assured that this is not the case. As a rule controversial PRs do not get merged. I'm just trying to get to the bottom of why #31756 + eb41f92c0889708045ce0a30c6d0f8d43cecd6d5 was c
...

💬 instagibbs commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#issuecomment-2624666467)
reACK 7426afbe62414fa575f91b4f8d3ea63bcc653e8b

I am -0 on "[p2p] assign just 1 random announcer in AddChildrenToWorkSet" though. That and similar changes discussed in https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1934078679 seem to complicate analysis for unclear performance gains?

💬 instagibbs commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#issuecomment-2624666944)
reACK 7426afbe62414fa575f91b4f8d3ea63bcc653e8b

I am -0 on "[p2p] assign just 1 random announcer in AddChildrenToWorkSet" though. That and similar changes discussed in https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1934078679 seem to complicate analysis for unclear performance gains?

🤔 Sjors reviewed a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2583927266)
Concept ACK

Mostly happy with 1bfabb4f4ec07c8fb0dfc2515d413b5196e01348, but I'm confused about the key origin handling.

💬 Sjors commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1935702280)
e743d11aae2e459058b1af70b6d19ab90293d1f4: I'm confused by this comment. I guess by "valid" you mean that a `KeyOriginInfo` field is present, but it can be a dummy?

It seems more intuitive if we insert an origin entry ourselves here ... if possible.

💬 Sjors commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1935623167)
1bfabb4f4ec07c8fb0dfc2515d413b5196e01348: maybe rename to `FillPrivKey`?

```cpp
/** Try to derive the private key at pos, if private data is available in arg, and fill out with it. */
virtual void FillPrivKey(int pos, const SigningProvider& arg, FlatSigningProvider& out) const = 0;
```

That also distinguishes it from `ConstPubkeyProvider::GetPrivKey`.

💬 Sjors commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1935646730)
e743d11aae2e459058b1af70b6d19ab90293d1f4: maybe rename this to `FillPubKey`. The filling seems more important, and the return value makes it clear enough it also gets it.

💬 Julian128 commented on pull request "RFC: optimize `CheckBlock` input checks (duplicate detection & nulls)":
(https://github.com/bitcoin/bitcoin/pull/31682#issuecomment-2624700760)
I experimented a bit more and it looks like I got another 17% speed improvement (on block413567 on my linux machine) over your changes @l0rinc , by using a faster comparison operator and doing direct comparisons for relatively small input sizes. Can I create a new pull request for that?

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