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

🚀 achow101 merged a pull request: "rpc: Fix invalid bech32 address handling"
(https://github.com/bitcoin/bitcoin/pull/27727)

💬 achow101 commented on pull request "25.0 Final Changes":
(https://github.com/bitcoin/bitcoin/pull/27686#issuecomment-1561508601)
ACK ac8d72668c7bbd9f8771442fe25fd37f08c3f5ae

💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204429122)
cce96182ba2457335868c65dc16b081c3dee32ee: the goal of the above loop seems to be to find one entry: the `vBlocksInFlight` entry for this particular block and `from_peer`. A followup could move that into its own function, and/or have the subsequent operations on the entry live _outside_ the loop.

💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204472811)
It ended up in 03423f8bd12b95a06a4a9d8377e781625dd38aae

💬 achow101 commented on pull request "test: Disable legacy wallet for mempool_packages.py":
(https://github.com/bitcoin/bitcoin/pull/27735#issuecomment-1561578795)
Is there a reason that this test needs the wallet in the first place?

💬 fanquake commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#discussion_r1204513106)
> -DLLVM_INCLUDE_BENCHMARKS=OFF -DLLVM_INCLUDE_TESTS=OFF to reduce CPU?

These two control the generation of build targets, however nothing should be getting compiled, as `LLVM_BUILD_TESTS` and `LLVM_BUILD_BENCHMARKS` [both default to `OFF`](https://llvm.org/docs/CMake.html). However I've also added some additional `-DLLVM_INCLUDE_*` options that should reduce compilation.

> -DLLVM_USE_LINKER=lld and/or -DLLVM_PARALLEL_LINK_JOBS=1 to reduce change of OOM?

I could follow up with a change
...

💬 achow101 commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1204518144)
AFAIK there aren't any other external signer software yet.