💬 ryanofsky commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1204047087)
re: https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1203035210

> Returning the error in an optional feels a bit wrong since it inverts the typical pattern. Rather than checking that the result is equivalent to `true`, we have to check that it's equivalent to `false`.

Returning `optional<bilingual_str>` or `Result<bool>` both seem ok here, but the ideal thing would be to return `Result<void>` so there are just 2 cases for calling code to handle 1-success, and 2-failure with an err
...

💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1561078786)
It's not a virtual machine. Logs are combined by default, right?

```
$ cat /tmp/bitcoin_func_test_zkozpx3h/test_framework.log | nc termbin.com 9999
```

https://termbin.com/c3yr

💬 fanquake commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#discussion_r1204069957)
Switched back to Jammy in next push.

📝 MarcoFalke opened a pull request: "ci: Add missing set -e to 01_base_install.sh"
(https://github.com/bitcoin/bitcoin/pull/27739)
Otherwise errors are silently ignored

💬 MarcoFalke commented on pull request "ci: Add missing set -e to 01_base_install.sh":
(https://github.com/bitcoin/bitcoin/pull/27739#issuecomment-1561108254)
Looks like it is working: https://cirrus-ci.com/task/4766589006905344?logs=build#L2788


Added another unrelated commit to re-trigger CI and to give more yummy review to reviewers.

💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1561118892)
Ah, this is more useful:

```
==2894643== Thread 25 b-msghand:
==2894643== Conditional jump or move depends on uninitialised value(s)
==2894643== at 0x24A6EC: (anonymous namespace)::PeerManagerImpl::RemoveBlockRequest(uint256 const&, std::optional<long>) [clone .isra.0] (in /home/sjors/dev/bitcoin/src/bitcoind)
==2894643== by 0x25AAA1: (anonymous namespace)::PeerManagerImpl::ProcessBlock(CNode&, std::shared_ptr<CBlock const> const&, bool, bool) (in /home/sjors/dev/bitcoin/src/bitcoin
...

💬 MarcoFalke commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1561137418)
Maybe open a separate issue? (There are already too many comments in this thread).

Make sure to include details, otherwise it will be harder to do something. Note that not all compiler versions are compatible with all valgrind versions.

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

👋 ryanofsky's pull request is ready for review: "refactor: Replace `std::optional<bilingual_str>` with `util::Result`"
(https://github.com/bitcoin/bitcoin/pull/25977)

💬 instagibbs commented on issue "Notes on Block-In-Flight Handling":
(https://github.com/bitcoin/bitcoin/issues/16172#issuecomment-1561195606)
As of https://github.com/bitcoin/bitcoin/pull/27626 this case should be covered, you can close

💬 ismaelsadeeq commented on issue "25.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/27736#issuecomment-1561196420)
Thank you @theStack for pointing this out, these are important features, I will add them to the guide.

✅ fanquake closed an issue: "Notes on Block-In-Flight Handling"
(https://github.com/bitcoin/bitcoin/issues/16172)

💬 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` ... ?)