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

👍 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