💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-1502863992)
Thanks for the helpful comments @willcl-ark, @martinus and @kouloumos! I plan to address these and update this PR with your suggestions. Currently, I'm prioritizing other projects during the feature freeze, so it might take a few more weeks.

💬 MarcoFalke commented on pull request "test: build Valgrind (3.20) from source & use Clang 16":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502875175)
You removed the suppression for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65434 without explanation? (Or with the explanation that it is an unused GUI supppression?)

💬 fanquake commented on pull request "test: build Valgrind (3.20) from source & use Clang 16":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502888918)
> You removed the suppression for

I removed that suppression as I can't seem to recreate the issue. I assume it's been "fixed" due to using the newer libstdc++ versions shipped with the newer Ubuntu. Note that this was the case with clang-16 & the ubuntu shipped valgrind 3.19 as well.

The GUI suppression was removed because it's unused, and any versions would be out of date when changing Ubuntu versions.

💬 0xB10C commented on pull request "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings":
(https://github.com/bitcoin/bitcoin/pull/27401#issuecomment-1502895511)
Concept ACK and thank you for looking into this!

I would include this in https://github.com/bitcoin/bitcoin/pull/26593 (e.g. with https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1155488768) too. If https://github.com/bitcoin/bitcoin/issues/26916 is blocking something (e.g. switching to a newer clang) I think merging this PR here first should be preferred as https://github.com/bitcoin/bitcoin/pull/26593 is a bigger change that might take a bit longer.

💬 MarcoFalke commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1502901268)
Maybe it reproduces faster with `rr`?

💬 MarcoFalke commented on pull request "test: build Valgrind (3.20) from source & use Clang 16":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502941860)
In any case, it would be good to provide a reason for the bump. Using non-LTS devel OS releases in CI should be an exception reserved for bugfixes or important features, not the norm.

No objection if you want to use bookworm, but for lunar there should be a rationale.

💬 real-or-random commented on pull request "Update src/secp256k1 subtree to release v0.3.1":
(https://github.com/bitcoin/bitcoin/pull/27445#issuecomment-1502947484)
Build failure on CI here. This is during `make distdir`.

```
(cd secp256k1 && make top_distdir=../../bitcoin-i686-pc-linux-gnu distdir=../../bitcoin-i686-pc-linux-gnu/src/secp256k1 \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[4]: Entering directory '/tmp/cirrus-ci-build/ci/scratch/build/src/secp256k1'
make distdir-am
make[5]: Entering directory '/tmp/cirrus-ci-build/ci/scratch/build/src/secp256k1'
python3 tools/tests_wycheproof_generate.py /t
...

💬 real-or-random commented on pull request "Update src/secp256k1 subtree to release v0.3.1":
(https://github.com/bitcoin/bitcoin/pull/27445#discussion_r1162482056)
Shot in the dark: Can you try this and see if it makes CI happy?
```suggestion
src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.h:
python3 tools/tests_wycheproof_generate.py src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.json > $@

```

💬 MarcoFalke commented on pull request "fuzz: Add HeadersSyncState target":
(https://github.com/bitcoin/bitcoin/pull/26662#issuecomment-1502965567)
concept ack

💬 josibake commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#issuecomment-1502967998)
ACK https://github.com/bitcoin/bitcoin/pull/27217/commits/635c50a57b3f72f83f1ee681f6a4b7766b8988f6

💬 MarcoFalke commented on pull request "test: build Valgrind (3.20) from source & use Clang 16":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502977423)
I just tried bookworm and it was sufficient to remove the gcc suppression.

https://packages.debian.org/bookworm/gcc
https://packages.debian.org/bookworm/valgrind

💬 real-or-random commented on pull request "Update src/secp256k1 subtree to release v0.3.1":
(https://github.com/bitcoin/bitcoin/pull/27445#issuecomment-1502994264)
Okay, I think I understand what's going on here:
1. `make distdir` is supposed to create a directory with all files to be distributed
2. the `distdir` target depends on all files to be distributed
3. these file include `src/wycheproof/secdsa_secp256k1_sha256_bitcoin_test.h` because it's listed in `EXTRA_DIST` (perhaps `noinst_HEADERS` would be more appropriate, but the result is the same)
4. `src/wycheproof/secdsa_secp256k1_sha256_bitcoin_test.h` has a dependency on `src/wycheproof/secdsa_se
...

💬 dergoegge commented on pull request "test: LLVM/Clang 16 for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27436#issuecomment-1503006167)
utACK 676671527f08ef8113af3661de73db583f6ea9e2

💬 fanquake commented on pull request "wallet: undo conflicts properly in case of blocks disconnection":
(https://github.com/bitcoin/bitcoin/pull/17543#issuecomment-1503021950)
Dropped up for grabs here, see #27145.

💬 fanquake commented on pull request "[wallet] Track conflicted transactions removed from mempool and fix UI notifications":
(https://github.com/bitcoin/bitcoin/pull/18600#issuecomment-1503022715)
Dropped up for grabs here, see #27307.

🚀 fanquake merged a pull request: "test: LLVM/Clang 16 for MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27436)

💬 MarcoFalke commented on pull request "refactor, net processing: Avoid CNode::m_relays_txs usage":
(https://github.com/bitcoin/bitcoin/pull/27270#issuecomment-1503063052)
lgtm ACK 55c4795c5794c5c2f8a69b394b847413c9cfbe36

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1503076466)
> Would be nice to split the actual fix from the other changes. I guess it would be easier for other reviewers too.

I'm working on it.

> I would have liked to remove the `replySent` argument, but that's secondary to this PR which fixes a serious bug. Let's get the bug fixed and if I feel strong enough about it, then maybe I will open a followup to remove that parameter.

I agree, later on a follow-up, I'd try to investigate if `evhttp_request* req` can be mocked somehow so the fuzz can g
...

🚀 fanquake merged a pull request: "refactor, net processing: Avoid CNode::m_relays_txs usage"
(https://github.com/bitcoin/bitcoin/pull/27270)

👍 vasild approved a pull request: "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns"
(https://github.com/bitcoin/bitcoin/pull/27374#pullrequestreview-1379043911)
ACK b5585ba5f97a19d1b435d9ab69b5a55cfd45dd70

I think this fixes the problem described in https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1481322628 and is ok to be merged in its current form.

Could it create another problem of connecting too much to Tor and not try to diversify to IPv4? Yes. Is this unlikely problem? Yes, with a common addrman that is filled mostly with IPv4 addresses. With a non-common addrman? I do not know.