💬 l0rinc commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#issuecomment-2497942820)
Rebased on top of https://github.com/bitcoin/bitcoin/pull/31306 and updated commit message

💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856568054)
Seems easier to just fix it right away, instead of touching the line to say to fix it and then fix it in the next commit?

💬 l0rinc commented on pull request "refactor: Fix remaining clang-tidy performance-inefficient-vector errors":
(https://github.com/bitcoin/bitcoin/pull/31305#issuecomment-2497947459)
Rebased on top of https://github.com/bitcoin/bitcoin/pull/31306 and updated commit messages

💬 maflcko commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2497949320)
> The false positives keep coming in, so it would be good to make progress here:

https://cirrus-ci.com/task/5175499276681216

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856576778)
I'm fine either way, but I guess this way it was easier to selectively revert a conflicting fix and leave only the suppression.

But I'd like to understand the reason for the supressions: would the lint problem break CI without them? Since this will still invalidate ACKed PRs if we merge it like this.

✅ maflcko closed an issue: "Segmentation fault when ./bitcoind"
(https://github.com/bitcoin/bitcoin/issues/31321)

💬 maflcko commented on issue "Segmentation fault when ./bitcoind":
(https://github.com/bitcoin/bitcoin/issues/31321#issuecomment-2497970472)
Closing for now, due to lack of details/follow-up

✅ maflcko closed an issue: "Maximum dbcache too small"
(https://github.com/bitcoin/bitcoin/issues/31326)

💬 maflcko commented on issue "Maximum dbcache too small":
(https://github.com/bitcoin/bitcoin/issues/31326#issuecomment-2497978288)
> Seems like the issue is also here: #27599

Closing for now as duplicate. Leave a comment, if this is not a duplicate, or if you have any other questions.

If you have more relevant details to add to #27599, please do so.

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856612322)
> Seems easier to just fix it right away, instead of touching the line to say to fix it and then fix it in the next commit?

The commits have been squashed.

> But I'd like to understand the reason for the supressions: would the lint problem break CI without them?

Yes, it would.

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#issuecomment-2498022212)
ACK f90fddfabd646c3f1d21de7bb004707a072e753e

💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856621868)
it would be good to understand why this is needed. If this is a bug, it would be good to fix it.

💬 brunoerg commented on pull request "fuzz: set the output argument of FuzzedSock::Accept()":
(https://github.com/bitcoin/bitcoin/pull/31316#issuecomment-2498034392)
So, it would be better to cherry-pick this into #28584?

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2498039344)
Can understand the checks failing on MacOS or Windows, but surprised that what I assume to be Linux on Cirrus CI is also failing. Will move to Windows and see how it behaves there. Might have to revert to explicitly checking `is_directory()`.

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856627172)
Right. I outlined this issue for follow-ups in the PR description.

💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856627249)
See b32845cb94a81e6fd8b01a8631e3d276c9fc9e35, so I think this is intentional.

I think it would be better to fix the bug, instead of silencing it.

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856631605)
We can likely simplify the call here, see: https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1846343700

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856640277)
Fixing all new "bugprone-use-after-move" warnings contradicts with other [comments](https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1846324884). On the other hand, fixing some of them is not consistent. That's why I chose to silence them.

`NOLINT` comments are easy searchable and effectively document the issues that have to be resolved.

👍 willcl-ark approved a pull request: "contrib: skip missing binaries in gen-manpages"
(https://github.com/bitcoin/bitcoin/pull/30986#pullrequestreview-2458506546)
ACK 4bbd28baf33382231f4f1dab20681c05f9915af2

Much better to add this behind a default-false flag

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1856659914)
2f9c2f4c6dcf8c45514e99d1018301aba61940fc: silent merge conflict with #30937.