💬 achow101 commented on issue "Wallet symlinks are not rejected as expected":
(https://github.com/bitcoin/bitcoin/issues/31842#issuecomment-2651774811)
Symlink to the wallet directory is allowed, but a symlink to a wallet file is not. What you've tested manually is a symlink to a wallet directory, so it works.

💬 0xB10C commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2651790997)
here a some per task stats on this:

e.g. for the `fuzzer,address,undefined,integer, no depends` task


scheduling + execution:
https://0xb10c.github.io/bitcoin-core-ci-stats/graph/schedule-and-execution/


scheduling only:
https://0xb10c.github.io/bitcoin-core-ci-stats/graph/schedule/

🚀 achow101 merged a pull request: "test: Add mockable steady clock, tests for PCP and NATPMP implementations"
(https://github.com/bitcoin/bitcoin/pull/31022)

🤔 mzumsande reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2607117435)
Halfway through, some minor thing below - my main conceptual question is why `m_total_announcements` is a meaningful metric in limiting the orphanage.

My understanding is that `m_total_orphan_usage` exists to limit memory usage, and `m_total_announcements` to limit CPU usage - but why the number of announcements instead of number of orphans?
Why would it make the situation any less DoSy if we remove an announcer but keep the orphan? Since we only assign the tx to one peer's workset after 74
...

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