Benchmark WalletCreateTxUsePresetInputsAndCoinSelection crashes due to #25273

[aureleoules]

Is there an existing issue for this?

• I have searched the existing issues

Current behaviour

The WalletCreateTxUsePresetInputsAndCoinSelection benchmark crashes on master due to the commit 758501b introduced in #25273 (I used git bisect to verify).

valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection -min-time=10000
==34277== Memcheck, a memory error detector
==34277== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==34277== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==34277== Command: ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection -min-time=10000
==34277==
bench_bitcoin: bench/wallet_create_tx.cpp:135: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>)::<lambda()>: Assertion `tx_res' failed.
==34277==
==34277== Process terminating with default action of signal 6 (SIGABRT)
==34277==    at 0x4DD79FC: __pthread_kill_implementation (pthread_kill.c:44)
==34277==    by 0x4DD79FC: __pthread_kill_internal (pthread_kill.c:78)
==34277==    by 0x4DD79FC: pthread_kill@@GLIBC_2.34 (pthread_kill.c:89)
==34277==    by 0x4D83475: raise (raise.c:26)
==34277==    by 0x4D697F2: abort (abort.c:79)
==34277==    by 0x4D6971A: __assert_fail_base.cold (assert.c:92)
==34277==    by 0x4D7AE95: __assert_fail (assert.c:101)
==34277==    by 0x29607B: operator() (wallet_create_tx.cpp:135)
==34277==    by 0x29607B: ankerl::nanobench::Bench& ankerl::nanobench::Bench::run<WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>)::{lambda()#3}>(WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>)::{lambda()#3}&&) [clone .isra.0] (nanobench.h:1221)
==34277==    by 0x297F81: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:132)
==34277==    by 0x21B74A: operator() (std_function.h:590)
==34277==    by 0x21B74A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==34277==    by 0x1F8B96: main (bench_bitcoin.cpp:132)
==34277==
==34277== HEAP SUMMARY:
==34277==     in use at exit: 48,159,834 bytes in 72,552 blocks
==34277==   total heap usage: 1,668,158 allocs, 1,595,606 frees, 552,252,252 bytes allocated
==34277==
==34277== LEAK SUMMARY:
==34277==    definitely lost: 0 bytes in 0 blocks
==34277==    indirectly lost: 0 bytes in 0 blocks
==34277==      possibly lost: 912 bytes in 3 blocks
==34277==    still reachable: 48,158,922 bytes in 72,549 blocks
==34277==         suppressed: 0 bytes in 0 blocks
==34277== Rerun with --leak-check=full to see details of leaked memory
==34277==
==34277== For lists of detected and suppressed errors, rerun with: -s
==34277== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Aborted

Expected behaviour

No crash

Steps to reproduce

git checkout master && make -j$(nproc)
valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection -min-time=10000

Relevant log output

No response

How did you obtain Bitcoin Core

Compiled from source

What version of Bitcoin Core are you using?

master@758501b71391136c33b525b1a0109b990d4f463e

Operating system and version

Ubuntu 22.04.3 LTS

Machine specifications

No response

[fanquake]

Was this fixed in #29055?

[maflcko]

Why was this not caught by CI?

[fanquake]

Why was this not caught by CI?

Looks like that is a "low priority" benchmark, meaning it's not run in the CI, which only run high?

[aureleoules]

Was this fixed in #29055?

Unfortunately no, it still crashes on master as of a7484be

[aureleoules]

Why was this not caught by CI?

Yeah, I don't believe all the benchmarks are being run. I think they should all be executed with at least -min-time=1000 as well.
I've noticed while working on my bench tool (context #27284 (comment)) that a few pulls almost introduced these bugs.
And some benchs only crash with -min-time=1000 or -min-time=10000 for some reasons.

[fanquake]

Unfortunately no, it still crashes on master as of a7484be

cc @achow101 @furszy

[furszy]

The issue is the change output index. Since #25273, needs to be std::nullopt, not -1 (which now throws "out of range").

[furszy]

Also, this will happen at the GUI level too. We are passing -1 at the wallet controller level as well.. --> false positive.
Will work on both things, add coverage and push the fix in few hours.

[furszy]

Created #29065 solving this.
Only the benchmark has been affected. The GUI side finding was just a false-positive; ugly code that we should clean up sooner rather than later.