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

✅ maflcko closed a pull request: "rpc: Allow fullrbf fee bump in (psbt)bumpfee"
(https://github.com/bitcoin/bitcoin/pull/31953)

📝 maflcko reopened a pull request: "rpc: Allow fullrbf fee bump in (psbt)bumpfee"
(https://github.com/bitcoin/bitcoin/pull/31953)
The RPCs (psbt)bumpfee, and the GUI, reject fee bumps when BIP 125 signalling is absent in the transaction even when the mempool and other RPCs allow them. Fix the confusion by allowing the fee bump.

This is done after fullrbf is always on (https://github.com/bitcoin/bitcoin/pull/30592)

💬 yancyribbens commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2816691663)
Concept ACK

💬 yancyribbens commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2816692394)
Concept ACK