💬 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) {
```

💬 l0rinc commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1770548378)
same, I think we can preallocate these as well:
```C++
vGetData.reserve(vToDownload.size());
for (const CBlockIndex *pindex : vToDownload) {
```
followed by
```C++
const auto& tx_requests = m_txdownloadman.GetRequestsToSend(pto->GetId(), current_time);
vGetData.reserve(vGetData.size() + tx_requests.size());
for (const GenTxid& gtxid : tx_requests) {
```

💬 Sjors commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2367873049)
> Dropped the first refactoring commit

Do you think that was the cause of the indeterminism, or are you still investigating that?

💬 Sjors commented on pull request "doc: macOS 15 ships llvm 16":
(https://github.com/bitcoin/bitcoin/pull/30949#issuecomment-2367877024)
Just tested that after manually installing Xcode 15.2 on macOS 13.7 (the last supported version on the most recent 27" iMac), picking it with `xcode-select`, I can still build the project. No need to install llvm via Homebrew.

That's the most recent version you can use. As @maflcko pointed out, Xcode 15.0 should work too, since the Github CI macOS 14 jobs picks it.

💬 ajtowns commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2367878874)
> an attacker cannot have an unbounded number of low-latency connections.

You only need one low-latency connection to run a pretty effective tx spamming attack, as low-latency means that the INV/GETDATA/TX cycle is fast, and you're not rate-limited by the maximum number of pending GETDATA requests. (And even if you don't have a low-latency connection, you can open many connections to get a multiple of that rate-limit)

💬 andrewtoth commented on pull request "sync: improve CCoinsViewCache ReallocateCache - 2nd try":
(https://github.com/bitcoin/bitcoin/pull/30370#issuecomment-2367890416)
In light of this, I think a better approach is to track the available memory in the freelist in the PoolAllocator and expose it in a public getter. Then, discount that available memory in `CCoinsViewCache::DynamicMemoryUsage`. And finally, revert 5e4ac5abf54f8e6d6330df0c73119aa0cca4c103 and achieve the speedup.

🤔 furszy reviewed a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2321884746)
ACK f20fe33