💬 tdb3 commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1856989333)
Probably overkill, but could add a test for undersized v1 SPK `s << OP_1 << std::vector<unsigned char>(31, 0x01);` as well.

pico nit: If updating the file again, could add a blank line before this comment to be consistent with other checks in the test.

💬 tdb3 commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1856607483)
nit: Should this be `ANCHOR_BYTES` instead?

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

👍 tdb3 approved a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2458423716)
ACK a3a4d199e298a76725d0c0424195ae54c7e95fe0

Nice catch/updates.

Induced failures locally by:
- re-using the P2WSH spk of the previous check and seeing `bad_variant_access` occur with `ExtractDestination()`
```diff
- s << OP_1 << ToByteVector(xpk);
+ s << OP_0 << ToByteVector(scripthash);
```
- using a purposefully different pubkey for the spk and seeing the comparison fail
```diff
+ CKey wrong_key = GenerateRandomKey();
+ CPubKey wrong_pubkey = w
...

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

At that point I feel like we'd have to do it for everything. If I understood the actually problem correctly, it is that we cannot serialize an int with our current serialization functions on that platform, so I'm not sure if that would be useful in the first place.

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

I don't think it makes sense in this context, but might still be good to do this for consistency. I'd rather change the absolute minimum possible in these files though.

> And how can we avoid this error next time, can we enable (a clang-tidy?) check that compares the cpp an
...

👍 theuni approved a pull request: "interpreter: Use the same type for SignatureHash in the definition"
(https://github.com/bitcoin/bitcoin/pull/31365#pullrequestreview-2459255428)
Obvious fix ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f.

Linker did its job here :)

👍 theuni approved a pull request: "cmake: Check `-Wno-*` compiler options for `leveldb` target"
(https://github.com/bitcoin/bitcoin/pull/31366#pullrequestreview-2459294286)
utACK 0f73d9dd49b371c28ba9e74ef3519ef8f6e80ebc

A more helpful commit message would've been useful though:
"Check for -Wfoo rather than -Wno-foo because the latter may not cause the test to fail".

💬 l0rinc commented on pull request "interpreter: Use the same type for SignatureHash in the definition":
(https://github.com/bitcoin/bitcoin/pull/31365#issuecomment-2498738059)
ACK c288c790cd9abe91e53164aba5d975ef1e26ee3f

🤔 theuni reviewed a pull request: "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors"
(https://github.com/bitcoin/bitcoin/pull/31364#pullrequestreview-2459336460)
Concept NACK to these new tidy checks if sufficient care is not going to be taken.

They should either be obviously/trivially safe or left alone.

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