🤔 rkrux reviewed a pull request: "wallet: Remove IsMine from migration code"
(https://github.com/bitcoin/bitcoin/pull/30328#pullrequestreview-2442077210)
Preliminary review fa155991ff30bc001229e1d71bf44e8782f02d17

I feel e6342ffdf0030f6350a07f325ce603fddfff1bd8 commit could be a separate PR, these functional tests are not dependent on the changes in the next two commits and don't need to be tied to these changes.

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

💬 rkrux commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1846392613)
```diff
- { return key.size() == 33; });
+ { return key.size() == CPubKey::COMPRESSED_SIZE; });
```

There's another usage down below as well on line 1816: https://github.com/bitcoin/bitcoin/commit/fa155991ff30bc001229e1d71bf44e8782f02d17#diff-55ce403d9c3440b4a38a261b7d452f84dd9a1d09b2a5ef1b4d3ac0c3567b9106R1816

💬 rkrux commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1846291918)
We can't reuse `IsMineSigVersion` because it will be removed soon?
Can we add few comments here like done in `IsMineSigVersion`? https://github.com/bitcoin/bitcoin/blob/28.x/src/wallet/scriptpubkeyman.cpp#L56-L67

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