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

💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126320439)
nit: It seems the rename to `GatherClusters` should have been squashed into efeee8b51d, but is in 5ee6f89e40 instead?

📝 MarcoFalke opened a pull request: "ci: Remove unused EXPECTED_TESTS_DURATION_IN_SECONDS env var"
(https://github.com/bitcoin/bitcoin/pull/27209)
Remove long unused travis leftover

👍 fanquake approved a pull request: "ci: Remove unused EXPECTED_TESTS_DURATION_IN_SECONDS env var"
(https://github.com/bitcoin/bitcoin/pull/27209)
ACK 3fffff50f625ed5e3c45139b3ae874f63f121a1e

💬 willcl-ark commented on pull request "util: Use steady clock in SeedStrengthen, FindBestImplementation, FlushStateToDisk":
(https://github.com/bitcoin/bitcoin/pull/27189#issuecomment-1456083159)
Concept ACK for moving to monotonic clocks.

Am I correct in my understanding that there are no current tests/benchmarks which are calling these while adjusting the system clock, but rather this is _additional_ hardening for a user whose system clock happens to be changed _while_ one of these is executing?

I couldn't easily find any callsites which might be directly affected by this, only indirectly. i.e. The system clock happens to change during a call to `GetRandHash()` or `GetRandBytes()
...

💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126384700)
well, this method is just a push function. It doesn't have to tell anything about the context where it is used. The struct is basically a map container with a cache, it doesn't need to explain how APS works.

If you are strong over the bool args usage, that would be the argument to make the changes for me.

💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126384935)
sounds good

💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126387031)
pushed, thanks

💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1456099290)
Updated per feedback, thanks S3RK 👌🏼.

[Small diff](https://github.com/bitcoin/bitcoin/compare/1cfed6ce6cd8c9f9f1e93662ff8f92cd5a5b91c4..6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44), changes are in the last test scenario and the test commit message.