👍 instagibbs approved a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2458872127)
light re-ACK 878b6c85466366c4ae5f454ec49b5a5f561e0ed2

Height returned is now optional, some doc cleanups, removal of duplicate heights check, use of `GetBlockChecked`

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2498447097)
`1592a7dad4...071e43ffae`: fix `feature_config_args.py` and `p2p_seednode.py` to not generate non-loopback traffic.

> It only becomes critical if network traffic would cause a CI failure.

My intention here is to fail the CI because otherwise the log will be buried in the CI output and nobody will notice it. It follows that if this fails randomly with false positives when one would have to investigate it manually for arbitrary PRs which is highly highly highly undesirable.

💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1856911491)
The return code is ignored, which is why CI passed, despite the tests calling out

📝 dergoegge opened a pull request: "ci: limit max stack size to 512 KiB"
(https://github.com/bitcoin/bitcoin/pull/31367)
Closes #29840

📝 dergoegge converted_to_draft a pull request: "ci: limit max stack size to 512 KiB"
(https://github.com/bitcoin/bitcoin/pull/31367)
Closes #29840

💬 theStack commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1856939888)
It seems that these conditions are not needed, as the checks are done in `is_valid_script` below anyway (see `Solver` cases `TxoutType::SCRIPTHASH` and `TxoutType::WITNESS_V...`)?
```suggestion
```
Tests still pass without them.

🤔 glozow reviewed a pull request: "policy: ephemeral dust followups"
(https://github.com/bitcoin/bitcoin/pull/31279#pullrequestreview-2455990857)
utACK 466e4df3fb83ef82b6add22e202e3a70dbf83a12

💬 glozow commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1856946021)
Why not `CTransactionRef`?

💬 glozow commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1856957211)
oh edit, i see now

💬 glozow commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1856947536)
Why not `CTransactionRef`? (3ed930a1f41f7d7160c6ede5dcf3d4d5f1cfa876)

💬 glozow commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1854898389)
Why is using `remaining_bytes()` better here? I didn't get that impression from reading https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1840754607

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