💬 vasild commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1905783548)
Is `bpf.perf_buffer_poll(timeout=400)` needed now that there is `self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == 0)`? If not needed, then better drop it.

💬 hebasto commented on pull request "depends: Avoid hardcoding `host_prefix` in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31358#issuecomment-2575795693)
> It does issue a warning though (at least when you don't build the GUI):
>
> ```
> CMake Warning:
> Manually-specified variables were not used by the project:
>
> CMAKE_TOOLCHAIN_FILE
> ```

Are you sure that you build directory is clean before the first `cmake` invocation?

💬 vasild commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1905788722)
Non blocker comment: this cast from `uint64_t` to `(void*)` looks odd, maybe it will produce compiler warning in some cases? Also, on 32 bit systems that should be `uint32_t`. Can these variables be declared as `void*` instead? Like:

```suggestion
void* conn_type_pointer;
void* address_pointer;
bpf_usdt_readarg(1, ctx, &inbound.conn.id);
bpf_usdt_readarg(2, ctx, &address_pointer);
bpf_usdt_readarg(3, ctx, &conn_type_pointer);
bpf_usdt_readarg(4, ctx, &inbound.con
...

👍 vasild approved a pull request: "tracing: network connection tracepoints"
(https://github.com/bitcoin/bitcoin/pull/25832#pullrequestreview-2534935820)
ACK 15dfd72f8ff47736a13c80743d5d132fd2763ecb (modulo to lint failure)

💬 vasild commented on pull request "ci: build msan's libc++ with _LIBCPP_ABI_BOUNDED_*":
(https://github.com/bitcoin/bitcoin/pull/31612#issuecomment-2575812488)
Excellent, that will simplify https://github.com/bitcoin/bitcoin/pull/31424

Thank you!

👍 hebasto approved a pull request: "doc: Clarify min macOS and Xcode version"
(https://github.com/bitcoin/bitcoin/pull/31608#pullrequestreview-2534950260)
re-ACK fa029a78780fd00e1d3ce1bebb81a95857bfbb94.

💬 fanquake commented on pull request "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree":
(https://github.com/bitcoin/bitcoin/pull/31379#issuecomment-2575819149)
> I guess I'm a little confused about how this should work. It seems it's not very consistent.

> So.. how should this all work ideally? Do we really need to forward the flags at all?

I agree. It'd be good to know what the goals are here, if some amount of this code is redundant, and what the status of this change is.

💬 vasild commented on pull request "build: enable libc++ and libstdc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1905810616)
All `_LIBCPP_ABI_BOUNDED*` additions have been removed from this PR.

💬 vasild commented on pull request "build: enable libc++ and libstdc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2575844233)
`39a1326488...1ef9f7ca4a`: drop the additions of `_LIBCPP_ABI_BOUNDED*` when compiling Bitcoin Core. See https://github.com/bitcoin/bitcoin/pull/31612#issuecomment-2575746681 for reasoning.

💬 l0rinc commented on pull request "init,log: Unify block index log line":
(https://github.com/bitcoin/bitcoin/pull/31616#discussion_r1905813574)
Good idea, [done](https://github.com/bitcoin/bitcoin/compare/2ee82dfdeb3777c1d1648daa7e009c420caf5973..60b7bbd3709519c6f2f8023a23ee0a194a5f0eb4)

📝 vasild opened a pull request: "ci: change the build to be verbose by default"
(https://github.com/bitcoin/bitcoin/pull/31619)
Previously CI would use a non-verbose build (where exact compilation and linking commands are not visible) and only if it fails, would rerun the build in verbose mode.

Change this to use verbose in the initial build as well for two reasons:
1. It is useful to be able to see the exact compilation and linking commands even for successful builds, to e.g. confirm some particular flags are (not) used as expected.
2. In failed builds, if the failure is during linking, the repeated verbose build m
...

💬 vasild commented on pull request "ci: change the build to be verbose by default":
(https://github.com/bitcoin/bitcoin/pull/31619#issuecomment-2575850927)
I actually needed this while working on https://github.com/bitcoin/bitcoin/pull/31424

💬 l0rinc commented on pull request "RPC: Fix invalid txid in `gettransaction` example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2575861575)
I understand the rationale for using placeholders instead of hard-coded values—especially for dangerous commands—and perhaps even correctly formatted ones that clearly don’t point to anything real. However, using slightly incorrect examples as guidance for harmless queries seems really counterproductive. Why would we want to deliberately mislead users? What’s the harm in providing a real example so they can see how a valid transaction should behave?

💬 Sjors commented on pull request "depends: Avoid hardcoding `host_prefix` in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31358#issuecomment-2575864267)
Mmm, not seeing this anymore, so maybe I did something wrong the first time.

After the depends are built:

```sh
rm -rf build
cmake -B build -DBUILD_GUI=ON --toolchain /Volumes/SSD/Dev/bitcoin/depends/x86_64-apple-darwin22.6.0/toolchain.cmake
```

Logs with and without GUI:

https://gist.github.com/Sjors/f4a493c57eb3ab5b62754c1d4e54a9c6

💬 maflcko commented on pull request "ci: change the build to be verbose by default":
(https://github.com/bitcoin/bitcoin/pull/31619#issuecomment-2575871852)
How is this different from just looking at the `C++ compiler flags ....................` output?

💬 jonatack commented on pull request "RPC: Fix invalid txid in `gettransaction` example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2575927475)
Valid addresses in RPC would be potentially more harmful due to the accidental risk of losing funds.

But in general, I believe the idea is to allow the RPC caller to invoke it with a value that is relevant to them / the use case of the software client, rather than an arbitrary bikesheddable real-life value, and by default to see an example of the RPC result with an invalid value provided.

💬 l0rinc commented on pull request "RPC: Fix invalid txid in `gettransaction` example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2575939284)
I don't see how "id should be 64 chars long not 63" is helpful to the users - but I agree that placeholders could be more in-line with other more dangerous commands.

💬 achow101 commented on pull request "[28.x] 28.1 backports and final changes":
(https://github.com/bitcoin/bitcoin/pull/31594#issuecomment-2575949969)
ACK 36314b8da2ee65afd5636fa830d436c5c22bd260

💬 Eunovo commented on pull request "Ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r1905885950)
That's true. I thought https://github.com/bitcoin/bitcoin/issues/31494 was focused on the best header chainwork not being up to minimumchainwork. I could change this to always skip script checks during a reindex, but IIUC having a block on disk doesn't mean that it was connected during the previous IBD. Might be worth it to sync headers first then connect blocks even during reindex.

🚀 achow101 merged a pull request: "[28.x] 28.1 backports and final changes"
(https://github.com/bitcoin/bitcoin/pull/31594)