💬 fjahr commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1954438224)
Concept ACK, looks good at first sight, will do a deeper review when it's clear if the CI failure is an issue. I couldn't reproduce it locally so far...

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1954448961)
To reproduce locally, you'll have to compile with the `integer` sanitizer (not to be confused by the `undefined` sanitizer)

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1954455499)
CI failure is just a preexisting bug in a line of code that doesn't affect anything:

https://github.com/bitcoin/bitcoin/blob/45b2a91897ca8f4a3e0c1adcfb30cf570971da4e/src/rpc/blockchain.cpp#L1658

Two `nChainTx` values in the `getchaintxstats` are subtracted and this triggers the undefined behavior sanitizer because they are unsigned and the result is negative. `getchaintxstats` has a number of edge case problems like this that should be cleaned up but don't really affect anything. I'll prob
...

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1954457409)
> To reproduce locally, you'll have to compile with the `integer` sanitizer (not to be confused by the `undefined` sanitizer)

Thanks! I was just trying to reproduce this locally using the undefined sanitizer.

💬 fanquake 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-1954461873)
This is `MAC_OSX` because that is what we actually mean, and using our own define gives us more control. We produce release binaries for macOS running on darwin hardware, not just any "`__APPLE__` target", which, for example, would include iOS running on darwin hardware (i.e `arm64e-apple-ios`).

Swapping to just use `__APPLE__` would mean it's no-longer as easy to differentiate the two. At this point, that may no-longer matter, but there is code in this codebase that is relevant for macOS, bu
...

✅ fanquake closed a pull request: "refactor: improve readability of numeric literals in consensus parameters and network settings"
(https://github.com/bitcoin/bitcoin/pull/29444)

💬 fanquake commented on pull request "refactor: improve readability of numeric literals in consensus parameters and network settings":
(https://github.com/bitcoin/bitcoin/pull/29444#issuecomment-1954471769)
Thanks, however we don't generally refactor consensus code for the sake of readability, so we'll leave this as-is for now.

✅ fanquake closed a pull request: "Change Luke Dashjr seed to dashjr-list-of-p2p-nodes-maybe-malware.us"
(https://github.com/bitcoin/bitcoin/pull/29145)

💬 fanquake commented on pull request "Change Luke Dashjr seed to dashjr-list-of-p2p-nodes-maybe-malware.us":
(https://github.com/bitcoin/bitcoin/pull/29145#issuecomment-1954477676)
Given it's been more than a month, and there's no more followup here, I'm going to close this for now. Feel free to ping for a re-open, if/when you've picked a more suitable domain name.

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