💬 theuni commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#discussion_r1857161138)
This is masking a legitimate bug, it just happens to be ok for now because of the types involved.

It shouldn't be marked nolint, rather it should be fixed: drop the forward and pass by `const Args&`.

💬 l0rinc commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#discussion_r1857167859)
Thanks for the review, please see https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856621868 for those commits - this PR is based on it to make sure the CI checks the mentioned errors, your nack is likely directed at non-performance checks

🚀 glozow merged a pull request: "policy: ephemeral dust followups"
(https://github.com/bitcoin/bitcoin/pull/31279)

💬 theuni commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1857191076)
> NOLINT comments are easy searchable and effectively document the issues that have to be resolved.

Sorry, but no, that is exactly the _opposite_ if what `NOLINT` documents!

`NOLINT` means "I'm smarter than my static analyzer".

`TODO` would effectively document an issue to be resolved.

Hard NACK here due to the reasoning being applied. In fact, it rises to the level of suspicion. I'll be reviewing all of these refactors in great detail.

👍 BrandonOdiwuor approved a pull request: "interpreter: Use the same type for SignatureHash in the definition"
(https://github.com/bitcoin/bitcoin/pull/31365#pullrequestreview-2459405878)
Code Review ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1857211774)
@hebasto, what if we invert this and push the fixes in separate PRs and only merge this TIDY_LLVM_V upgrade after all problems were already resolved?

💬 glozow commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1857216280)
Check the error?
```
BOOST_CHECK_EQUAL(result_single_tx_low_fee.m_state.GetResult(), PackageValidationResult::PCKG_TX);
auto it_low_fee = result_single_tx_low_fee.m_tx_results.find(tx_single_low_fee->GetWitnessHash());
BOOST_CHECK_EQUAL(it_low_fee->second.m_state.GetResult(), TxValidationResult::TX_RECONSIDERABLE);
```

💬 laanwj commented on issue "Tracepoint Interface Tracking Issue":
(https://github.com/bitcoin/bitcoin/issues/31274#issuecomment-2498832633)
> I just found out about https://github.com/libbpf/usdt which is a single usdt.h header file which could be an alternative for systemtaps sys/sdt.h.

Neat, that seems preferable to maintaining our own! They even recommend copying the header file into your project.

💬 l0rinc commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#discussion_r1857217900)
I've removed the commits from #31364

💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1857233992)
done

✅ l0rinc closed a pull request: "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing"
(https://github.com/bitcoin/bitcoin/pull/29473)

💬 l0rinc commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2498871171)
I just found this was already attempted in a very similar way by @martinus a few years ago in https://github.com/bitcoin/bitcoin/pull/21176 - so now we have two versions, if this will turn out to be important again.
Closing, thanks for the reviews!

👍 theuni approved a pull request: "refactor: Fix remaining clang-tidy performance-inefficient-vector errors"
(https://github.com/bitcoin/bitcoin/pull/31305#pullrequestreview-2459478618)
ACK 11f3bc229ccd4b20191855fb1df882cfa6145264

💬 l0rinc commented on pull request "refactor: Fix remaining clang-tidy performance-inefficient-vector errors":
(https://github.com/bitcoin/bitcoin/pull/31305#issuecomment-2498906183)
Note that while [the benchmarks](https://corecheck.dev/bitcoin/bitcoin/pulls/31305) indicate a speedup, [they're actually the ones that were changed](https://github.com/bitcoin/bitcoin/pull/31305/files#diff-3a60bcd828090e387348302ccc8453c055170d5b5ddb828cd890228191d0fe42R90-R103):
<img width="633" alt="image" src="https://github.com/user-attachments/assets/808d3dac-d91a-4353-ac68-e078124fd7c6">

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1857275360)
> > NOLINT comments are easy searchable and effectively document the issues that have to be resolved.
>
> Sorry, but no, that is exactly the _opposite_ of what `NOLINT` documents!
>
> `NOLINT` means "I'm smarter than my static analyzer".
>
> `TODO` would effectively document an issue to be resolved.
>
> Hard NACK here due to the reasoning being applied. In fact, it rises to the level of suspicion. I'll be reviewing all of these refactors in great detail.

A bare `NOLINT`, without a
...

✅ hebasto closed a pull request: "ci: Update Clang in "tidy" job"
(https://github.com/bitcoin/bitcoin/pull/31306)

💬 theuni commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1857275666)
As mentioned [here](https://github.com/bitcoin/bitcoin/pull/31364#discussion_r1857161138), this change struck me as dangerous.

> It shouldn't be marked nolint, rather it should be fixed: drop the forward and pass by const Args&.

I understand the desire to turn on new checks and notate the false-positives, but the result should _always_ be better/safer/cleaner code.

In this case, the result would've been a latent bug explicitly marked notabug. And while a buggy test isn't the end of the
...

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1857278221)
Right.

📝 ijunxyz123 opened a pull request: "Update COPYING"
(https://github.com/bitcoin/bitcoin/pull/31368)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1857283241)
> As mentioned [here](https://github.com/bitcoin/bitcoin/pull/31364#discussion_r1857161138), this change struck me as dangerous.

My apologies.

An additional context available here: https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1846324884.