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

📝 glozow opened a pull request: "[NO MERGE] BIP331 Ancestor Package Relay"
(https://github.com/bitcoin/bitcoin/pull/27742)
**WORK IN PROGRESS.** Please do not run it for any use other than testing.

This PR is not meant for merge. This branch exists to help reviewers see what package relay would look like all together. I will open separate PRs for these components and expect to add more tests, docs, and polish along the way. This PR contains all of the functionality built in a linear manner. However, since there are pieces in various areas of the codebase and they can make progress in parallel, commits don't nec
...

💬 stickies-v commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204062504)
nit: I think you also need to `#include <util/translation.h>` for `bilingual_str`

🤔 stickies-v reviewed a pull request: "kernel: Remove util/system from kernel library, interface_ui from validation."
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1441753577)
Code Review ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e

I'm still familiarising myself with the libbitcoinkernel project so for now I've mostly focused on the code being correct, readable, ... and not so much on architecture (which is arguably the most important thing here) etc. All the changes made sense, and are very well structured and readable.

I've left a few nits (+[here](https://github.com/TheCharlatan/bitcoin/commit/7d3b35004b039f2bd606bb46a540de7babdbc41e#commitcomment-114818127)
...

💬 stickies-v commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204065433)
nit: Do we need this?

💬 stickies-v commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204082373)
nit: could use `warn()` to avoid having an identical fn and param name (also more intuitive for a fn name imo, but that's personal)

💬 stickies-v commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204148265)
nit: I think this include is no longer necessary

💬 stickies-v commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204278405)
nit: I think adding line number is unmaintainable, would suggest leaving it out (+ a few lines down). Even the filename is probably not even necessary, but less of a problem.

💬 achow101 commented on pull request "Deniability - a tool to automatically improve coin ownership privacy":
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1561460052)
It seems like a feature like this would be better to integrate into the wallet directly rather than something that is done through and managed by the GUI.