💬 hebasto commented on pull request "cmake: Generate man pages at install time":
(https://github.com/bitcoin/bitcoin/pull/31764#issuecomment-2624488053)
This approach is not applicable for general cross-compilation scenarios :man_facepalming:

💬 Sjors commented on pull request "rpc: have getblocktemplate mintime account for timewarp":
(https://github.com/bitcoin/bitcoin/pull/31600#issuecomment-2624518361)
I don't understand what your script is looking for. Are you saying you applied the timewarp rule to every block instead, and then checked which block violated it?

💬 fjahr commented on pull request "rpc: have getblocktemplate mintime account for timewarp":
(https://github.com/bitcoin/bitcoin/pull/31600#issuecomment-2624524721)
> I don't understand what your script is looking for. Are you saying you applied the timewarp rule to every block instead, and then checked which block violated it?

Yes, I checked for which blocks `block_time < (prev_block_time - 600)` applies in general.

💬 fanquake commented on pull request "cmake: Generate man pages at install time":
(https://github.com/bitcoin/bitcoin/pull/31764#issuecomment-2624543963)
One other side-effect of this change would be that someone cloning a tag would no-longer get the manpages. Maybe this doesn't matter, but would be good to mention.

📝 fanquake opened a pull request: "[WIP] leveldb: pull upstream C++23 changes"
(https://github.com/bitcoin/bitcoin/pull/31766)
PR to test https://github.com/bitcoin-core/leveldb-subtree/pull/47 in our CI.

Cherry-picks two commits from upstream (https://github.com/google/leveldb/commit/302786e211d1f2e6fd260261f642d03a91e5922c, https://github.com/google/leveldb/commit/e829478c6a3a55d8e5c1227e2678dcc18d518609), which remove the usage of `std::aligned_storage/std::aligned_union`.

Note the first cherry-pick is not clean, because due to Google tooling issues, it accidently contained a revert of the prior two commits. S
...

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