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

🤔 ryanofsky reviewed a pull request: "test: Shut down framework cleanly on RPC connection failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2532868324)
Code review 608b9ff097214bff3527cc240b7be313c62131d5

Nice changes in this PR. I've been confused by the error output from wait_for_rpc_connection() before, so think this will be helpful.

One thing that wasn't obvious to me from the PR description is that this improving error output in two separate ways:

- One change here is *adding* error output to `wait_for_rpc_connection`. Previously errors that happened during polling before the final "Unable to connect to bitcoind" error were silent
...

💬 ryanofsky commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1904579994)
In commit "test refactor: wait_for_rpc_connection - Treat OSErrors the same" (5321eb089cd1b39160038050a6ef6f27fd6f4ef9)

Found both this comment "cookie file is missing" and previous comment "cookie file not found" confusing because it is not clear why the cookie file would not be present. IMO saying "node has not generated the cookie file yet" would be clearer.

💬 ryanofsky commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1904607706)
In commit "test: Allow changing expected internal return code" (db0e1580a5212a162908ac4f23e935e1f76e06dc)

Now that this logic is getting more complicated with `self.expected_ret_code` I think the `expect_error` argument should be dropped so the code can be more understandable. It looks like there is only a single `expect_error` usage in `feature_abortnode.py` and it can be easily replaced by using `expected_ret_code`. Would suggest:

```diff
--- a/test/functional/feature_abortnode.py
+++
...

💬 ryanofsky commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1905811915)
In commit "test: Add feature_framework_rpc_failure.py" (f67d8963575feecdd4914c1a56bab4e0bebc000d)

Note: there is no test coverage for the "ignored errors" and "latest error" parts of the message. I think it would be good to add some coverage for this, even if it just checking for 0 and None, so there is a at least a hint that more coverage could be added here, especially since it's possible to imagine this functionality becoming broken in the future and more coverage helping prevent this.