👍 ryanofsky approved a pull request: "build: disable bitcoin-node if daemon is not built"
(https://github.com/bitcoin/bitcoin/pull/31834#pullrequestreview-2606125408)
Code review ACK 2ffea09820e66e25ab639c9fc14810fd96ad6213

💬 maflcko commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2648315209)
Given all the GCC/lcov/gcov problems, I wonder if it wouldn't be easier to just go with clang/llvm:

* Require compilation with `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DCMAKE_CXX_FLAGS='-fPIC -fprofile-instr-generate -fcoverage-mapping'`
* apply the below diff

```diff
diff --git a/contrib/devtools/test_deterministic_coverage.sh b/contrib/devtools/test_deterministic_coverage.sh
index 885396bb25..48d331ae70 100755
--- a/contrib/devtools/test_deterministic_coverage.sh

...

🤔 stratospher reviewed a pull request: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2598002988)
reACK 4ba2e48.

💬 stratospher commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r1944318554)
eca8d91: micro-style-nit only if you have to retouch - could make double (()) to ().

💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2648416127)
> Attempted to run Guix build [e717fb5](https://github.com/bitcoin/bitcoin/commit/e717fb5a429d9d00e008fa1eb2b85179050be8fe) cross-built for Windows, but it fails already on test "0" / line 133:

Is this failure introduced by this PR, or does it occur on the master branch as well?

> Was able to find a solution on [StackOverflow](https://stackoverflow.com/questions/69560731) - activated _Developer Mode_ in Windows settings to avoid it.

There are many [ways](https://neacsu.net/posts/win_sym
...

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