✅ dergoegge closed a pull request: "bench: Remove various extraneous benchmarks"
(https://github.com/bitcoin/bitcoin/pull/31153)
(https://github.com/bitcoin/bitcoin/pull/31153)
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2596901446)
Added an additional inspector function:
* `CountDistinctClusters`, which efficiently counts how many distinct clusters a list of specified Refs belong to, for helping with enforcing RBF cluster policy limits.
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2596901446)
Added an additional inspector function:
* `CountDistinctClusters`, which efficiently counts how many distinct clusters a list of specified Refs belong to, for helping with enforcing RBF cluster policy limits.
👍 danielabrozzoni approved a pull request: "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2557317115)
tACK eb325bc0efd3f999189e155ba836be92e6cc9af7 - I reviewed the code and manually verified the behavior of the affected RPC arguments, both old and new.
Thank you for splitting the changes into several separate commits; it made the review process much easier! :)
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2557317115)
tACK eb325bc0efd3f999189e155ba836be92e6cc9af7 - I reviewed the code and manually verified the behavior of the affected RPC arguments, both old and new.
Thank you for splitting the changes into several separate commits; it made the review process much easier! :)
💬 theuni commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1919214193)
Done.
(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.
(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.
(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).
(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).
(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).
(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.
(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.
</
...
(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.
(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
(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
(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
(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.
(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)
(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.
(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
(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.
(https://github.com/bitcoin/bitcoin/pull/30951#pullrequestreview-2557320101)
No opinion on concept. Just code review.