💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949998393)
should call `reserve` before pushing entries to `peer_it_heap`, not after.

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949955925)
should this be `int64_t` instead of `int` in the spirit of the first commit?

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1950002603)
Could `m_orphans.size() <= max_orphans` be inside `NeedsTrim`?

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949856455)
nit: doesn't really matter if unsigned int (return value) or int is used, but would be nice to make it consistent, also with the %u / %d format specifiers in the logprints.

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949899180)
nit: commit msg of c929d71c0828544be509934312b6a7d11b47ea4d lacks a verb (e.g. "split").

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2651900348)
re: https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2546088852

> I found another segmentation fault in updating my previous ACK to the latest tip of this PR. Here are minimal steps to reproduce

This seems closely related to the _broken connection during node shutdown causing unclean shutdown_ issue reported earlier so added a note about this there: https://github.com/chaincodelabs/libmultiprocess/issues/123#issuecomment-2651851492

💬 maflcko commented on pull request "fuzz: Use serial task runner to increase fuzz stability":
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2651908196)
> On alternative would be to inline the scheduler in tests (make it synchronous), but the patch would be larger.

Looks like this will lead to deadlocks due to lock order inversions between cs_main and cs_wallet in the unit tests. I'll try to apply it to the fuzz tests only. If there are still issues in this pull, or in the future, they could be worked around for the affected fuzz target.

💬 Prabhat1308 commented on pull request "test: clarify timewarp grace period griefing attack":
(https://github.com/bitcoin/bitcoin/pull/31725#discussion_r1951495427)
this is introducing a more stricter equality. Is it because we now have the liberty to choose `future` as we wish whereas previously it was `t + MAX_FUTURE_BLOCK_TIME` which not necessarily be equal to template's curtime ?

💬 Prabhat1308 commented on pull request "test: clarify timewarp grace period griefing attack":
(https://github.com/bitcoin/bitcoin/pull/31725#discussion_r1951488861)
The log mentions `t` to be strictly less than `MAX_FUTURE_BLOCK_TIME`. But in line 165 , the assert statement is for `future<=MAX_FUTURE_BLOCK_TIME`.

💬 Prabhat1308 commented on pull request "test: clarify timewarp grace period griefing attack":
(https://github.com/bitcoin/bitcoin/pull/31725#discussion_r1951491551)
on previous line 199 , `mtp` is set to `node.getblock(node.getbestblockhash())["mediantime"] + 1` . from the rpc commands , shouldnt `mtp` be just `node.getblock(node.getbestblockhash())["mediantime"]` ? and then `nTime` is correct with `mtp + 1`. Have checked it locally and the test passes.

🤔 Prabhat1308 reviewed a pull request: "test: clarify timewarp grace period griefing attack"
(https://github.com/bitcoin/bitcoin/pull/31725#pullrequestreview-2609845672)
ACK [`6065b78`](https://github.com/bitcoin/bitcoin/pull/31725/commits/6065b78e22e47f053227f68afb8e3afa7bf0ab95)

💬 hodlinator commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2651918799)
Just reproduced the same failure using binaries produced by CI cross-compilation setup from #31176 as I had before using the [Guix](https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2604068753) cross-compiled build. Good to see that the two have the same behavior... but worrying that nobody else is getting this.
```
$ BITCOINCLI=/c/Users/hodlinator/Downloads/18274b6-bins/bitcoin-cli.exe BITCOIND=/c/Users/hodlinator/Downloads/18274b6-bins/bitcoind.exe build/test/functional/wallet_
...

🤔 Prabhat1308 reviewed a pull request: "test: check `scanning` field from `getwalletinfo`"
(https://github.com/bitcoin/bitcoin/pull/31768#pullrequestreview-2609887988)
re-ACK [`bb0879d`](https://github.com/bitcoin/bitcoin/pull/31768/commits/bb0879ddabc8b3a7253bc774d23b842937d18015)

💬 theuni commented on pull request "cmake: Install man pages for configured targets only":
(https://github.com/bitcoin/bitcoin/pull/31765#issuecomment-2651979630)
> Considering that in order to hook up `COMPONENTS` usage, we'd need to add an `install()` line for each as opposed to our current `installable_targets` approach, I'm not sure that `install_manpage` is the right abstraction.
>
> Instead, I think we're going to want an `add_install()` or so, right?

See [this branch](https://github.com/theuni/bitcoin/commits/cmake-install-components/) for an alternative which adds `install_binary_component` as described above. Imo it's more thorough/correct.
...

💬 theStack commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2651994364)
> > I wonder if it wouldn't be easier to just go with clang/llvm
>
> I've tested the suggested diff and it worked for me. llvm/clang is my default anyway so I would give this a Concept ACK if you want to follow this approach @theStack

Concept ACK on switching over to clang/llvm as well considering all the reported problems. Note though that this PR merely tries to restore the previous behaviour of the script for CMake builds (and minor improvements that are enabled by that), so I think a d
...

💬 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-2652004183)
Sure, but it may take some time until I get around to it.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1951563218)
Fixed.

💬 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-2652009268)
In any case, I am sure this script is broken or at least stale for years and shouldn't be a blocker for 29.0 (even if it was used, it is a dev-only tool). So I'll be removing the milestone for now.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1951565256)
Creating a reference doesn't cost anything (in simple cases); it's just introducing a new name for an existing object. It's true that it's kind of pointless at this stage, but it'll be used in "group per-graph data in ClusterSet", and in "add staging support".

📝 theuni opened a pull request: "cmake: add a component for each binary"
(https://github.com/bitcoin/bitcoin/pull/31844)
This makes it possible to build/install only the desired binaries regardless of the configuration.
For consistency, the component names match the binary names. `Kernel` and `GUI` have been renamed.

Additionally it fixes #31762 by installing only the manpages for the configured targets (and includes them in the component installs for each).

Alternative to #31765 which is (imo) more correct/thorough.

Can be tested using (for ex):
```bash
$ cmake -B build
$ cmake --build build -t bitco
...