💬 maflcko commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2897032758)
> And the other slimmer libcs may not have `ftruncate` (https://www.sourceware.org/newlib/libc.html), so they can't be used anyway.

Actually, it is used in https://github.com/bitcoin/bitcoin/pull/31425, so I wonder how that compiles and links. I guess it will start failing once more than a trivial dummy code is linked.

💬 vasild commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2099675521)
Did you try to start the timer before connecting? Like this:

```diff
# Get the underlying socket from HTTP connection so we can send something unusual
conn = http.client.HTTPConnection(urlNode2.hostname, urlNode2.port)
+ start = time.time()
conn.connect()
sock = conn.sock
sock.sendall(http_request.encode("utf-8"))
# Wait for response, but expect a timeout disconnection after 1 second
- start = time.time()
re
...

💬 vasild commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2099677203)
_continued the discussion in https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2081584805_

💬 maflcko commented on pull request "RFC: Riscv bare metal CI job":
(https://github.com/bitcoin/bitcoin/pull/31425#issuecomment-2897104226)
Coming from https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2897032758, I noticed that https://www.sourceware.org/newlib/libc.html doesn't list `ftruncate`, so I guess the build will fail once more code is compiled/linked?

💬 fanquake commented on pull request "RFC: Riscv bare metal CI job":
(https://github.com/bitcoin/bitcoin/pull/31425#issuecomment-2897113177)
I think the docs might be incomplete / outdated? See https://github.com/bminor/newlib/blob/b39b510c1ce68757e79410585262ca2cd48da839/newlib/libc/include/sys/unistd.h#L287.

💬 maflcko commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2897127443)
> > `createrawtransaction` doesn't do other policy checks either, such as OP_RETURN checks, so one could argue this is fine as well.
>
> `fundrawtransaction` does though, and that was my point.

I don't think `fundrawtransaction` checks for OP_RETURN. At least locally, the following passes for me:

```
createrawtransaction '[]' '[{"data":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
...

👍 vasild approved a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2856867196)
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2099737369)
This can be written like:

```cpp
util::ExecVp(exec_args[0], (char*const*)exec_args.data()); // If it returns then an error has occurred.

if (allow_notfound && errno == ENOENT) return false;
throw std::system_error(errno, std::system_category(), strprintf("execvp failed to execute '%s'", exec_args[0]));
```
it is less indentation and less code - no need for the "this will never be reached" `throw`.

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2099742506)
super nit, but I can't resist :rescue_worker_helmet: :

"null terminated strings" and "null terminated" array -- it is not the same "null":

```suggestion
//! array should consist of '\0'-terminated strings and be nullptr-terminated
//! itself, like the POSIX function.
```
:runner:

💬 vasild commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#issuecomment-2897167995)
`626cc06c69...528b4d6b9c`: rebase and do https://github.com/bitcoin/bitcoin/pull/29418#discussion_r2098827990

🚀 fanquake merged a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622)

💬 fanquake commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2897179032)
> But, it probably would still make sense to close https://github.com/bitcoin/bitcoin/issues/32391.

Yea, on our side, this should be enough to close #32391.

💬 fanquake commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-2897204778)
Reading the description, I'm still not really clear on the goals here. It seems like this is just adding even more ways to compile for Windows, which we have to add more CI / and more work arounds for, but doesn't actually improve the Windows binaries we are shipping to end users in any way. This might make more sense if we were actually going to switch to shipping Windows binaries using Clang.

🚀 fanquake merged a pull request: "qt: drop unused watch-only functionality"
(https://github.com/bitcoin/bitcoin/pull/32459)

✅ maflcko closed an issue: "Dropping unused legacy wallet code"
(https://github.com/bitcoin-core/gui/issues/873)

💬 maflcko commented on issue "Dropping unused legacy wallet code":
(https://github.com/bitcoin-core/gui/issues/873#issuecomment-2897268477)
> In that case I can close this once [bitcoin/bitcoin#32459](https://github.com/bitcoin/bitcoin/pull/32459) is merged, unless there's another issue found by then.

🤔 janb84 reviewed a pull request: "qa: feature_framework_startup_failures.py fixes & improvements (#30660 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/32509#pullrequestreview-2857005161)
LGTM ACK https://github.com/bitcoin/bitcoin/commit/bf950c4544d3a8478b46faf0b93c0dc647274c9b

- code review ✅
- build and tested ✅

Improved code readability and addressed open issues from #30660

🤔 janb84 reviewed a pull request: "ci: Avoid && dropping errors"
(https://github.com/bitcoin/bitcoin/pull/32573#pullrequestreview-2857038264)
Code review ACK https://github.com/bitcoin/bitcoin/commit/fab97f583f119f43da352774479dd78e39729632

💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-2897330791)
I have used `ApacheBench 2.3` for benchmarking REST API, and the following Rust client for `getrawtransaction` RPC:

# fetching using the new index
```
$ ab -k -c 1 -n 100000 http://localhost:8332/rest/txfromblock/0000000000000000000083a0cff38278aae196d6d923a7e8ee7e5a0e371226fe-42.bin

Document Path: /rest/txfromblock/0000000000000000000083a0cff38278aae196d6d923a7e8ee7e5a0e371226fe-42.bin
Document Length: 301 bytes

Concurrency Level: 1
Time taken for tests: 13.7
...

👍 hebasto approved a pull request: "ci: Avoid && dropping errors"
(https://github.com/bitcoin/bitcoin/pull/32573#pullrequestreview-2857077463)
ACK fab97f583f119f43da352774479dd78e39729632.

Could this approach be forced by a linter?