💬 TheCharlatan commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1747630899)
It is capitalized to match the substitution in the pkg-config file. I had the impression the substitution variables are usually capitalized as a convention.

💬 TheCharlatan commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#issuecomment-2334702440)
Updated 766881e33d13ca2887f7ed179cdcc6bc007a6325 -> 0dd16d7118f10ac291a45644769121cbdfd2352f ([staticKernel_0](https://github.com/TheCharlatan/bitcoin/tree/staticKernel_0) -> [staticKernel_1](https://github.com/TheCharlatan/bitcoin/tree/staticKernel_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/staticKernel_0..staticKernel_1))

* Addressed @hebasto's [comment](https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1747381222), fixed spacing around `if` statements.
* Address
...

👍 hebasto approved a pull request: "kernel: Create usable static kernel library"
(https://github.com/bitcoin/bitcoin/pull/30814#pullrequestreview-2287050210)
ACK 0dd16d7118f10ac291a45644769121cbdfd2352f.

💬 TheCharlatan commented on pull request "build: Introduce "Kernel" installation component":
(https://github.com/bitcoin/bitcoin/pull/30835#discussion_r1747648065)
Why call this `Kernel` and not `bitcoinkernel`?

🤔 TheCharlatan reviewed a pull request: "test: Work around boost compilation error"
(https://github.com/bitcoin/bitcoin/pull/30834#pullrequestreview-2287065362)
Nice, post-merge ACK

👍 TheCharlatan approved a pull request: "multiprocess: Add -ipcbind option to bitcoin-node"
(https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2287066648)
Re-ACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d

💬 hebasto commented on pull request "build: Introduce "Kernel" installation component":
(https://github.com/bitcoin/bitcoin/pull/30835#discussion_r1747651146)
For brevity. Do you want me to update it?

💬 TheCharlatan commented on pull request "build: Introduce "Kernel" installation component":
(https://github.com/bitcoin/bitcoin/pull/30835#discussion_r1747665388)
I don't think it matters really what it is called. As far as I know there is no way to list all available install components from the command line and discover its name. With the targets you can at least do `cmake --build build --target help`, so my idea was that this might make it more discoverable. But a component might also include multiple targets, so I don't think colliding their names here really helps in terms of making it clearer. So in short, no need to change.

👍 TheCharlatan approved a pull request: "build: Introduce "Kernel" installation component"
(https://github.com/bitcoin/bitcoin/pull/30835#pullrequestreview-2287088274)
ACK 7b04fabe2d95f05a295b1ef30c9aeab7f753ed46

👍 hebasto approved a pull request: "cmake: add `USE_SOURCE_PERMISSIONS` to all `configure_file()` usage"
(https://github.com/bitcoin/bitcoin/pull/30823#pullrequestreview-2287096217)
ACK 1f054eca4e779cfa5f9f6e9278071adf65e5eafe.

> Looks like this is not the solution for the reproducibility issue

The PR description rightfully stated that "this should not change behaviour".

📝 hebasto opened a pull request: "build: Use CMake's default permissions in macOS `deploy` target"
(https://github.com/bitcoin/bitcoin/pull/30838)
This PR is an attempt to fix https://github.com/bitcoin/bitcoin/issues/30815.

Based on https://github.com/bitcoin/bitcoin/pull/30823.

💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1747656292)
Did you move these out of here because you'd otherwise get the following error:
```
*** Uncaught exception ***
capnp/compiler/node-translator.c++:1374: context: member.name = arg
kj/filesystem.c++:315: failed: expected parts.size() > 0 [0 > 0]; can't use ".." to break out of starting directory
stack: 59a492c93c94 59a492c93fc7 59a492c916b3 59a492a192f8 59a492b094db 59a492b08eb6 59a492b35fad 59a492b431c8 59a492b043ad 59a492b085a9 59a492b07e69 59a492b080d7 59a492b35b7f 59a492b36834 59a492b4302
...

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2334759967)
This is a confusing CI failure:

```
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-tx.1
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-cli.1
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoind.1
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-qt.1
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-util.1
+ false
```

https://cirrus-ci.com/task/6743063731634176


It's
...

👍 TheCharlatan approved a pull request: "cmake: add `USE_SOURCE_PERMISSIONS` to all `configure_file()` usage"
(https://github.com/bitcoin/bitcoin/pull/30823#pullrequestreview-2287108659)
ACK 1f054eca4e779cfa5f9f6e9278071adf65e5eafe

💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2334764375)
> This is a confusing CI failure:

I'm guessing you saw the actual error further up?
```
In file included from /ci_container_base/src/test/ipc_test.cpp:7:
/ci_container_base/ci/scratch/build-i686-pc-linux-gnu/src/test/ipc_test.capnp.h:18:10: fatal error: '../ipc/capnp/mining.capnp.h' file not found
18 | #include "../ipc/capnp/mining.capnp.h"
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[ 5%] Building CXX object CMakeFiles/minisketch.dir/src/minisketch/src/fields/generic_4bytes.cpp.
...

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1747285877)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1732637930

> Nit (clang-format): Missing space after `template`.

Thanks, fixed!

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1747694722)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1747656292

> Did you move these out of here because you'd otherwise get the following error:

Yes, that's the reason. It doesn't seem to be possible to access `src/ipc/` files from the files in the `src/test/` directory by writing `../ipc/` when the `capnp compile` source prefix is `src/test/`, but it does work if the source prefix is `src/`.

I think a different way to work around this would be to pass `--import-path=/path/to/
...

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2334787042)
re: https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2334764375

> I'm guessing you saw the actual error further up?

No thanks. I missed that. That is also confusing but at least it's something to work with. I'm sure why it doesn't happen in normal non-CI builds or seem to happen in the followup PR #30437. Will try to reproduce and fix

💬 jonatack commented on pull request "Add NODE_TXRELAY_V2.":
(https://github.com/bitcoin/bitcoin/pull/30837#discussion_r1747701520)
(in case you're not waiting for concept acks)

```
/src/protocol.cpp:94:13: error: enumeration value 'NODE_TXRELAY_V2' not handled in switch [-Werror,-Wswitch]
94 | switch ((ServiceFlags)service_flag) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
```

💬 hebasto commented on issue "cmake: passing options from depends build to main build":
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2334792022)
> it seems like a very clean one is to set the [`CMAKE_USER_MAKE_RULES_OVERRIDE`](https://cmake.org/cmake/help/v3.3/variable/CMAKE_USER_MAKE_RULES_OVERRIDE.html) hook

FWIW, something similar was [considered](https://github.com/hebasto/bitcoin/pull/240) during work on the CMake staging branch.