💬 maflcko commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1496051696)
a74c3972bf8d0fa51e8f9672422bc34945b66ca7: It would be easier to review if style changes were separate from move-only changes? Otherwise, options such as `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space` can not be used.

Also, renaming tokens in a block of code will break the parity check used to find the introduction of the code block (https://git-scm.com/docs/git-log#Documentation/git-log.txt--Sltstringgt). So it may be easier to track the history, if the renames were done in a
...

💬 paplorinc commented on pull request "build: replace custom `MAC_OSX` macro with standard `__APPLE__` for consistent macOS detection":
(https://github.com/bitcoin/bitcoin/pull/29450#issuecomment-1954527134)
Hey @fanquake, thanks for your input.
How do you suggest we unify this in the codebase, since it's not used consistently currently?
This change could level the field before branching off to properly differentiate between the cases.

💬 paplorinc commented on pull request "refactor: improve readability of numeric literals in consensus parameters and network settings":
(https://github.com/bitcoin/bitcoin/pull/29444#issuecomment-1954535533)
Hey @fanquake, thanks for your review.
Can you tell me when you'd expect consensus code to be changed otherwise?

💬 maflcko commented on pull request "refactor: improve readability of numeric literals in consensus parameters and network settings":
(https://github.com/bitcoin/bitcoin/pull/29444#issuecomment-1954566217)
Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:

* Time spent on review
* Accidental introduction of bugs
* (Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer time or introduce bugs.

For more in
...

💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1496137942)
Done as suggested.

💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1496138057)
Done as suggested.

💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1496138206)
Done

💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1496138292)
Done

💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1496138577)
Changed to `std::numeric_limits<uint32_t>::max()`

💬 sr-gi commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1496142945)
I did a bit more digging and found the \_actual\_ origin of the DNS result randomization, a code refactor before the commit I posted before got me confused: https://github.com/bitcoin/bitcoin/commit/1a5a4e648873c4cd88b936648ebf2858393e5510

This was introduced in https://github.com/bitcoin/bitcoin/pull/8113 by @sipa, so he may know better

💬 paplorinc commented on pull request "refactor: improve readability of numeric literals in consensus parameters and network settings":
(https://github.com/bitcoin/bitcoin/pull/29444#issuecomment-1954628565)
Thanks @maflcko, understood, should we keep https://github.com/bitcoin/bitcoin/pull/29444/commits/9d89358f34416c97bbe6ce4b63b7eaac2c714c54?

👋 paplorinc's pull request is ready for review: "optimization: Speed up TryParseHex by 300%"
(https://github.com/bitcoin/bitcoin/pull/29458)

💬 theuni commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1954651604)
Rebased. We'll see what c-i thinks now. Benchmarks should be all good.

I think it makes sense for #29408 to go first though.

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1496191920)
took in a modified form, left the direct exposure of `tail_feerate` as a YNGNI extension

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1496192018)
updated to be more explicit on what those values represent

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1496192101)
changed to standard lingo

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1496192220)
taken with some liberties

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1496192331)
replaced, even though I thought it sounded pretty smart

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1496192417)
@sdaftuar going to hold off on this for now if that's ok

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1496193191)
taken with only light modifications, thanks!