💬 hebasto commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2051451297)
I wonder why `res->watchonly_wallet.use_count() == 3`? Is this the expected behavior of the `MigrateLegacyToDescriptor()` function?

💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2051452641)
Maybe @furszy knows.

💬 hebasto commented on issue "ci: failure in Windows cross-test":
(https://github.com/bitcoin/bitcoin/issues/32291#issuecomment-2816658407)
The failure in the `WalletMigration` migration benchmark is not Windows-specific.

On Ubuntu 24.04:
```
$ ./build/bin/bench_bitcoin -filter=WalletMigration -min-time=100000
...
Segmentation fault (core dumped)
```

Here is the backtrace:
```
(lldb) bt
error: bench_bitcoin 0x00067927: DW_TAG_member '_M_local_buf' refers to type 0x00000000000b3ba7 which extends beyond the bounds of 0x0006791d
* thread #1, name = 'b-test', stop reason = signal SIGSEGV: address not mapped to object (fault address: 0
...

👋 l0rinc's pull request is ready for review: "bench: close wallets after migration"
(https://github.com/bitcoin/bitcoin/pull/32309)

💬 hebasto commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2051454202)
Unfortunately, this patch does not fix the test case posted [here](https://github.com/bitcoin/bitcoin/issues/32291#issuecomment-2816658407).

💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2051457940)
> Maybe it passes with only res->watchonly_wallet->Close() - I can check, if you think it makes sense

No, we do need to close both: https://github.com/l0rinc/bitcoin/actions/runs/14548535133/job/40817048030?pr=9

✅ maflcko closed a pull request: "Update netaddress.cpp"
(https://github.com/bitcoin/bitcoin/pull/32307)

💬 maflcko commented on pull request "ci: Temporarily disable `WalletMigration` benchmark":
(https://github.com/bitcoin/bitcoin/pull/32306#issuecomment-2816671123)
lgtm ACK 18a035145d6abb41e49711de60c4feabe3ba31e9

Makes sense to temporarily disable a single known-to-fail benchmark, until it is known to work.

✅ maflcko closed a pull request: "ci: Temporarily revert "Drop bench -priority-level in win cross CI""
(https://github.com/bitcoin/bitcoin/pull/32302)

💬 maflcko commented on pull request "ci: Temporarily revert "Drop bench -priority-level in win cross CI"":
(https://github.com/bitcoin/bitcoin/pull/32302#issuecomment-2816672317)
> Can we add a different change, to continue running all benchmarks (I guess just not Windows), so we aren't back to the same state that allowed #32277 to happen?

Yes, happy to follow-up next week, as explained in https://github.com/bitcoin/bitcoin/pull/32288#discussion_r2048585578



> May this be closed in favour of #32306?

No strong opinion, I think either is equally fine.

Closing this one for now, because it has less acks.

💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2051461073)
> Unfortunately, this patch does not fix the test case posted https://github.com/bitcoin/bitcoin/issues/32291#issuecomment-2816658407.

I can reproduce the failure with
```bash
cmake --preset=libfuzzer -DBUILD_FOR_FUZZING=OFF -DBUILD_BENCH=ON && cmake --build build_fuzz -j$(nproc) && build_fuzz/bin/bench_bitcoin -filter='WalletMigration' --min-time=10000
```

I pushed a new version based on https://github.com/bitcoin/bitcoin/blob/master/src/bench/wallet_create.cpp#L54 which calls RemoveWa
...

💬 hebasto commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2816673705)
In commit 86646448770888f66c446eacdd47d1a62d5552eb, "... we're only running 1 epochs anyway" -- where does this assumption comes from?

💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2816673874)
> where does this assumption comes

> bench.epochs(/*numEpochs=*/1)

🚀 hebasto merged a pull request: "ci: Temporarily disable `WalletMigration` benchmark"
(https://github.com/bitcoin/bitcoin/pull/32306)

📝 maflcko opened a pull request: "test: Run all benchmarks in the sanity check"
(https://github.com/bitcoin/bitcoin/pull/32310)
It is unclear why not all benchmarks are run, given that:

* they only run as a sanity check (fastest version)
* no one otherwise runs them, not even CI
* issues have been missed due to this

👍 hebasto approved a pull request: "test: Run all benchmarks in the sanity check"
(https://github.com/bitcoin/bitcoin/pull/32310#pullrequestreview-2779948289)
ACK fa2a38a8a5a6b4130b9796df91596964d882ddfa, I have reviewed the code and it looks OK.

💬 maflcko commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2816675608)
needs to revert d91a746815e4428c738f1a096a950292cbdb5469 here in this pull

💬 hebasto commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2816675828)
Tested 86646448770888f66c446eacdd47d1a62d5552eb on Ubuntu 24.04:
```
$ ./build/bin/bench_bitcoin -filter=WalletMigration -min-time=100000
Warning, results might be unstable:
* CPU frequency scaling enabled: CPU 0 between 800.0 and 4,800.0 MHz
* CPU governor is 'powersave' but should be 'performance'
* Turbo is enabled, CPU frequency will fluctuate

Recommendations
* Use 'pyperf system tune' before benchmarking. See https://github.com/psf/pyperf
Segmentation fault (core dumped)
```

💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2816676660)
> Segmentation fault (core dumped)

Can you show me the full build command please, `cmake --preset=libfuzzer -DBUILD_FOR_FUZZING=OFF -DBUILD_BENCH=ON && cmake --build build_fuzz -j$(nproc) && build_fuzz/bin/bench_bitcoin -filter='WalletMigration' --min-time=10000` passes for me on Ubuntu.

💬 hebasto commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#issuecomment-2816677598)
> > Segmentation fault (core dumped)
>
> Can you show me the full build command please, `cmake --preset=libfuzzer -DBUILD_FOR_FUZZING=OFF -DBUILD_BENCH=ON && cmake --build build_fuzz -j$(nproc) && build_fuzz/bin/bench_bitcoin -filter='WalletMigration' --min-time=10000` passes for me on Ubuntu.

```
$ rm -rf build && cmake -B build -DBUILD_BENCH=ON
$ cmake --build build -t bench_bitcoin
$ ./build/bin/bench_bitcoin -filter=WalletMigration -min-time=100000
Warning, results might be unstabl
...