💬 rkrux commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1846279325)
`arbitrary`

💬 rkrux commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1846396195)
```diff
- std::vector<std::vector<unsigned char>> sols;
+ std::vector<std::vector<unsigned char>> solutions;
```

Agree, would be easier to read.

💬 rkrux commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1846447833)
There must be a reason for making `IsCompressedPublicKey()` here static, which I dont know about, otherwise, it could be used within `all_of` here, thereby getting rid of some logic duplication: https://github.com/bitcoin/bitcoin/blob/28.x/src/script/interpreter.cpp#L85

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1846482350)
I don't quite understand this^.

From what I understood:
If `include_mempool` is true, then the `blockhash` can be empty if the transaction is in mempool.
If `include_mempool` is false, then the `blockhash` can't be empty because there is no other way for the transaction to appear in the result here besides being in a block.

Am I missing something?

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