💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1162375326)
Yes this is nice to have. Since the key format could never change due to backward compatibility requirements, this is not an issue. You can resolve this comment.

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1162378013)
I guess that's ok, but `AddHDKey` already has an early return for existing keys.

👍 1440000bytes approved a pull request: "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet"
(https://github.com/bitcoin/bitcoin/pull/27279#pullrequestreview-1378615445)
utACK https://github.com/bitcoin/bitcoin/pull/27279/commits/7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55

💬 MarcoFalke commented on pull request "test: build Valgrind (3.20) from source & use Clang 16":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502850609)
Not sure. Requiring clang-16, and valgrind to be compiled from source, along with removing support for gcc seems a bit aggressive.

💬 fanquake commented on pull request "test: build Valgrind (3.20) from source & use Clang 16":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502851953)
What do you mean by "removing support for GCC"?

💬 0xB10C commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1502851999)
I've been trying to reproduce this by re-running the CI on master https://github.com/bitcoin/bitcoin/commit/49b87bfe7e2799d25ce709123ecafa872b36e87a: https://cirrus-ci.com/task/4749392670883840

So far all runs passed..

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