💬 maflcko commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2367816284)
> > There is no need to use brew to install clang/llvm. You can just pick the newer XCode: `sudo xcode-select --switch /Applications/Xcode_15.0.app`
>
> That only works if you install Xcode, otherwise you're stuck on whichever version of the command-line tools came with the OS.
>
> You can't install Xcode via the App Store (on macOS 13). You have to download it via the Developer Center. Account required (the free one). You'll need version 15.2, because 15.4 required macOS 14.
>
> So tha
...

💬 fanquake commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2367824247)
> From googling around I get the impression this check isn't reliable, and you have to check for specific features. E.g. https://stackoverflow.com/questions/75296149/cmake-set-project-to-c-20#comment132865328_75296149

That is what CMake does behind the scenes, i.e the tests in https://github.com/Kitware/CMake/tree/master/Tests/CompileFeatures. So if they are missing coverage for a certain compiler/cxx std combo, it's a bug they should address, not something we should have to test ourselves.

💬 Sjors commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2367832023)
@fanquake do you mean this file? https://github.com/Kitware/CMake/blob/master/Tests/CompileFeatures/cxx_std_20.cpp

:lock: fanquake locked an issue: "Minar"
(https://github.com/bitcoin/bitcoin/issues/30944)

:lock: fanquake locked an issue: "bitcoin core发送后，链上没有"
(https://github.com/bitcoin/bitcoin/issues/30943)

💬 fanquake commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2367839709)
I'd start by looking at https://github.com/Kitware/CMake/blob/master/Tests/CompileFeatures/CMakeLists.txt, and the various combinations of compiler (versions) + feature tests.

💬 maflcko commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2367839797)
Individual compile features are not provided for C++ 17 or later. See https://cmake.org/cmake/help/latest/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html#low-level-individual-compile-features

And I don't think they are tested either, because code should be using the feature test macros introduced in recent versions of C++.

💬 theStack commented on pull request "test: add test for specifying custom pidfile via `-pid`":
(https://github.com/bitcoin/bitcoin/pull/30724#discussion_r1771176602)
Good point, done.

💬 fanquake commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2367854771)
It seems like they add cpp feature checks for various compilers if it's otherwise broken/doesn't always report the right version, i.e https://github.com/Kitware/CMake/blob/master/Modules/CMakeCXXCompilerId.cpp.in#L46, maybe they need to do the same for Apple Clang.

💬 theStack commented on pull request "test: add test for specifying custom pidfile via `-pid`":
(https://github.com/bitcoin/bitcoin/pull/30724#issuecomment-2367855172)
@tdb3:
> `"bitcoind.pid"` is also mentioned in `feature_filelock.py`. Could add a commit that deduplicates this.
>
> Created an example commit here: [tdb3@b622390](https://github.com/tdb3/bitcoin/commit/b622390b33cf3813b4f80d13377b248d6e898a4f)

Thanks! I've cherry-picked your commit, but reordered it to appear before the test introduction commit and changed the commit subject accordingly (introduce constant, instead of deduplicating). Let me know if that's okay for you.

@naiyoma:
> T
...

⚠️ maflcko opened an issue: "Test p2p_headers_presync fuzz target on macOS/Windows"
(https://github.com/bitcoin/bitcoin/issues/30950)
The target runs slowly on macOS/Windows, so it would be good if someone benchmarked it. At least the macOS 14 CI ran into timeouts consistently.

I've removed the fuzz inputs temporarily in https://github.com/bitcoin-core/qa-assets/commit/84cea7068728bc2c765b9da680eedcb85f9f3c1b in the meantime.

_Originally posted by @maflcko in https://github.com/bitcoin/bitcoin/issues/30661#issuecomment-2363634967_

🤔 l0rinc reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2320844778)
Concept ACK

I went over the code a few times and tried to document my immediate observations.
I understand if you think some comments are outside the scope of this PR - I'll provide a higher-level review later, once I understand the problem better. I still need a few iterations to be able to zoom out and comment meaningfully, for now I only left nits about implementation specifics, modernization attempts, potential off-by-one errors, readability suggestions, etc.

<details>
<summary>Sugge
...

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1770545259)
```suggestion
Assume(!package_to_validate);
```

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1770544781)
* how come we're only documenting the `first_time_failure` parameter?
* is there any particular reason for this weird formatting?

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1770543742)
Commit messages:

> add TxDownloadOptions bool to make deterministic TxRequestTracker

nit: may be clearer as `add TxDownloadOptions bool to make TxRequestTracker deterministic`

> [refactor] ProcessInvalidTx logic to put peerman tasks at the end

nit2: `[refactor] put peerman tasks in ProcessInvalidTx logic at the end`

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1770549777)
nit: formatting seems off

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1770549633)
We might as well simplify these:
```C++
PackageToValidate(PackageToValidate&&) = default;
PackageToValidate(const PackageToValidate&) = default;
PackageToValidate& operator=(PackageToValidate&&) = default;
```

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1770553366)
Similarly we can avoid two lookups here:
```suggestion
auto it = m_peer_info.find(peer);
if (it == m_peer_info.end()) return false;
const auto& info = it->second.m_connection_info;
```

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1770550296)
can we log warn/error if it does?
Or even better, I assume this is because of testing, can we rather move the test to the testing and assert here instead?

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1770546568)
We could reserve the vector to the expected size to avoid resizing:
```C++
tx_invs.reserve(std::ranges::count_if(vInv, &CInv::IsGenTxMsg));
for (CInv &inv : vInv) {
```