💬 hebasto commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#issuecomment-2498088917)
https://cirrus-ci.com/task/4898976128827392:
```
/ci_container_base/src/qt/bitcoinunits.cpp:169:13: error: the variable 'whole' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [performance-unnecessary-copy-initialization,-warnings-as-errors]
169 | QString whole = parts[0];
| ^
| const &
```

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1856663445)
`PACKAGE_NAME` changed in #31042

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856666679)
Unfortunately silencing will also invalidate the ACK-ed PR.
But if we could instead add `-bugprone-use-after-move` to `src/.clang-tidy` this concern could be addressed separately.

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856672277)
It will expose the whole codebase to re-introducing related bugs, no?

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856679357)
We could tackle it in a separate pr like we do with the `performance-*` checks, right?

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856696985)
We could, but `performance-*` checks are not about safety, whereas `bugprone-*` checks are.

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856700230)
If you decide to fix them here, I'll just rebase the related PR and ask for a re-review, it's not a tragedy :)

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1856703174)
Never mind, this causes more headaches that it solves.

💬 l0rinc commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#issuecomment-2498168179)
> the variable 'whole' is copy-constructed

Thanks, had to enable GUI as well (and clean the previous build directory, otherwise it's not detected at all for some reason):
```bash
git clean -fxd \
&& cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_GUI=ON \
&& cmake --build build -j$(nproc) \
&& run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,performance-unnecessary-copy-initialization' | grep -v 'clang-tidy'
```

📝 TheCharlatan opened a pull request: "interpreter: Use the same type for SignatureHash in the definition"
(https://github.com/bitcoin/bitcoin/pull/31365)
This was missed during the original PR switching the nHashType argument to int32_t in SignatureHash in bc52cda1f3c007bdf1ed00aa3011e207c7531017.

The problem was discovered after running into a linker error when attempting to link this code as a static library using the header as a declaration with a riscv32 bare metal toolchain. The compiler would error with:
```
/opt/riscv-ilp32/lib/gcc/riscv32-unknown-elf/13.2.0/../../../../riscv32-unknown-elf/bin/ld: build_kernel_riscv/src/libbitcoin_con
...

💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856742649)
I looked at this and this is an upstream *boost* bug, not an llvm or bitcoin-core bug.

The reason is that the macro is a loop (not a do-while-0-loop):

```
#define BOOST_TEST_TOOL_IMPL( frwd_type, P, assertion_descr, TL, CT, ARGS ) \
do { \
BOOST_TEST_PASSPOINT(); \
::boost::test_tools::tt_detail:: \
BO
...

💬 hebasto commented on pull request "interpreter: Use the same type for SignatureHash in the definition":
(https://github.com/bitcoin/bitcoin/pull/31365#issuecomment-2498233867)
Does it make sense to make sighash types/flags enum explicitly typed as well?

💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856759837)
The bug is fixed in https://github.com/boostorg/test/commit/eb0c07b9f004981184bbd24c435726d735eb5581

💬 l0rinc commented on pull request "interpreter: Use the same type for SignatureHash in the definition":
(https://github.com/bitcoin/bitcoin/pull/31365#discussion_r1856760996)
Would it make sense to do this for the remaining `nHashType` types, e.g. in `CTransactionSignatureSerializer`, `ParseSighashString`, `SignTransaction`, `MutableTransactionSignatureCreator`, `SignatureHashOld`, `SignSignature`, etc?

And how can we avoid this error next time, can we enable (a clang-tidy?) check that compares the cpp and header types?

💬 laanwj commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2498279037)
> Another source of false positive could be if somebody from the outside initiates communication to the VM to which it responds. E.g. an outsider tries to connect to the VM to which it responds with an outbound packet e.g. TCP RST.

Exactly. For all we know, the CI VM is firewalled off sufficiently that this can't happen, but we don't know.

> At least that should be obvious from the error log, showing the incoming packet first (I just pushed a slight change for that).

Ah yes, as long as
...

📝 hebasto opened a pull request: "cmake: Check `-Wno-*` compiler options for `leveldb` target"
(https://github.com/bitcoin/bitcoin/pull/31366)
Otherwise, https://cirrus-ci.com/task/4830737755537408:
```
At global scope:
cc1plus: note: unrecognized command-line option ‘-Wno-conditional-uninitialized’ may have been intended to silence earlier diagnostics
```

💬 mzumsande commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1856824817)
> In fact it does it after reindex is finished. ActivateBestChain connects blocks one by one until it has nothing left to do so that makes sense.

Just as an aside: ABC is also called [during](https://github.com/bitcoin/bitcoin/blob/2638fdb4f934be96b7c798dbac38ea5ab8a6374a/src/validation.cpp#L5149) reindexing when encountering the genesis block. That's why we have this slightly weird behavior with a full `-reindex` that normally first reindexes all blocks and then builds the chainstate, but i
...

💬 laanwj commented on pull request "cmake, qt: Use absolute paths for includes in MOC-generated files":
(https://github.com/bitcoin/bitcoin/pull/31361#issuecomment-2498350728)
Tested `-DBUILD_GUI=ON` builds:
- make + whole sourcedir in symlink → works
- ninja + whole sourcedir in symlink → works
- make + build directory is symlink → works
- ninja + build directory is symlink → works

Tested ACK 6f4128e3a838d03f46d397c15bc5333287e14863

💬 laanwj commented on issue "Cmake build system breaks with symbolic links":
(https://github.com/bitcoin/bitcoin/issues/31145#issuecomment-2498353318)
> Unable to reproduce this part on Ubuntu 24.04.

Okay... i'll retest the multiprocess case. If it's still an issue, it's seperate from the Qt case anyway and would need to be fixed seperately.

💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2498369703)
Would have been remiss to not push a regression test for the issue the fuzzer caught. Pushed test case for coverage of a reconsiderable tx.