💬 l0rinc commented on pull request "ci: Temporarily disable `WalletMigration` benchmark":
(https://github.com/bitcoin/bitcoin/pull/32306#discussion_r2051446820)
I've pushed https://github.com/bitcoin/bitcoin/pull/32309 which seems to fix the build (based on https://github.com/l0rinc/bitcoin/pull/9)

💬 hebasto commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2051447678)
Should the fact that the migrated wallet databases remain open be documented in `MigrateLegacyToDescriptor`?

💬 l0rinc commented on pull request "bench: close wallets after migration":
(https://github.com/bitcoin/bitcoin/pull/32309#discussion_r2051448501)
I needed to close both for the build to pass (`res->wallet->Close()` alone wasn't enough).
Maybe it passes with only `res->watchonly_wallet->Close()` - I can check if you think it makes sense.

💬 muaawiyahtucker commented on issue ""Rolling forward" at startup can take a long time, and is not interruptible (after unclean shutdown)":
(https://github.com/bitcoin/bitcoin/issues/11600#issuecomment-2816654839)
Running Raspberry pi 4, 8GB ram, I put the dbcache at 5500, thinking I was smart and would get a faster sync, but then it crashed at around 833432.
I got the error in the log:
` ERROR: Commit: Failed to commit latest txindex state`
I didn’t understand why it did that, so I tried to reduce the dbcache to 5000, but it seems that when I did that, it ‘unclearly restarted’, because when it rebooted, I got this rolling business. It is slow, but for me, every time it got to a certain point, near where
...

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