🤔 ismaelsadeeq reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2821963515)
Concept ACK

💬 maflcko commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2077794853)
nit: "should recorded" looks like a typo

should recorded → should record

💬 maflcko commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2072667393)
nit in 93563ecef5e1be1522d4784b0092737487156bab: wallt → wallet

💬 maflcko commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2072667513)
tranasctions → transactions

🤔 pablomartin4btc reviewed a pull request: "Remove the legacy wallet and BDB dependency"
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2822045930)
post-merge ACK de054df6dc32cbd8b127c6761d9c65d95025e08f

🤔 hodlinator reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2821205668)
Concept ACK 81c0b9edfe533afbb2f4dda56142afdedffdb347

Introducing a `bitcoin` wrapper command seems like a good step to improve discoverability of the project's functionality.

Only briefly tested on NixOS (Linux) for now.

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077503435)
nit: Could use util/string.h:
```suggestion
for (const auto& element : Split(std::string_view{path_env}, ':')) {
fs::path candidate = fs::path{std::string_view{element.data(), element.size()}} / argv0_path;
```

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077515690)
nit:
Could keep style consistent with function body rather than wrapped function.
```suggestion
int ExecVp(const char* file, char* const argv[])
```

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077299356)
Potential for use after free?

This relies on `escaped_args`-elements being moved around with the backing `char`-buffers remaining in place as the vector is resized. It probably holds for all stdlib-implementations by now, but might be slightly safer to declare and fill `new_argv` after this loop, or resize `escaped_args` ahead of the loop.

Maybe there's some standardeese reason this is guaranteed to work that I'm unaware of.

Could use `const_cast`?

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077548018)
nit: Could verify we succeeded:
```suggestion
assert(path.has_parent_path()); // Failed resolving directory.
}
```

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077565200)
nit: Wrap in `#ifdef WIN32`?

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077800405)
nit: Child executable names leak through

```
₿ ./build/bin/bitcoin rpc --help
Bitcoin Core RPC client version v29.99.0-81c0b9edfe53

The bitcoin-cli utility provides a command line interface to interact with a Bitcoin Core RPC server.

It can be used to query network information, manage wallets, create or broadcast transactions, and control the Bitcoin Core server.

Use the "help" command to list all commands. Use "help <command>" to show help for that command.
The -named option allo
...

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077517153)
nit:
```suggestion
}
```

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077560730)
Would be nice if we printed an error when failing to execute anything at all.

(Nice lisp code. :) )

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077722254)
Is `argv` guaranteed to always end with an empty string?

💬 adamjonas commented on issue "bitcoin core crashes when too many rpc calls are made":
(https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-2858893784)
@johnnyasantoss what version were you running?

💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2858907702)
Rebased after #28710.

💬 polespinasa commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2077834027)
If you move the `start = time.time()` before connecting the test passes correctly.

💬 nick1981 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2858955700)
> After looking further into this issue; I can understand removing the limits as a default config in core (although i would prefer it not to be this way) - but why is the ability to configure it being removed all together?
>
> As a node runner I should be allowed to configure my mempool and not have it filled with what I consider spam. This can make running a node very RAM expensive.
>
> A conservative approach would be to let the defaults be as it is and allow it to be increased for those wh
...

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2077869573)
Taken in latest push.

💬 kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2077904842)
oops, thanks for the review!

updated in [ed4aef6](https://github.com/bitcoin/bitcoin/pull/32243/commits/ed4aef699801d79fb34211282007c87e4451b604)