🤔 ryanofsky reviewed a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2606185530)
Rebased e7743aa5df6387964c23f0629debf8c3cbb8ed4c -> e484d0bd11408489d83aaecf49a238f98a117696 ([`pr/subtree.13`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.13) -> [`pr/subtree.14`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.14), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree.13-rebase..pr/subtree.14)) adding https://github.com/chaincodelabs/libmultiprocess/pull/161 and trying another suggested readability improvement to depends code, hiding more capnp
...

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1949306180)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1948687873


> [1b36135](https://github.com/bitcoin/bitcoin/commit/1b3613505deb4baeacc3c4c2fc91ccca1a8ecec5)
>
> Usually, external dependencies follow the internal ones.

Makes sense, fixed this.

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1949305591)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1948609163

> nit: `$(1)_file_name)` -> `$(1)_file_name`
>
> Thanks for the comment. That line is still ~!@*&~(!@*&~&_ in my eyes. I wonder if it can be split into shorter and more easily understandable `define foo` functions...

Thanks for the close reading. Fixed comment and split this up to use a helper function. I think it should be more readable now.

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1949304691)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1948608064

> nit: ` | head -n1` is unnecessary

I think it's natural and important to have to prevent bad worst-case behavior. Having `head -n1` lets `find` command exit early and not continue looking for newer files after it has already found some, because its stdout will be closed. Also (probably more importantly) it prevents the shell from having to build a up a huge string with a list of new paths.

🤔 hebasto reviewed a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2606299867)
Approach ACK c6658d675d0bb945f53dc62f7e9b95c7bba973bb.

💬 josibake commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1949386041)
nit: `int` seems more appropriate here and I find `num_ecdsa` and `num_schnorr` to be more intuitive than `taproot` and `nontaproot`

```suggestion
std::pair<std::vector<CKey>, std::vector<CTxOut>> CreateKeysAndOutputs(const CKey& coinbaseKey, int num_ecdsa, int num_schnorr)
```

💬 josibake commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1949388434)
per the style guide in `doc/developer-notes.md`, `++I` is preferred over `i++`

```suggestion
for (int i{0}; i < num_schnorr; ++i) {
```

(same feedback for the other loops below)

👍 josibake approved a pull request: "Benchmark Chainstate::ConnectBlock duration"
(https://github.com/bitcoin/bitcoin/pull/31689#pullrequestreview-2606336089)
ACK https://github.com/bitcoin/bitcoin/pull/31689/commits/1c6b886465df0f00549e7d10c3bfefd27be7f1c2

I spent some time trying to come up with a way to create a mixed block of all schnorr input transactions and all ecdsa input transactions (as opposed to transactions with both schnorr and ecdsa inputs), but I couldn't find a simple way to do it. Furthermore, it felt unnecessary for what we are trying to accomplish: establish a baseline for signature validation of blocks with both schnorr and ecd
...

📝 maflcko opened a pull request: "contrib: Add deterministic-fuzz-coverage"
(https://github.com/bitcoin/bitcoin/pull/31836)
The goal of this script is to detect and debug the remaining fuzz determinism and stability issues (https://github.com/bitcoin/bitcoin/issues/29018).

💬 brunoerg commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2648581344)
Concept ACK

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2648581728)
I just enabled the fuzz test, which fails:

```
[11:23:22.695] [ 95%] Linking CXX executable fuzz
[11:23:25.986] [ 95%] Built target multiprocess
[11:23:28.177] [ 95%] Linking CXX executable mpgen
: CMakeFiles/mpgen.dir/src/mp/gen.cpp.o: in function `':
[11:23:28.379] /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_vector.h:734: multiple definition of `main; /usr/lib/llvm-20/lib/clang/20/lib/linux/libclang_rt.fuzzer-x86_64.a(FuzzerMain.cpp.o):(.text.main+0x0): first d
...

💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2648593899)
cc @marcofleon @dergoegge (Checking in to see if you have something similar already?)

💬 ryanofsky commented on issue "Rename bitcoin-wallet?":
(https://github.com/bitcoin/bitcoin/issues/31827#issuecomment-2648600548)
Thanks for opening this issue. The [multiprocess tracking issue](https://github.com/bitcoin/bitcoin/issues/28722) has a section called "PRs for review related to building & releasing" and if you look at two PRs there https://github.com/bitcoin/bitcoin/pull/31375 and https://github.com/bitcoin/bitcoin/pull/31679 they should clear these questions up if you want to see exactly what changes are planned and implemented.

But basically, there is no need to rename any current command line tools or brea
...

💬 Sjors commented on issue "Rename bitcoin-wallet?":
(https://github.com/bitcoin/bitcoin/issues/31827#issuecomment-2648632118)
Maybe to phrase it differently, it seems that the idea is to expand the current `bitcoin-wallet` to support IPC, as implemented in #19460. So there's no name clash.

💬 ryanofsky commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2648649632)
re: https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2648581728

Thanks I guess this is not too surprising. I think you can try the following change, and if it works and seems like the right approach. I can add it to the base PR.

<details><summary>diff</summary>
<p>

```diff
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -150,6 +150,8 @@ if(ENABLE_IPC AND WITH_EXTERNAL_LIBMULTIPROCESS)
)
endif()

+cmake_dependent_option(BUILD_IPC_TESTS "Build IPC test executables." ON
...

💬 ryanofsky commented on issue "Rename bitcoin-wallet?":
(https://github.com/bitcoin/bitcoin/issues/31827#issuecomment-2648684591)
> Maybe to phrase it differently, it seems that the idea is to expand the current `bitcoin-wallet` to support IPC, as implemented in [#19460](https://github.com/bitcoin/bitcoin/pull/19460). So there's no name clash.

I just think that's an implementation detail and there's no need to break the command line or rename binaries in any case.

There are two cases (1) wallet ipc features (like connecting to a node to perform some online operation, or starting an dedicated RPC server) are implemented i
...

💬 danielabrozzoni commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1949530919)
This is way nicer! Updated, thanks

🤔 danielabrozzoni reviewed a pull request: "logging: Ensure -debug=0/none behaves consistently with -nodebug"
(https://github.com/bitcoin/bitcoin/pull/31767#pullrequestreview-2606566000)
Thank you for the reviews! In my last push:
- I removed every occurences of "rpc" (sorry I didn't catch them the first time!)
- I've renamed a couple of variables, added a comment, and addressed @hodlinator's nits, to improve clarity

💬 danielabrozzoni commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1949532227)
I find this a bit harder to read, so I'll keep it as it is.

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2648772960)
@ryanofsky ab831be2a93d9c30408192ca4eaa1552ac6bc3dc didn't help, same `undefined reference to `LLVMFuzzerTestOneInput'`: https://cirrus-ci.com/task/5233064575500288?logs=ci#L3205