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

👍 Sjors approved a pull request: "policy: make pathological transactions packed with legacy sigops non-standard"
(https://github.com/bitcoin/bitcoin/pull/32521#pullrequestreview-2857047398)
ACK 29e8fe71440a7e27ee89527a49753be615f205c7

💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2099855247)
29e8fe71440a7e27ee89527a49753be615f205c7: note to other reviewers, yes `generate` checks for errors, as you can see if you duplicate `nonstd_tx.serialize().hex()`

💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2099858669)
29e8fe71440a7e27ee89527a49753be615f205c7: `MAX_TX_LEGACY_SIGOPS` would be consistent with the c++ code

💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2099863081)
a139a538c298621674ba622e63971704cbb7ceff: what's this magic `424242` value?