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

💬 kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2077905380)
Yes that makes sense to me!

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

💬 kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#issuecomment-2859026107)
> Good stuff, nice to have new coverage.
>
> I think it'd be worth it to add an oracle in the test where you reconstruct the merkle root with the path you get from `TransactionMerklePath` and verify that it matches the root from the earlier `ComputeMerkleRoot` call.

Thank you for the review!

In [ed4aef6](https://github.com/bitcoin/bitcoin/pull/32243/commits/ed4aef699801d79fb34211282007c87e4451b604) I added a helper function ComputeMerkleRootFromPath to help rebuild the root so then I co
...

💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2859036255)
Rebased after https://github.com/bitcoin/bitcoin/pull/28710

💬 fanquake commented on pull request "crypto: disable ASan for sha256_sse4 with Clang":
(https://github.com/bitcoin/bitcoin/pull/32437#issuecomment-2859044849)
Guix build:
```bash
20608607794819e22c0bc02c920ba03a4edf98b902e278f1e2dcc9d92f2f485f guix-build-4e8ab5e00fa7/output/aarch64-linux-gnu/SHA256SUMS.part
f3268558a957a2b55191b14bdb9f7c23d60dc72a84ecbfe6b56a280f8f09fd1f guix-build-4e8ab5e00fa7/output/aarch64-linux-gnu/bitcoin-4e8ab5e00fa7-aarch64-linux-gnu-debug.tar.gz
791e1b3e81b358343dd5fafa53ab1d503e819d6935515277443dcfef4091c470 guix-build-4e8ab5e00fa7/output/aarch64-linux-gnu/bitcoin-4e8ab5e00fa7-aarch64-linux-gnu.tar.gz
1b689d1c92f6026d
...