💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2497640819)
Re: https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2451143073

21bc3a3657fea5ed44172963639ee8ca5cd8e46c

> Very subjective, but I think the unrelated code cleanups in this commit make it hard to follow and are very distracting. Would be much happier to see them dropped or moved to a refactoring commit.

Broke out 215be3dfba90a6d5b2b37f0f94bcb621dd3d4a9a from commit. Also broke out eb23f55ae2470bd2376307ca293feb0be972de50 from test-commit.


---

879a8d3a6820a86e6faa8d
...

👍 hebasto approved a pull request: "ci: Split out native fuzz jobs for macOS and windows (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31221#pullrequestreview-2458051442)
ACK b031b7910d62819443813b91705211c466933a53.

💬 hebasto commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1856372906)
nit: Add a year?
```suggestion
# Copyright (c) 2024-present The Bitcoin Core developers
```

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1856388559)
Took the comment and expanded it a bit.

💬 dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1856390207)
I'll leave this as is unless another reviewer prefers having the year as well.

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2497685083)
I added a comment based on https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1855741271 to clarify under which circumstances `m_tip_block` is unset at this init stage.

I find it quite confusing that `ActivateBestChain` is called by `ImportBlocks` even if no blocks are imported, but haven't dared to refactor it.

💬 maflcko commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2497727233)
> I find it quite confusing that `ActivateBestChain` is called by `ImportBlocks` even if no blocks are imported, but haven't dared to refactor it.

It is explained in the code comment above the line that does it. It was added as a fix for https://github.com/bitcoin/bitcoin/issues/2239

💬 fanquake commented on pull request "depends, refactor: Avoid hardcoding `host_prefix` in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31358#issuecomment-2497736026)
Is there any particular motivation (to do this now), given it doesn't work/can't be tested?

🤔 rkrux reviewed a pull request: "include verbose "reject-details" field in testmempoolaccept response"
(https://github.com/bitcoin/bitcoin/pull/28121#pullrequestreview-2458147883)
I want to test this out, do you mind rebasing it so that the `CMake` build changes are in this as well?

```
➜ bitcoin git:(tma-debug) ✗ bitcoinmakeall
CMake Error: The source directory "/Users/rkrux/projects/bitcoin" does not appear to contain CMakeLists.txt.
Specify --help for usage, or press the help button on the CMake GUI.
make: *** [cmake_check_build_system] Error 1
```

💬 rkrux commented on pull request "include verbose "reject-details" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1856436682)
Forgot to push?

💬 hebasto commented on pull request "depends, refactor: Avoid hardcoding `host_prefix` in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31358#issuecomment-2497746776)
> Is there any particular motivation (to do this now), given it doesn't work/can't be tested?

To achieve the same quality in `toolchain.cmake` as it was with `config.site` in Autotools.

💬 maflcko commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2497769121)
re-lgtm ACK b031b7910d62819443813b91705211c466933a53

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856458151)
Thanks! Switched to use `APT_LLVM_V`.

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856459919)
This change has been removed from this PR.

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856460150)
This change has been removed from this PR.

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1856462103)
This change has been removed from this PR.

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#issuecomment-2497780660)
@maflcko @l0rinc

Thank you for your reviews. The PR has been updated per your feedback.

💬 maflcko commented on issue "Tracepoint Interface Tracking Issue":
(https://github.com/bitcoin/bitcoin/issues/31274#issuecomment-2497785934)
There is also a `TRACE_RAII` idea from https://github.com/bitcoin/bitcoin/pull/28226, mostly for performance/timing tracking in that example.

💬 vasild commented on issue "Discover() will not run if listening on any address with an explicit bind=0.0.0.0":
(https://github.com/bitcoin/bitcoin/issues/31293#issuecomment-2497792487)
@andremralves good observation! `Discover()` seems to be ok, but the test has rotten because it is not run regularly in CI (it was clear upfront that this may happen, but still it is better to have it than not, maybe it can be made to be run by CI if we can `ifconfig lo:0 1.1.1.1/32 up` in the CI?).

At some point the test framework was changed to add some `-bind`s:

https://github.com/bitcoin/bitcoin/blob/2638fdb4f934be96b7c798dbac38ea5ab8a6374a/test/functional/test_framework/test_node.py#L
...

💬 maflcko commented on pull request "refactor: Fix remaining clang-tidy performance-unnecessary-copy-initialization errors":
(https://github.com/bitcoin/bitcoin/pull/31364#issuecomment-2497801969)
I don't think you need to repeat the full diff (or a variation of it) in the commit message. If someone was interested in the detailed diff, they could just look at the diff.