💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1919085877)
Added in #31666

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1919085955)
Added in #31666

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1919086045)
Added in #31666

💬 hebasto commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2596652055)
The CI failure is [unrelated](https://github.com/bitcoin/bitcoin/pull/31675).

💬 hebasto commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1919101037)
This is no longer relevant for the recent [push](https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2596595610).

🤔 pablomartin4btc reviewed a pull request: "gui, psbt: Use SIGHASH_DEFAULT when signing PSBTs"
(https://github.com/bitcoin-core/gui/pull/850#pullrequestreview-2557125142)
utACK 3e97ff9c5eaa3160426ba112930b047404c54c9e

I agree with @Sjors but tha'ts not covered by doc at the `/** Signature hash types/flags */` `enum` definition (`src/script/interpreter.h`)?

> https://github.com/bitcoin/bitcoin/commit/e94bb4e5fc6dea67844a8495b948b679649b7b65 makes this problem go away though.

is that part of a PR?

💬 maflcko commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1919104106)
I guess it is fine to ignore any "hidden" files (starting with `.`) as an alternative.

💬 hebasto commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1919104517)
In the recent [push](https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2596595610), this branch relies on setting the `base_dir` option by the user because the required value depends on actual user's build environment.

💬 hebasto commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1919106437)
This is no longer relevant for the recent [push](https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2596595610).

💬 mzumsande commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1919108948)
Yes, taking a random peer would be ok.
Though I kinda like the idea of calling `ProcessOrphanTx()` in `FinalizeNode()` in general, even if just 1 peer is involved - this is scheduled work that is assigned to but doesn't need any input from the peer, so there is no reason not do it just because the peer decides to disconnect at the wrong time.

💬 hebasto commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1919109640)
[Reworked](https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2596595610) to work equally well across both Git's wortktrees and build trees.

💬 hebasto commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1919110665)
This is no longer relevant for the recent [push](https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2596595610).

💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#issuecomment-2596757762)
re-ACK 0f716f28896c6edfcd4e2a2b25c88f478a029c7b

💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1919132992)
Okay, I've checked CMake docs again.

`CMAKE_REQUIRED_LINK_OPTIONS` is forwarded to `target_link_options()`, which is [designed](https://cmake.org/cmake/help/latest/command/target_link_options.html) "to add any link options".

I'll update the PR shortly.

📝 darosior opened a pull request: "fuzz: add targets for PCP and NAT-PMP port mapping requests"
(https://github.com/bitcoin/bitcoin/pull/31676)
Based on https://github.com/bitcoin/bitcoin/pull/31022, this introduces a fuzz target for `PCPRequestPortMap` and `NATPMPRequestPortMap`.

Like in #31022 we set `CreateSock` to return a `Sock` which mocks the responses from the server and uses a mocked steady clock for the `Wait`s. Except here we simply respond with fuzzer-provided data until the client stop sending requests. We also sometimes inject errors and connection failures based on fuzzer-provided data.

I ran both targets overnight
...

💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1919145857)
> I meant as `LINK_OPTIONS`. I'd rather have LDFLAGS forward to that unless there's a reason not to.

In that case a static library (e.g., `/usr/lib/libFuzzingEngine.a`) would be placed _before_ object files being linked, and the linker might fail.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2596810683)
> Can you update the PR description to have a list of pre-requisite PRs?

There's a tracking issue with everything listed #31246

👍 ryanofsky approved a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2556956641)
Code review ACK e9edf28ba4ff4ebf02ae90874794c51c55ef789e. Nice changes! Again I think using consistent build locations that match install locations should make it is easier to see what binaries are available, simplify scripts and wrappers, and avoid bugs that only happen in developer builds but not final installs or vice-versa.

Since last review the main change seems to be that subproject binaries are not affected anymore by this PR, which is great.

I don't understand the details of the di
...

💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919027627)
In commit "cmake: Set top-level target output locations" (9691848c29a772b5a8f0fc68855f8b2ba3c61921)

It wasn't really clear to me what how this comment "The top-level target output locations for subprojects remain unmodified" is related to code it is commenting on.

Would maybe suggest changing to something more explanatory like "These need to be included before CMAKE_*_OUTPUT_DIRECTORY variables are set, so output locations of subproject tests and libraries are not overridden."

💬 ryanofsky commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919075538)
In commit "cmake: Set top-level target output locations" (9691848c29a772b5a8f0fc68855f8b2ba3c61921)

I was confused why univalue seemed to be handled differently than the other subprojects above.

Might be helpful to add a comment like "# The univalue subproject is different from the other subprojects because we actually use the CMakeLists.txt in the project subdirectory instead of defining separate cmake targets, so use add_subdirectory() instead of include() to include it."