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

💬 achow101 commented on pull request "bugfix, Change up submitpackage results to return results for all transactions":
(https://github.com/bitcoin/bitcoin/pull/28848#issuecomment-1836478642)
ACK f23ba24aa079d68697d475789cd21bd7b5075550

💬 achow101 commented on pull request "rpc: keep `.cookie` file if it was not generated":
(https://github.com/bitcoin/bitcoin/pull/28784#issuecomment-1836489552)
ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d

🚀 achow101 merged a pull request: "bugfix, Change up submitpackage results to return results for all transactions"
(https://github.com/bitcoin/bitcoin/pull/28848)

💬 fanquake commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#issuecomment-1836491942)
Yea, happy for https://github.com/bitcoin/bitcoin/pull/28846/commits/156366f10aa38c612e26a1f93d8503786f3d3a34 to be cherry-picked in here.

🚀 achow101 merged a pull request: "rpc: keep `.cookie` file if it was not generated"
(https://github.com/bitcoin/bitcoin/pull/28784)

💬 achow101 commented on pull request "wallet: Fix migration of blank wallets":
(https://github.com/bitcoin/bitcoin/pull/28976#discussion_r1412388284)
Done

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#issuecomment-1836530146)
rebased on https://github.com/bitcoin/bitcoin/pull/28848 and ready for review

💬 ryanofsky commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#issuecomment-1836557161)
> Yea, happy for [156366f](https://github.com/bitcoin/bitcoin/commit/156366f10aa38c612e26a1f93d8503786f3d3a34) to be cherry-picked in here.

In case you do cherry pick it, I suggested a comment to go along with the code here: https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1399057303. I think it would also be good link to https://github.com/chaincodelabs/libmultiprocess/pull/79 in the commit message or pull request description since that what the triggered the problem.

Also seems
...

💬 romanz commented on pull request "rpc: keep `.cookie` file if it was not generated":
(https://github.com/bitcoin/bitcoin/pull/28784#issuecomment-1836591659)
Thanks :)

💬 mzumsande commented on pull request "validation: log which peer sent us a header":
(https://github.com/bitcoin/bitcoin/pull/27826#discussion_r1412484600)
I like the first commit, but the second/third commit are not ideal, because there is a difference between a new block and a new header:
I started up this branch on mainnet while being a few blocks behind (but not that far such that I'd be in IBD so that the log messages would go to `BCLog::VALIDATION`).
Then, the log entries I'd get for each downloaded block ("Saw new header via unsolicited block") are wrong in two ways:
The header is not new (only the block), plus the blocks is not unsolicit
...

💬 mzumsande commented on pull request "validation: log which peer sent us a header":
(https://github.com/bitcoin/bitcoin/pull/27826#discussion_r1412496288)
I looked at this independently (tracing the call chains before reading this thread), and I came to the same conclusion as Sjors, except for the case of a direct BLOCK, which has an issue that I mentioned below.