:lock: fanquake locked an issue: "Vahhh"
(https://github.com/bitcoin/bitcoin/issues/27204)

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1126052875)
> add (a TODO for) method to the wallet tool to re-run the helper functions; this would be for advanced users who somehow ended up with a wallet not matching this pattern, but have repented, added the right descriptors, and now want the global HD key field set


Is this really needed if we add support for `sethdseed` later?

💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1455748229)
utACK 1cfed6ce6cd8c9f9f1e93662ff8f92cd5a5b91c4 (also reviewed the tests)

💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126088720)
this is slightly confusing because I'd expect the number to go one up from the previous case (from 3 => 4).

The test is passing, because you flipped `positive_only` and now excluded negative group is balanced by the group for `dest7`.

I'd suggest keeping `positive_only=false` and bumping the numbers

💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126099320)
Commit message is now inconsistent with the code. Either drop the details of test cases or update them.

💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126122594)
The reason why I suggested this change is because it's hard to explain why we need a `insert_mixed` flag. Why don't we just always add groups to `mixed` vector? I understand that's because of how aps works, but just looking at this method it's mysterious.

💬 MarcoFalke commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1126125555)
Any reason to use the cli, when the normal rpc can be used directly?

Also, I wonder if this introduces another race where the actual rescan in `t.start()` is scheduled after this call. Avoiding this race might be ugly, but I think it could be possible by using assert_debug_log with a timeout value to sync on the rescan actually starting?

💬 TheCharlatan commented on pull request "guix: pass `--enable-initfini-array` to release GCC":
(https://github.com/bitcoin/bitcoin/pull/27153#issuecomment-1455762187)
ACK 127c637cf0a80e0ea68a7c5aaa088e5ccc9d3d13

💬 MarcoFalke commented on issue "Intermittent issue in p2p_ibd_stalling.py self.wait_until(lambda: self.total_bytes_recv_for_blocks() == 172761)":
(https://github.com/bitcoin/bitcoin/issues/27112#issuecomment-1455762739)
Looks like a different issue, so maybe open a new one? Also, we should probably increase the default timeout when `--valgrind`

⚠️ fanquake opened an issue: "Issue in `p2p_ibd_stalling.py` under Valgrind"
(https://github.com/bitcoin/bitcoin/issues/27208)
At 40c6c85c05812ee8bf824b639307b1ac17a001c4 with the native_valgrind job:
```bash
test 2023-03-05T21:26:43.074000Z TestFramework.node0 (DEBUG): Connecting to 127.0.0.1:12173 outbound-full-relay
node0 2023-03-05T21:26:43.265731Z [msghand] [net_processing.cpp:5807] [SendMessages] [net] Requesting block 752405439cea869d584044084502582bc209e4ef97e4bf3b8c2ba3958acaf606 (21) peer=0
node0 2023-03-05T21:26:43.267295Z [msghand] [net.cpp:2816] [PushMessage] [net] sending getdata (37 bytes) peer=
...

💬 fanquake commented on issue "Intermittent issue in p2p_ibd_stalling.py self.wait_until(lambda: self.total_bytes_recv_for_blocks() == 172761)":
(https://github.com/bitcoin/bitcoin/issues/27112#issuecomment-1455783529)
> Looks like a different issue, so maybe open a new one?

#27208

💬 MarcoFalke commented on issue "Issue in `p2p_ibd_stalling.py` under Valgrind":
(https://github.com/bitcoin/bitcoin/issues/27208#issuecomment-1455785923)
What did you set the timeout factor to? Is there a combined log or similar?

👍 TheCharlatan approved a pull request: "wallet: use Mutex for g_sqlite_mutex instead of GlobalMutex"
(https://github.com/bitcoin/bitcoin/pull/25491)
ACK 4163093d6374e865dc57e33dbea0e7e359062e0a

💬 MarcoFalke commented on pull request "clang-tidy: Fix `modernize-use-default-member-init` in headers and force to check all headers":
(https://github.com/bitcoin/bitcoin/pull/26705#discussion_r1126213761)
Fixed in clang-tidy-17, see https://github.com/llvm/llvm-project/commit/fa491fefb0f862b653b196eb61f0e690b0b71460

👍 TheCharlatan approved a pull request: "clang-tidy: Add more `performance-*` checks and related fixes"
(https://github.com/bitcoin/bitcoin/pull/26642)
ACK 7db868b49b60de371d01cda1f8e5faaa0e18f822

I reproduced all changes here by running `clang-tidy` locally. They all seem like a clear improvement to me.

💬 TheCharlatan commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1455960808)
Light code review ACK 142718890e718397e0026c315c8940102b9657ce

> I mostly reviewed the overall PR diff and didn't review individual commits in detail since it was easier to think about class definitions with only move operations or only swap operations, not combinations of both.

I can echo this, reviewing this piecemeal was a bit more difficult than just looking at it all at once.

💬 naumenkogs commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#discussion_r1126306044)
nevermind.

💬 hebasto commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1456009757)
This PR causes cross compiling errors for some hosts using default toolchains on Ubuntu 22.04.

For example, for `riscv64-linux-gnu`
```
$ make -C src bitcoind
...
CXXLD bitcoind
/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: libbitcoin_node.a(libbitcoin_node_a-net_processing.o): in function `.L11743':
net_processing.cpp:(.text+0x1cc82): undefined reference to `std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsig
...

💬 hebasto commented on pull request "util: add missing include and fix function signature":
(https://github.com/bitcoin/bitcoin/pull/27192#issuecomment-1456026433)
Post-merge ACK.

💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126270559)
> Is m_ready_to_calculate an error indication, since it seems it can be false only If we hit the DoS limit?

No it's not just for indicating DoS limit reached, notice that it is also set to false after `BuildMockTemplate` is called. It prevents somebody from constructing a `MiniMiner` and calling `CalculateBumpFees()` or `CalculateTotalBumpFees()` multiple times, which could result in very incorrect results if the target feerate changes.