💬 maflcko commented on pull request "test: Add and use option for tx-version in MiniWallet methods":
(https://github.com/bitcoin/bitcoin/pull/28972#issuecomment-1836208727)
It is also an alternative to ab2c3eafb7ccadeb3588a89e6383fc751e6578bd (cc @glozow )

💬 Sjors commented on pull request "bugfix, Change up submitpackage results to return results for all transactions":
(https://github.com/bitcoin/bitcoin/pull/28848#issuecomment-1836284078)
Light re-utACK f23ba24aa079d68697d475789cd21bd7b5075550

💬 ryanofsky commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-1836293866)
re: https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-1823568385 and earlier discussion about libcrypto starting https://github.com/bitcoin/bitcoin/pull/28690#pullrequestreview-1734365185:

> I agree that crypto isn't the most ideal home for hex/string functions, but it _does_ actually make it easier to use the lib as a standalone and not have to write your own versions.

Just want to note that `libbitcoin_crypto` and `src/crypto/` are not currently mentioned in the libraries docume
...

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1412243707)
Just adding a comment with some context, saying that for chances lower than 1% it may not be worth the computation may be fine. It just struck me as weird where that value came from.

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1412244443)
Fair

💬 hebasto commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836332387)
> Is the plan to also delete any unused code from this header?

I haven't consider it yet.

> it maybe already doesn't work with c++20?

CI jobs that uses C++20 are passing so far.

💬 sipa commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836337134)
> Is the plan to also delete any unused code from this header?

If at all possible, I'd prefer for it to be subtreed, so that updating to newer upstream versions is easy.

💬 fanquake commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836341684)
I was actually thinking the opposite. I think a subtree is overkill here, and, upstream isn't exactly active (some random merges but no development), so I'm not sure if there will be many new versions.

I'd rather take the small(er) portion of any code we need, and have some better integrated/reviewed. Hopefully that will help avoid randomly pulling code from upstream, that isn't well developed or reviewed.

💬 fanquake commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836346184)
Note that the last version from upstream was > 3 years ago.

💬 hebasto commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836346980)
> I'd rather take the small(er) portion of any code we need, and have something better integrated/reviewed.

Maybe a subtree from our own fork? (to avoid bringing testing stuff to the main repo)

💬 fanquake commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836353115)
> Maybe a subtree from our own fork?

I think that would mostly just re-introduce the awkwardness we ended up with with univalue, for not much benefit, and for code that shouldn't be critical (i.e compared to leveldb & libsecp256k1), this is for an optional feature, that is not used by a large proportion of the people running this software.

👍 ryanofsky approved a pull request: "depends: Build the `native_capnp` and `capnp` packages with CMake"
(https://github.com/bitcoin/bitcoin/pull/28856#pullrequestreview-1760050677)
Code review ACK d1604d4b1d1ee8df279a1776303e167cc3d06193. I left a question and suggestion, but they are not critical. Overall seems good to switch from autotools to cmake here. I'm actually not sure why I didn't write this using cmake originally, since the cmake build has been around since capnproto 0.5. It also seems good to get rid of the special --disable-shared flag for android, since that was inconsistent with other platforms.

I think I understand how this change works around the mingw3
...

💬 ryanofsky commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#discussion_r1412254941)
In commit "depends: Build `capnp` package with CMake" (2cdb2f8bd6aaff64803482e4128da34db34effa8)

Is this needed? It seems like a potential bug in libmultiprocess if its build system is getting confused by the pkgconfig install files.

💬 ryanofsky commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#discussion_r1412257852)
In commit "depends: Build `capnp` package with CMake" (2cdb2f8bd6aaff64803482e4128da34db34effa8)

This change seems like a good way to speed things up by only building the libmultiprocess runtime library, not the code generation tool in the non-native cross build (assuming that is the intention here). But it would be good to write this in a comment so it is clear what is going on.

💬 TheCharlatan commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836362178)
Re https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836341684

> I'd rather take the small(er) portion of any code we need, and have something better integrated/reviewed.

Looking at the coverage report of the header, there seems to be some potential for pruning out (89.2% function coverage and 67.4% line coverage),

💬 hebasto commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#discussion_r1412304189)
> Is this needed?

No, it isn't. However, we usually try to minimize a prefix size by dropping unneeded stuff.

👍 stickies-v approved a pull request: "init: don't delete PID file if it was not generated"
(https://github.com/bitcoin/bitcoin/pull/28946#pullrequestreview-1760140851)
ACK 2b28f7df2b7dd140e52e93ef3e1a330db8da32a9

💬 stickies-v commented on pull request "init: don't delete PID file if it was not generated":
(https://github.com/bitcoin/bitcoin/pull/28946#discussion_r1412308897)
nit: I think this more clearly explains the rationale without being too explicit about implementation
```suggestion
//! True if this process created the pid file, so we don't delete it too early if multiple processes were started
*/
```

💬 fanquake commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#issuecomment-1836390190)
> but I don't understand how this would fix either of the two problems the closed PR

You are right, this PR doesn't fix at least the first problem, which is configure failing to find libmultiprocess:
```bash
checking for libmultiprocess... no
configure: error: --enable-multiprocess=yes option specified but libmultiprocess library was not found. May need to install libmultiprocess library, or specify install path with PKG_CONFIG_PATH environment variable. Running 'pkg-config --debug libmult
...

💬 ryanofsky commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#issuecomment-1836421425)
> You are right, this PR doesn't fix at least the first problem, which is configure failing to find libmultiprocess:

Make sense, I think the first commit from #28846 is a good fix for that problem.