💬 theuni commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1919214193)
Done.

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1919215050)
Do we care about keeping the checksum calculation compatible? For almost all users, this PR being merged will mean a new asmap database anyway, so they'll incur the price of addrman invalidation that brings anyway.

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2596961825)
> Code review ACK [e9edf28](https://github.com/bitcoin/bitcoin/commit/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.

@ryanofsky

Thank you for your review! The branch has been updated per your feedback.

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919269581)
Thanks! [Fixed](https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2596961825).

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919271417)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2596961825).

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1919272736)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2596961825).

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1919285177)
I like that idea too. But what if there is a large amount of orphans left? There shouldn't be a ton, but I don't know how to feel about a peer being granted more than the normal amount of compute because they are going to be disconnected.

💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1919306217)
> The last push wasn't correct. This will fail

Thanks for catching, I did this because without quotes the linter complains about SC2086: https://www.shellcheck.net/wiki/SC2086 and I didn't test any images built with the default `CI_IMAGE_PLATFORM` value.

I am trying to use the native platform by default now in the latest push

Don't fully grasp the context of #28138 but `native_nowallet_libbitcoinkernel` <details>
<summary>
worked on my fedora 40 machine even when this line fails.
</
...

💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1919306517)
Thanks, fixed with a different approach.

💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1919308734)
typo in this comment, I'll fix if I retouch

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#issuecomment-2597023473)
Followup #31666 wraps up all the comments here except for 2 things which might be a little more involved:

https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1902153685 (because we need to add a txid index)
https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1919108948

💬 tcharding commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#issuecomment-2597024315)
cc @dangould

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1919312846)
Right. Not changing for now because it may be useful to recurse in-orphanage ancestors in this function.

👋 glozow's pull request is ready for review: "multi-peer orphan resolution followups"
(https://github.com/bitcoin/bitcoin/pull/31666)

💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#issuecomment-2597025353)
Rebased to incorporate feedback, now try to set a good default `--platform linux/$arch` which should be an improvement for running CI on macOS.

This solution also avoids an issue I ran into after making the image tags less specific, which is that if no `--platform` argument is set, docker will try to use any image that it has cached before fetching a native image.

💬 luke-jr commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r1919212142)
nit: indentation

🤔 luke-jr requested changes to a pull request: "net: option to disallow v1 connection on ipv4 and ipv6 peers"
(https://github.com/bitcoin/bitcoin/pull/30951#pullrequestreview-2557320101)
No opinion on concept. Just code review.

💬 luke-jr commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r1919211882)
Rather avoid the temporary global and just check this when setting `connOptions`

💬 luke-jr commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r1919254645)
The method doesn't change anything, it just returns a value...

💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1919320878)
Marking this as resolved since my latest push tries to set the right native "--platform=linux/{arch}", falling back to "--platform=linux" which is also valid according to docker's documentation although I haven't tested this yet.