💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-2896320121)
re: https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-2895479357

> I found that `bitcoin-node` will (still) segfault if i try to stop it while the client is running.

Thanks for testing this! I created a new issue in https://github.com/bitcoin-core/libmultiprocess/issues/176 to help debug. So far I couldn't reproduce the problem running a very basic client there, but I can try to extend it to do more of the operations your client is doing and see if I can make the problem happen. I
...

🤔 maflcko reviewed a pull request: "Fees: add Fee rate Forecaster Manager"
(https://github.com/bitcoin/bitcoin/pull/31664#pullrequestreview-2856553062)
(from the llm linter)

💬 maflcko commented on pull request "Fees: add Fee rate Forecaster Manager":
(https://github.com/bitcoin/bitcoin/pull/31664#discussion_r2099527580)
unable a fee rate -> unable to find a fee rate [Incorrect grammar; "unable to find" or "unable to provide" would be clearer.]

💬 maflcko commented on pull request "Fees: add Fee rate Forecaster Manager":
(https://github.com/bitcoin/bitcoin/pull/31664#discussion_r2099526827)
could remove the redundant comments, given that at least one of them is wrong?

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-2896845391)
concept ack, but yeah, needs rebase

💬 Sjors commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2896961033)
utACK b23cefb4f3abe33c7bc933b60f1c0d15137c627c

💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2099616827)
Sounds fine, forgot to push this?

💬 maflcko commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2896998818)
> I wonder if there is a platform without `posix_fallocate` (outside of macos and windows). If not, the fallback can probably be fully removed, along with the detection and it can just be assumed to be always present?

I am asking, because the fallback was just the normal code in the beginning (bba89aa82a80f0373dcb7288d96d5b0fcb453d73) and was not removed in 288fdc092aff9d7e0cea159196b2e96044a786c7.

However, even bioniC has `posix_fallocate` since API level 21 (https://android.googlesource
...

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