💬 dergoegge commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1846667260)
Same as above, this will create long running inputs and maybe even run out of memory?

💬 dergoegge commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1846665944)
This will create very long running inputs (e.g. txs = std::numeric_limits<uint32_t>::max()).

```suggestion
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), N) {
```

or

```suggestion
LIMITED_WHILE(fuzzed_data_provider.remaining_bytes(), N) {
```

💬 sipa commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1846676347)
TxGraph will be able reason about cluster count limits using arbitrary staged changes (so any combination of transaction deletions, transaction additions, and added dependencies).

These operations are always ordered as (1) tx additions (2) tx deletions (3) dep additions, even if they are invoked in a different order. So if you first add dependencies which - if applied - would violate the limit, and then delete some transactions in the cluster such that the result breaks up again and stays below
...

💬 ryanofsky commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1846589575)
re: https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1845361924

I think the advantage of your suggestion is avoids repeating `str`. So I'd probably write the code that way if `str` was something more complicated than a single variable.

I think the story of the current version is "this returns message plus a list of details if any" and the story of the suggested version is "this returns a message plus a suffix, which can be empty or can be a list of details" and I think both are re
...

💬 ryanofsky commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1846585429)
re: https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1845399972

> I don't fully understand why this couldn't be an Untranslated(strprintf

Good catch! Thanks for the close reading. The commit description 75643fd325748f8b81dc78edfc1266f47990f9b4 was wrong when it said that this change was needed to let the scripted diff compile. That comment is only true for previous commit d392068d7257e0cb8ca8e95fcb5e504b4b4f2839, not this one. Fixed the description now.

About the Result class,
...

💬 ryanofsky commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1846675831)
re: https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1845404225

Right, I think it would be bad if changing or tweaking the name of the program required newly translating "The %s developers" with the new name of the program hardcoded.

💬 ryanofsky commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1846588485)
re: https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1845409666

> what is the reason for these still being false positives?

IIUC, linter is just very simple and doesn't expand macros. If I remove this line running the linter shows:

```
src/clientversion.cpp: Expected 0 argument(s) after format string but found 1 argument(s): strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION)
```

🤔 ryanofsky reviewed a pull request: "refactor: Clean up messy strformat and bilingual_str usages"
(https://github.com/bitcoin/bitcoin/pull/31072#pullrequestreview-2442622172)
Thanks for the reviews!

Updated 85df2fbf26c73f97f85797868155247c11a4ccd6 -> 3b24c24889c09f6c8f33408b57c2042c4e565388 ([`pr/bfmt.5`](https://github.com/ryanofsky/bitcoin/commits/pr/bfmt.5) -> [`pr/bfmt.6`](https://github.com/ryanofsky/bitcoin/commits/pr/bfmt.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bfmt.5..pr/bfmt.6)) fixing a commit description (no code changes)

💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1846689340)
My point was it doesn't have to be empty if searching in mempool, as your suggestion sounds to me.

💬 hebasto commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2483236103)
> I see a ` -DBUILD_FUZZ_BINARY=ON` in the setup, which suggests to me that we are at least building the fuzz tests. We are, however, at [0ca1d1b](https://github.com/bitcoin/bitcoin/commit/0ca1d1bf69ca364393e924cf41becfde1b68126c) which seems to be about a month old. Our Real-World-Code projects don't live at head, they jump forward periodically. If it's a recent regression we may not have seen it yet.

```
> git rev-parse HEAD
0ca1d1bf69ca364393e924cf41becfde1b68126c
> cmake -B build-stati
...

👍 ryanofsky approved a pull request: "refactor: Prepare compile-time check of bilingual format strings"
(https://github.com/bitcoin/bitcoin/pull/31295#pullrequestreview-2442809913)
Code review ACK fa3e074304780549b1e7972217930e34fa55f59a. Nice changes! These should allow related PRs to be simpler.

💬 furszy commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846750673)
The max number of workers threads before was 15 and now seems to be 14. E.g. provide `-par=16`:

Previous Code:
```
script_threads = 16.
worker_threads_num = std::clamp(script_threads-1, 0, 15);
...
Outcome: 15 workers
```

Current Code
```
script_threads = std::clamp(16, 0, 15); -> output 15
worker_threads_num = script_threads - 1;
..
Outcome: 14 workers.
```

💬 l0rinc commented on pull request "refactor: Fix remaining clang-tidy performance-inefficient-vector errors":
(https://github.com/bitcoin/bitcoin/pull/31305#issuecomment-2483381183)
Thanks @hebasto, addressed the bench and fuzz ones as well (in separate commits, explaining the decisions)

📝 Sjors opened a pull request: "Drop script_pub_key arg from createNewBlock"
(https://github.com/bitcoin/bitcoin/pull/31318)
Providing a script for the coinbase transaction is only done in test code and for CPU solo mining.

Production miners use the getblocktemplate RPC which omits the coinbase transaction entirely from its block template, leaving it to external (pool) software to construct it.

This commit removes the script_pub_key argument from createNewBlock() in the Mining interface.

A coinbase script can still be passed via BlockCreateOptions instead. Tests are modified to do so.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1846795785)
#31318 drops this argument. It's only slightly easier to merge that first, so I'll leave this PR open.

💬 furszy commented on pull request "test: Revert to random path element":
(https://github.com/bitcoin/bitcoin/pull/31317#discussion_r1846797832)
What about checking `TryCreateDirectories` too? And doing this in a loop until it creates a new dir?
Probably we might also need to lock the directory like we do for the custom datadir path.

💬 hebasto commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2483394112)
> TODO:
>
> * [ ] fix `arm-linux-gnueabihf-g++: error: unrecognized command-line option ‘-mpclmul’` [failure](https://cirrus-ci.com/task/4621127983562752) and re-enable multiprocess for arm job

I don't think the problem description is correct.

In my opinion, the root of the issue is:
```
$ make --no-print-directory -C depends HOST=arm-linux-gnueabihf print-multiprocess_packages
multiprocess_packages=
```

💬 fanquake commented on pull request "build: Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC":
(https://github.com/bitcoin/bitcoin/pull/31307#issuecomment-2483395447)
Can you link to your upstream bug report.

💬 l0rinc commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#issuecomment-2483397450)
ACK 3b24c24889c09f6c8f33408b57c2042c4e565388

💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2483402160)
The PR was prompted because I noticed in #31283 I had to needlessly pass `script_pub_key` around.