💬 vasild commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2482911757)
`e7cf8e8fc6...5766bbefa9`: rebase and make sure `DynSock::Accept()`'s output argument is initialized properly: https://github.com/Sjors/bitcoin/pull/50#issuecomment-2478468037

👍 brunoerg approved a pull request: "build: Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC"
(https://github.com/bitcoin/bitcoin/pull/31307#pullrequestreview-2442541152)
ACK b2d536100282bd901d3e0be7f7f4a6966e0ef817

📝 vasild opened a pull request: "fuzz: set the output argument of FuzzedSock::Accept()"
(https://github.com/bitcoin/bitcoin/pull/31316)
`FuzzedSock::Accept()` properly returns a new socket, but it forgot to set the output argument `addr`, like `accept(2)` is expected to.

This could lead to reading uninitialized data during testing when we read it, e.g. from `CService::SetSockAddr()` which reads the `sa_family` member.

Set `addr` to a fuzzed IPv4 or IPv6 address.

💬 vasild commented on pull request "fuzz: set the output argument of FuzzedSock::Accept()":
(https://github.com/bitcoin/bitcoin/pull/31316#issuecomment-2483042252)
Found in another mocked socket implementation (not `FuzzedSock`) in https://github.com/Sjors/bitcoin/pull/50#issuecomment-2478468037, but `FuzzedSock` has the same problem.

📝 maflcko opened a pull request: "test: Revert to random path element"
(https://github.com/bitcoin/bitcoin/pull/31317)
The randomness in the path element is required to allow a single fuzz test to run in parallel. Previous releases used a uint256 random value, but 10 random bytes should be sufficient as well, while avoiding a `MAX_PATH` violation on Windows.

The issue was introduced by myself, by suggestion to use the current time in https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1835351305.

💬 maflcko commented on pull request "test: Revert to random path element":
(https://github.com/bitcoin/bitcoin/pull/31317#issuecomment-2483056878)
This should fix https://issues.oss-fuzz.com/issues/379418970, which seems to be running on servers that do not have nanosecond precision:

```
libc++abi: terminating due to uncaught exception of type std::__1::__fs::filesystem::filesystem_error: filesystem error: in remove_all: No such file or directory ["/tmp/test_common bitcoin/1731742732339168000/regtest/fuzzed_i2p_private_key"]
```

To test locally, one can reduce to microsecond precision (or less) via:

```diff
diff --git a/src/tes
...

💬 hebasto commented on pull request "refactor: Fix remaining clang-tidy performance-inefficient-vector errors":
(https://github.com/bitcoin/bitcoin/pull/31305#issuecomment-2483124499)
What about:
```
/ci_container_base/src/bench/prevector.cpp:91:13: error: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
90 | for (size_t i = 0; i < 260; ++i) {
91 | vec.emplace_back();
| ^
/ci_container_base/src/bench/prevector.cpp:104:13: error: 'emplace_back' is called inside a loop; consider pre-allocating the container capaci
...

🤔 glozow reviewed a pull request: "cluster mempool: Implement changeset interface for mempool"
(https://github.com/bitcoin/bitcoin/pull/31122#pullrequestreview-2442656388)
Still working on review. I've skimmed through the commits and reviewed the end state of `MemPoolAccept` so far.

💬 glozow commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1846628355)
Re: "all mempool additions and removals"

Is the plan to use changesets for all other mempool changes as well, such as `removeForBlock` and `removeForReorg`?

💬 glozow commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1846606719)
This is maybe a TxGraph question, but will we get to the point where `CMPA` and `CheckPackageLimits` can do the calculations using all staged changes? For example, if it looks like I would bust cluster limits using current mempool, but my parent's RBF is breaking up the cluster / removing things from it.

I'm wondering if we could easily align our multi-`testmempoolaccept` interface with the `ProcessNewPackage` interface to support package CPFP and RBF by pointing `testmempoolaccept` down the
...

💬 glozow commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1846638974)
Comment seems clear to me.

🤔 hebasto reviewed a pull request: "guix: remove `util-linux`"
(https://github.com/bitcoin/bitcoin/pull/31285#pullrequestreview-2442735704)
Post-merge ACK 4d668549825cc2f999b349e8fcd8cc9434c409c3.

My Guix build:
```
riscv64
b681575760a0d19445a17d0d54f50a65d705923caee3a7cd7502e6611950dc11 guix-build-4d668549825c/output/aarch64-linux-gnu/SHA256SUMS.part
7af1420fedd8b4d10df650e92840287106dd74c289b6ef6fd2fd039387824647 guix-build-4d668549825c/output/aarch64-linux-gnu/bitcoin-4d668549825c-aarch64-linux-gnu-debug.tar.gz
bd6520732b5c7805002a3227fe2d16f56a77453d913f5204e5be869aaf4a9534 guix-build-4d668549825c/output/aarch64-linux
...

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