💬 mzumsande commented on issue "Parallel compact block download":
(https://github.com/bitcoin/bitcoin/issues/25258#issuecomment-1561244418)
should this stay open for a while as per https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1557224231?

💬 instagibbs commented on issue "Parallel compact block download":
(https://github.com/bitcoin/bitcoin/issues/25258#issuecomment-1561246246)
We could open a new issue with fresh context as well, summarizing the discussion to this point?

💬 fanquake commented on issue "Parallel compact block download":
(https://github.com/bitcoin/bitcoin/issues/25258#issuecomment-1561246426)
> should this stay open

The PR shouldn't stay open, but we can open a tracking issue if needed.

💬 fanquake commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1561260881)
> Do you happen to know what the difference to the debian package is? Maybe it can be fixed upstream instead?

I'm not completely sure, the answer will be in here somewhere: https://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/blob/snapshot/debian/rules (also taking into account [all of the patches](https://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/tree/snapshot/debian/patches) they apply). They should be doing a multi stage build, and building everything with the 2nd-built Clang, but
...

💬 MarcoFalke commented on pull request "refactor: Replace `std::optional<bilingual_str>` with `util::Result`":
(https://github.com/bitcoin/bitcoin/pull/25977#issuecomment-1561270092)
Looks like there is a clang bug. Could be fixed by either bumping it again, see https://github.com/bitcoin/bitcoin/pull/27682, but I am not sure how to do that for macOS. Or by replacing `return addrman;` with `return {std::move(addrman)};`, or something like that (temporarily).

⚠️ instagibbs opened an issue: "Parallel compact blocks bandwidth reduction or improvements"
(https://github.com/bitcoin/bitcoin/issues/27740)
Opening a tracking issue post https://github.com/bitcoin/bitcoin/issues/25258

To summarize a few potential strategies:

1) It may be beneficial to reduce/eliminate concurrent block fetches from high-bandwidth outbound peers. That's at tension with the potential block relay speedup for non-listening peers, but at least non-listening peers aren't as vulnerable to sybil nodes.
2) Could reduce number of concurrent to 2, instead of 3, for non-listening only, or both modes, to reduce worst-case
...

💬 instagibbs commented on issue "Parallel compact block download":
(https://github.com/bitcoin/bitcoin/issues/25258#issuecomment-1561271505)
opened a new issue: https://github.com/bitcoin/bitcoin/issues/27740

💬 pinheadmz commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1204269104)
Good point, although "if empty, return now" logic would make sense after each step in this function (why bother calling `EraseLastKElements()` with an empty array?) even without this PR. One thing we could do is add that logic to the beginning of `EraseLastKElements()` itself, but I dunno how much time is really wasted in there (`sort`, `min`, `erase` ... ?)

💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204276234)
This is the line valgrind trips over. Adding something like `fprintf(stderr, "from_peer=%ld\n", *from_peer);` above it makes it stop complaining. Maybe this is just a valgrind a bug?

💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1203951678)
cce96182ba2457335868c65dc16b081c3dee32ee: maybe do `if (!mapBlocksInFlight.count(hash) {` first and then construct the range after the `Assume` below?

💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1203997112)
Lost in rebase?

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204280315)
hmmm appears so

💬 MarcoFalke commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#discussion_r1204288987)
Some ideas (feel free to ignore):

* `-DLLVM_USE_LINKER=lld` and/or ` -DLLVM_PARALLEL_LINK_JOBS=1` to reduce change of OOM?
* `-DLLVM_INCLUDE_BENCHMARKS=OFF -DLLVM_INCLUDE_TESTS=OFF` to reduce CPU?

👍 MarcoFalke approved a pull request: "ci: compile Clang and compiler-rt in msan jobs"
(https://github.com/bitcoin/bitcoin/pull/27737#pullrequestreview-1442083675)
lgtm. Longer term it may be better to try to go back to using the pre-compiled one from debian, or alternatively try a different Linux distro for the msan tasks?

💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1204313559)
I had similar considerations to e.g. wrap the `EraseLastKElements` call in a lambda fn, but it's probably not worth the LoC change.

I think this case is slightly different in that it better highlights that if there are no inbound NoBan peers, the result is always `std::nullopt`.

💬 pinheadmz commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1204318480)
good point, i'll add it. one line i think in this case will improve readability

💬 ArmchairCryptologist commented on issue "Frequent "Timeout downloading block" with 24.1":
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1561343514)
> Can you block this peer?

I did block the peer right after I posted the latest logs four days ago, and after that the block timeouts dropped to 2-3 per day, but like I mentioned in the bug report, since Core doesn't normally log the IP of disconnected incoming peers you pretty much have to enable debug=net logging to find the IP of the misbehaving peer. Which on this particular node is ~1GB of logs per 2-3 hours.

⚠️ Sjors opened an issue: "Spurious (?) valgrind failure for p2p_compactblocks.py"
(https://github.com/bitcoin/bitcoin/issues/27741)
Ubuntu 23.04 with g++ 12.2.0 and valgrind 3.19.0. Built without depends and without `--enable-debug`.

The failure is introduced by:
https://github.com/bitcoin/bitcoin/pull/27626/commits/cce96182ba2457335868c65dc16b081c3dee32ee

Offending line in `RemoveBlockRequest`:

```cpp
if (from_peer && *from_peer != node_id) {
```

It seems to believe `from_peer` is uninitialized. Adding a log statement that prints `from_peer` makes the error go away. As does compiling with `--enable-debug`. Ot
...

💬 fanquake commented on issue "Spurious (?) valgrind failure for p2p_compactblocks.py":
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1561375155)
Can you make it clear what version of the code you are actually running valgrind on? Are you compiling cce96182ba2457335868c65dc16b081c3dee32ee or are you compiling master (if so, which commit).

💬 theuni commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1561389825)
Concept ACK. I'm not so familiar with the history here, but the changes seem sane to me.

The libc++ changes here are especially interesting. I assume they work with no issues because there's no SDK to take into consideration, but (as @fanquake suggested in an offline discussion yesterday) I'm going to see if we can maybe port this over to our depends builds with the SDK included. Unsure if it'll work 🤷 but it would be much cleaner if it did.