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

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