💬 hebasto commented on pull request "tests: Improve stderr validation in test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/32183#discussion_r2053824868)
This is intended to handle multi-config generator, which create per-config subdirectories:
```
$ cmake -B build-mc -G "Ninja Multi-Config"
$ cmake --build build-mc
$ ctest --test-dir build-mc -R util_test_runner
```

💬 hebasto commented on pull request "cmake: Improve robustness and usability":
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2053826793)
Answered in https://github.com/bitcoin/bitcoin/pull/32183#discussion_r2053824868.

👍 TheCharlatan approved a pull request: "rpc: add optional blockhash to waitfornewblock, unhide wait methods in help"
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-2783648578)
Re-ACK c6e2c31c55123cc97b4400bcbf3c37a39b067a22

💬 thesamesam commented on issue "PIE+LTO causes Bitcoin-Qt to segfault at startup":
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2820894205)
Ubuntu builds their Qt with "reduce relocations" disabled, so it doesn't hit this (see https://bugs.gentoo.org/933110#c5).

The root cause is a mix between Qt, CMake, and the fact there's no way for the toolchain to communicate a problem. CMake doesn't seem particularly interested in fixing it given https://gitlab.kitware.com/cmake/cmake/-/issues/15570#note_477736. Qt says that the right thing to do is to honour their own recommendation (naturally).

I spent a significant amount of time investig
...

⚠️ fanquake opened an issue: "build: CMake caching failing PIE check"
(https://github.com/bitcoin/bitcoin/issues/32323)
Similar to https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2703801270, CMake caches the check failure, which leaves the user in a broken state (unless they nuke the build dir):
```bash
cmake -B build -DAPPEND_CXXFLAGS="-ftrivial-auto-var-init"
-- The CXX compiler identification is GNU 14.2.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile
...

📝 hebasto opened a pull request: "test: Treat executable paths in tests consistently"
(https://github.com/bitcoin/bitcoin/pull/32324)
When using multi-config CMake generators, executable paths include per-config subdirectories, which require special handling in tests. Using dedicated environment variables to specify executable paths works well in such scenarios. However, the `util_test_runner` test sets these variables for the `util/test_runner.py` script unconditionally, which diverges from the approach used when running `functional/test_runner.py`.

This change makes the usage of the aforementioned environment variables un
...

💬 hebasto commented on pull request "tests: Improve stderr validation in test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/32183#discussion_r2053861553)
@maflcko

I believe https://github.com/bitcoin/bitcoin/pull/32324 should address your concerns.

💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2053864587)
This wasn't needed in the end, removed in latest push

💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2820945963)
Thanks for the suggestions @furszy, @achow101 and @maflcko.
Updated the commits to use `MakeWalletLoader` instead of manually closing, and moved the `epochIterations(1)` after the CI change to obviate that the CI already does that.

💬 TheCharlatan commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2820962881)
> I believe we've kept it merely as a sanity check because it's the only test that will survive the annihilation of the legacy wallet code.

Sounds like this should be an actual test then instead of a benchmark, no?

💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2053895857)
Thanks, extracted to a separate commit - as suggested by @luke-jr as well - added you both as co-authors.

💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2053896250)
Done, thanks

💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2820998715)
> Sounds like this should be an actual test then instead of a benchmark, no?

I also asked the same.
I suggest we fix and reenable full CI-validation first, and we can create a detailed test with proper asserts separately - since I'm not a wallet expert (yet?)

👍 laanwj approved a pull request: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2783785728)
Re-ACK e39118ab0bf08fdaf68b16c86ad304b9133b43c7
git range-diff b8cefeb221490868d62b7a0695f8b5ea392d3654..545a36ae0aa5687366fc4fdd54058072f333e73f b2bb27f40c7ab7b51720e41d136dcc9077a525b3..e39118ab0bf08fdaf68b16c86ad304b9133b43c7 commit
- `test: rpcs disabled for descriptor wallets were removed` was dropped and moved to another PR
- `tests, gui: Use descriptors watchonly wallet for watchonly test` was added
- `test: Remove legacy wallet unit tests`: qt related changes were removed, replaced b
...

👍 l0rinc approved a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2783771949)
ACK ea3cc8388b4aefcaee61f75e8bf1bc03281a0c91

LGTM, left a few nits, but none are blockers.
Before merging someone with more experience should also review to be sure.

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053942125)
I'm fine with this as well, but to avoid complexity on declaration site and usage site (and to skip the "weak" part which seems out-of-place), we could try:
```suggestion
hash_id = int(sha256(expected_exception.encode("utf-8")).hexdigest()[:8], 16)
if self.options.tmpdir:
args.append(f"--tmpdir={self.options.tmpdir}/{hash_id}")
if self.options.randomseed is not None:
args.append(f"--randomseed={self.options.randomseed ^ hash_id}")
```

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053935108)
Maybe we could make the tenses more uniform here:
```suggestion
# Launches a child test process that runs this same file, instantiates
# a child test, and verifies that it raises only the expected exception.
```

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053946928)
nit:
I know I wrote `.*` but we might want `.+` instead, since we likely don't support empty ones

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053917393)
yes, I meant either print + raise or just `raise Exception("Should have failed earlier during startup")`, as you've mentioned.
`assert False` feels like a hack...

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053944080)
nit: '\n' seems less cryptic:
```suggestion
assert not errors, f"Child test didn't contain (only) expected errors:\n{'\n'.join(errors)}\n<CHILD OUTPUT BEGIN>\n{output}\n<CHILD OUTPUT END>\n"
```