bench: wallet, fix change position out of range error #29065

furszy · 2023-12-12T18:23:11Z

Fixes #29061. Only the benchmark is affected.

Since #25273, the behavior of 'inserting change at a random position'
is instructed by passing ´std::nullopt´ instead of -1.

Also, added missing documentation about the meaning of
'change_pos=std::nullopt' inside 'CWallet::CreateTransaction()'

Since bitcoin#25273, the behavior of 'inserting change at a random position' is instructed by passing std::nullopt instead of -1. Also, added missing documentation about the meaning of 'change_pos=std::nullopt' inside 'CWallet::CreateTransaction()'

DrahtBot · 2023-12-12T18:23:14Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, kevkevinpal, BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

achow101 · 2023-12-12T20:02:56Z

ACK 37c75c5

aureleoules · 2023-12-12T20:31:48Z

This pull does fix the crash but I am still getting valgrind errors when executing the benchmark, which did not happen on master before. I haven't verified that these errors were introduced by #25273 but I would suppose so.

valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection
==243376== Memcheck, a memory error detector
==243376== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==243376== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==243376== Command: ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection
==243376==
==243376== Use of uninitialised value of size 8
==243376==    at 0x606AF0: wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1240)
==243376==    by 0x6086A6: wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1333)
==243376==    by 0x29843E: operator() (wallet_create_tx.cpp:132)
==243376==    by 0x29843E: 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)
==243376==    by 0x29A381: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:130)
==243376==    by 0x21BA5A: operator() (std_function.h:590)
==243376==    by 0x21BA5A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==243376==    by 0x1F84B6: main (bench_bitcoin.cpp:132)
==243376==
==243376== Use of uninitialised value of size 8
==243376==    at 0x7FABE9: IsDust(CTxOut const&, CFeeRate const&) (policy.cpp:67)
==243376==    by 0x606B2B: wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1249)
==243376==    by 0x6086A6: wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1333)
==243376==    by 0x29843E: operator() (wallet_create_tx.cpp:132)
==243376==    by 0x29843E: 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)
==243376==    by 0x29A381: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:130)
==243376==    by 0x21BA5A: operator() (std_function.h:590)
==243376==    by 0x21BA5A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==243376==    by 0x1F84B6: main (bench_bitcoin.cpp:132)
==243376==
==243376== Use of uninitialised value of size 8
==243376==    at 0x7FAACD: is_direct (prevector.h:170)
==243376==    by 0x7FAACD: size (prevector.h:292)
==243376==    by 0x7FAACD: IsUnspendable (script.h:554)
==243376==    by 0x7FAACD: GetDustThreshold(CTxOut const&, CFeeRate const&) (policy.cpp:42)
==243376==    by 0x7FAC00: IsDust(CTxOut const&, CFeeRate const&) (policy.cpp:67)
==243376==    by 0x606B2B: wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1249)
==243376==    by 0x6086A6: wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1333)
==243376==    by 0x29843E: operator() (wallet_create_tx.cpp:132)
==243376==    by 0x29843E: 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)
==243376==    by 0x29A381: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:130)
==243376==    by 0x21BA5A: operator() (std_function.h:590)
==243376==    by 0x21BA5A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==243376==    by 0x1F84B6: main (bench_bitcoin.cpp:132)
==243376==
==243376== Use of uninitialised value of size 8
==243376==    at 0x7FAB88: IsUnspendable (script.h:554)
==243376==    by 0x7FAB88: GetDustThreshold(CTxOut const&, CFeeRate const&) (policy.cpp:42)
==243376==    by 0x7FAC00: IsDust(CTxOut const&, CFeeRate const&) (policy.cpp:67)
==243376==    by 0x606B2B: wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1249)
==243376==    by 0x6086A6: wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1333)
==243376==    by 0x29843E: operator() (wallet_create_tx.cpp:132)
==243376==    by 0x29843E: 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)
==243376==    by 0x29A381: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:130)
==243376==    by 0x21BA5A: operator() (std_function.h:590)
==243376==    by 0x21BA5A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==243376==    by 0x1F84B6: main (bench_bitcoin.cpp:132)
==243376==
==243376== Use of uninitialised value of size 8
==243376==    at 0x8F8364: is_direct (prevector.h:170)
==243376==    by 0x8F8364: size (prevector.h:292)
==243376==    by 0x8F8364: CScript::IsWitnessProgram(int&, std::vector<unsigned char, std::allocator<unsigned char> >&) const (script.cpp:228)
==243376==    by 0x7FAB1F: GetDustThreshold(CTxOut const&, CFeeRate const&) (policy.cpp:54)
==243376==    by 0x7FAC00: IsDust(CTxOut const&, CFeeRate const&) (policy.cpp:67)
==243376==    by 0x606B2B: wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1249)
==243376==    by 0x6086A6: wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1333)
==243376==    by 0x29843E: operator() (wallet_create_tx.cpp:132)
==243376==    by 0x29843E: 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)
==243376==    by 0x29A381: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:130)
==243376==    by 0x21BA5A: operator() (std_function.h:590)
==243376==    by 0x21BA5A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==243376==    by 0x1F84B6: main (bench_bitcoin.cpp:132)
==243376==
==243376== Use of uninitialised value of size 8
==243376==    at 0x8F83AB: CScript::IsWitnessProgram(int&, std::vector<unsigned char, std::allocator<unsigned char> >&) const (script.cpp:231)
==243376==    by 0x7FAB1F: GetDustThreshold(CTxOut const&, CFeeRate const&) (policy.cpp:54)
==243376==    by 0x7FAC00: IsDust(CTxOut const&, CFeeRate const&) (policy.cpp:67)
==243376==    by 0x606B2B: wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1249)
==243376==    by 0x6086A6: wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1333)
==243376==    by 0x29843E: operator() (wallet_create_tx.cpp:132)
==243376==    by 0x29843E: 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)
==243376==    by 0x29A381: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:130)
==243376==    by 0x21BA5A: operator() (std_function.h:590)
==243376==    by 0x21BA5A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==243376==    by 0x1F84B6: main (bench_bitcoin.cpp:132)
==243376==
==243376== Use of uninitialised value of size 8
==243376==    at 0x8F83BF: item_ptr (prevector.h:167)
==243376==    by 0x8F83BF: operator[] (prevector.h:322)
==243376==    by 0x8F83BF: CScript::IsWitnessProgram(int&, std::vector<unsigned char, std::allocator<unsigned char> >&) const (script.cpp:234)
==243376==    by 0x7FAB1F: GetDustThreshold(CTxOut const&, CFeeRate const&) (policy.cpp:54)
==243376==    by 0x7FAC00: IsDust(CTxOut const&, CFeeRate const&) (policy.cpp:67)
==243376==    by 0x606B2B: wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1249)
==243376==    by 0x6086A6: wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1333)
==243376==    by 0x29843E: operator() (wallet_create_tx.cpp:132)
==243376==    by 0x29843E: 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)
==243376==    by 0x29A381: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:130)
==243376==    by 0x21BA5A: operator() (std_function.h:590)
==243376==    by 0x21BA5A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==243376==    by 0x1F84B6: main (bench_bitcoin.cpp:132)
==243376==
==243376== Use of uninitialised value of size 8
==243376==    at 0x8F840B: is_direct (prevector.h:170)
==243376==    by 0x8F840B: size (prevector.h:292)
==243376==    by 0x8F840B: end (prevector.h:302)
==243376==    by 0x8F840B: CScript::IsWitnessProgram(int&, std::vector<unsigned char, std::allocator<unsigned char> >&) const (script.cpp:236)
==243376==    by 0x7FAB1F: GetDustThreshold(CTxOut const&, CFeeRate const&) (policy.cpp:54)
==243376==    by 0x7FAC00: IsDust(CTxOut const&, CFeeRate const&) (policy.cpp:67)
==243376==    by 0x606B2B: wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1249)
==243376==    by 0x6086A6: wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1333)
==243376==    by 0x29843E: operator() (wallet_create_tx.cpp:132)
==243376==    by 0x29843E: 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)
==243376==    by 0x29A381: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:130)
==243376==    by 0x21BA5A: operator() (std_function.h:590)
==243376==    by 0x21BA5A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==243376==    by 0x1F84B6: main (bench_bitcoin.cpp:132)
==243376==
==243376== Conditional jump or move depends on uninitialised value(s)
==243376==    at 0x8F844B: _S_check_init_len (stl_vector.h:1769)
==243376==    by 0x8F844B: _M_range_initialize<prevector<28, unsigned char>::const_iterator> (stl_vector.h:1582)
==243376==    by 0x8F844B: vector<prevector<28, unsigned char>::const_iterator> (stl_vector.h:657)
==243376==    by 0x8F844B: CScript::IsWitnessProgram(int&, std::vector<unsigned char, std::allocator<unsigned char> >&) const (script.cpp:236)
==243376==    by 0x7FAB1F: GetDustThreshold(CTxOut const&, CFeeRate const&) (policy.cpp:54)
==243376==    by 0x7FAC00: IsDust(CTxOut const&, CFeeRate const&) (policy.cpp:67)
==243376==    by 0x606B2B: wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1249)
==243376==    by 0x6086A6: wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1333)
==243376==    by 0x29843E: operator() (wallet_create_tx.cpp:132)
==243376==    by 0x29843E: 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)
==243376==    by 0x29A381: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:130)
==243376==    by 0x21BA5A: operator() (std_function.h:590)
==243376==    by 0x21BA5A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==243376==    by 0x1F84B6: main (bench_bitcoin.cpp:132)
==243376==
==243376== Use of uninitialised value of size 8
==243376==    at 0x8F8470: __copy_m<prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:385)
==243376==    by 0x8F8470: __copy_move_a2<false, prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:495)
==243376==    by 0x8F8470: __copy_move_a1<false, prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:522)
==243376==    by 0x8F8470: __copy_move_a<false, prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:529)
==243376==    by 0x8F8470: copy<prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:620)
==243376==    by 0x8F8470: __uninit_copy<prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_uninitialized.h:110)
==243376==    by 0x8F8470: uninitialized_copy<prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_uninitialized.h:151)
==243376==    by 0x8F8470: __uninitialized_copy_a<prevector<28, unsigned char>::const_iterator, unsigned char*, unsigned char> (stl_uninitialized.h:333)
==243376==    by 0x8F8470: _M_range_initialize<prevector<28, unsigned char>::const_iterator> (stl_vector.h:1585)
==243376==    by 0x8F8470: vector<prevector<28, unsigned char>::const_iterator> (stl_vector.h:657)
==243376==    by 0x8F8470: CScript::IsWitnessProgram(int&, std::vector<unsigned char, std::allocator<unsigned char> >&) const (script.cpp:236)
==243376==    by 0x7FAB1F: GetDustThreshold(CTxOut const&, CFeeRate const&) (policy.cpp:54)
==243376==    by 0x7FAC00: IsDust(CTxOut const&, CFeeRate const&) (policy.cpp:67)
==243376==    by 0x606B2B: wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1249)
==243376==    by 0x6086A6: wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1333)
==243376==    by 0x29843E: operator() (wallet_create_tx.cpp:132)
==243376==    by 0x29843E: 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)
==243376==    by 0x29A381: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:130)
==243376==    by 0x21BA5A: operator() (std_function.h:590)
==243376==    by 0x21BA5A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==243376==    by 0x1F84B6: main (bench_bitcoin.cpp:132)
==243376==
==243376== Use of uninitialised value of size 8
==243376==    at 0x8F8482: __copy_m<prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:383)
==243376==    by 0x8F8482: __copy_move_a2<false, prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:495)
==243376==    by 0x8F8482: __copy_move_a1<false, prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:522)
==243376==    by 0x8F8482: __copy_move_a<false, prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:529)
==243376==    by 0x8F8482: copy<prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:620)
==243376==    by 0x8F8482: __uninit_copy<prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_uninitialized.h:110)
==243376==    by 0x8F8482: uninitialized_copy<prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_uninitialized.h:151)
==243376==    by 0x8F8482: __uninitialized_copy_a<prevector<28, unsigned char>::const_iterator, unsigned char*, unsigned char> (stl_uninitialized.h:333)
==243376==    by 0x8F8482: _M_range_initialize<prevector<28, unsigned char>::const_iterator> (stl_vector.h:1585)
==243376==    by 0x8F8482: vector<prevector<28, unsigned char>::const_iterator> (stl_vector.h:657)
==243376==    by 0x8F8482: CScript::IsWitnessProgram(int&, std::vector<unsigned char, std::allocator<unsigned char> >&) const (script.cpp:236)
==243376==    by 0x7FAB1F: GetDustThreshold(CTxOut const&, CFeeRate const&) (policy.cpp:54)
==243376==    by 0x7FAC00: IsDust(CTxOut const&, CFeeRate const&) (policy.cpp:67)
==243376==    by 0x606B2B: wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1249)
==243376==    by 0x6086A6: wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1333)
==243376==    by 0x29843E: operator() (wallet_create_tx.cpp:132)
==243376==    by 0x29843E: 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)
==243376==    by 0x29A381: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:130)
==243376==    by 0x21BA5A: operator() (std_function.h:590)
==243376==    by 0x21BA5A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==243376==    by 0x1F84B6: main (bench_bitcoin.cpp:132)
==243376==
==243376== Conditional jump or move depends on uninitialised value(s)
==243376==    at 0x8F8482: __copy_m<prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:383)
==243376==    by 0x8F8482: __copy_move_a2<false, prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:495)
==243376==    by 0x8F8482: __copy_move_a1<false, prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:522)
==243376==    by 0x8F8482: __copy_move_a<false, prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:529)
==243376==    by 0x8F8482: copy<prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_algobase.h:620)
==243376==    by 0x8F8482: __uninit_copy<prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_uninitialized.h:110)
==243376==    by 0x8F8482: uninitialized_copy<prevector<28, unsigned char>::const_iterator, unsigned char*> (stl_uninitialized.h:151)
==243376==    by 0x8F8482: __uninitialized_copy_a<prevector<28, unsigned char>::const_iterator, unsigned char*, unsigned char> (stl_uninitialized.h:333)
==243376==    by 0x8F8482: _M_range_initialize<prevector<28, unsigned char>::const_iterator> (stl_vector.h:1585)
==243376==    by 0x8F8482: vector<prevector<28, unsigned char>::const_iterator> (stl_vector.h:657)
==243376==    by 0x8F8482: CScript::IsWitnessProgram(int&, std::vector<unsigned char, std::allocator<unsigned char> >&) const (script.cpp:236)
==243376==    by 0x7FAB1F: GetDustThreshold(CTxOut const&, CFeeRate const&) (policy.cpp:54)
==243376==    by 0x7FAC00: IsDust(CTxOut const&, CFeeRate const&) (policy.cpp:67)
==243376==    by 0x606B2B: wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1249)
==243376==    by 0x6086A6: wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient> > const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) (spend.cpp:1333)
==243376==    by 0x29843E: operator() (wallet_create_tx.cpp:132)
==243376==    by 0x29843E: 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)
==243376==    by 0x29A381: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:130)
==243376==    by 0x21BA5A: operator() (std_function.h:590)
==243376==    by 0x21BA5A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==243376==    by 0x1F84B6: main (bench_bitcoin.cpp:132)
==243376==



|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|    1,013,313,787.80 |                0.99 |    0.8% |12,459,882,645.20 |5,208,416,083.40 |  2.392 |1,982,028,973.60 |    1.1% |     55.84 | `WalletCreateTxUsePresetInputsAndCoinSelection`
==243376==
==243376== HEAP SUMMARY:
==243376==     in use at exit: 1,241 bytes in 7 blocks
==243376==   total heap usage: 23,470,975 allocs, 23,470,968 frees, 6,267,436,104 bytes allocated
==243376==
==243376== LEAK SUMMARY:
==243376==    definitely lost: 0 bytes in 0 blocks
==243376==    indirectly lost: 0 bytes in 0 blocks
==243376==      possibly lost: 0 bytes in 0 blocks
==243376==    still reachable: 1,241 bytes in 7 blocks
==243376==         suppressed: 0 bytes in 0 blocks
==243376== Rerun with --leak-check=full to see details of leaked memory
==243376==
==243376== Use --track-origins=yes to see where uninitialised values come from
==243376== For lists of detected and suppressed errors, rerun with: -s
==243376== ERROR SUMMARY: 480 errors from 12 contexts (suppressed: 0 from 0)

kevkevinpal · 2023-12-13T02:34:23Z

ACK 37c75c5

closes #29069

Was able to reproduce issue and building with this change fixed it for me

BrandonOdiwuor

Concept ACK

BrandonOdiwuor

ACK 37c75c5

Looks good to me

achow101 · 2023-12-13T17:38:03Z

This pull does fix the crash but I am still getting valgrind errors when executing the benchmark

Unable to replicate this, so probably a separate issue.

aureleoules · 2023-12-13T18:02:24Z

Unable to replicate this, so probably a separate issue.

I can consistently replicate the valgrind errors on a fresh new ubuntu docker container:

Ubuntu 22.04.3 LTS
g++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
valgrind-3.18.1

Using : 
./configure --enable-bench --disable-tests --disable-gui --disable-zmq --disable-fuzz --enable-fuzz-binary=no
valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection

These errors do not appear in the commit 2d39db7 which is the previous commit of 758501b, the one that introduced the crash.
This seems important to fix to me.

maflcko · 2023-12-13T19:52:40Z

This seems important to fix to me.

I tried on current master with clang-13 and valgrind on Jammy, but it seems to pass.

Maybe it is a bug in your g++?

# git log -1 && valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection 
commit 9f0f83d6509a214b827f5110c0f857b494ae854c (HEAD, origin/master, origin/HEAD)
Merge: 019ec8a 37c75c5
Author: Ava Chow <github@achow101.com>
Date:   Wed Dec 13 12:38:11 2023 -0500

    Merge bitcoin/bitcoin#29065: bench: wallet, fix change position out of range error
    
    37c75c58202f89b752500f76c872d7f8caf6bdb3 test: wallet, fix change position out of range error (furszy)
    
    Pull request description:
    
      Fixes #29061. Only the benchmark is affected.
    
      Since #25273, the behavior of 'inserting change at a random position'
      is instructed by passing <C2><B4>std::nullopt<C2><B4> instead of -1.
    
      Also, added missing documentation about the meaning of
      'change_pos=std::nullopt' inside 'CWallet::CreateTransaction()'
    
    ACKs for top commit:
      achow101:
        ACK 37c75c58202f89b752500f76c872d7f8caf6bdb3
      kevkevinpal:
        ACK [37c75c5](https://github.com/bitcoin/bitcoin/pull/29065/commits/37c75c58202f89b752500f76c872d7f8caf6bdb3)
      BrandonOdiwuor:
        ACK 37c75c58202f89b752500f76c872d7f8caf6bdb3
    
    Tree-SHA512: d9a8d8533540455716a5090fcf407573cad9f0d0018a05f903f89e51620302f9b256318db6f7338b85c047f7fab372d724e916b1721d7ed302dbf3d845b08734
==45269== Memcheck, a memory error detector
==45269== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==45269== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==45269== Command: ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection
==45269== 
Warning, results might be unstable:
* CPU frequency scaling enabled: CPU 0 between 800.0 and 2,800.0 MHz
* CPU governor is 'powersave' but should be 'performance'

Recommendations
* Use 'pyperf system tune' before benchmarking. See https://github.com/psf/pyperf

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|    2,889,646,053.20 |                0.35 |    0.6% |    159.99 | `WalletCreateTxUsePresetInputsAndCoinSelection`
==45269== 
==45269== HEAP SUMMARY:
==45269==     in use at exit: 1,241 bytes in 7 blocks
==45269==   total heap usage: 23,469,947 allocs, 23,469,940 frees, 6,267,372,825 bytes allocated
==45269== 
==45269== LEAK SUMMARY:
==45269==    definitely lost: 0 bytes in 0 blocks
==45269==    indirectly lost: 0 bytes in 0 blocks
==45269==      possibly lost: 0 bytes in 0 blocks
==45269==    still reachable: 1,241 bytes in 7 blocks
==45269==         suppressed: 0 bytes in 0 blocks
==45269== Rerun with --leak-check=full to see details of leaked memory
==45269== 
==45269== For lists of detected and suppressed errors, rerun with: -s
==45269== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

aureleoules · 2023-12-13T21:07:04Z

Maybe it is a bug in your g++?

Right maybe, I just tested with g++ 13.2.0 and it runs fine now. No worries then.

Post-merge ACK 37c75c5

DrahtBot added the Tests label Dec 12, 2023

furszy mentioned this pull request Dec 12, 2023

Benchmark WalletCreateTxUsePresetInputsAndCoinSelection crashes due to #25273 #29061

Closed

1 task

achow101 requested a review from aureleoules December 12, 2023 20:03

furszy mentioned this pull request Dec 13, 2023

[bench] Assertion error on wallet_create_tx.cpp, line 133. #29069

Closed

1 task

fanquake requested review from achow101 and removed request for achow101 December 13, 2023 09:44

BrandonOdiwuor reviewed Dec 13, 2023

View reviewed changes

BrandonOdiwuor approved these changes Dec 13, 2023

View reviewed changes

achow101 merged commit 9f0f83d into bitcoin:master Dec 13, 2023
16 checks passed

furszy deleted the 2023_bench_wallet_fix branch December 13, 2023 20:17

furszy mentioned this pull request Apr 2, 2024

refactor: interfaces, make 'createTransaction' less error-prone bitcoin-core/gui#807

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bench: wallet, fix change position out of range error #29065

bench: wallet, fix change position out of range error #29065

furszy commented Dec 12, 2023

DrahtBot commented Dec 12, 2023 •

edited

achow101 commented Dec 12, 2023

aureleoules commented Dec 12, 2023

kevkevinpal commented Dec 13, 2023 •

edited

BrandonOdiwuor left a comment

BrandonOdiwuor left a comment

achow101 commented Dec 13, 2023

aureleoules commented Dec 13, 2023

maflcko commented Dec 13, 2023

aureleoules commented Dec 13, 2023 •

edited

bench: wallet, fix change position out of range error #29065

bench: wallet, fix change position out of range error #29065

Conversation

furszy commented Dec 12, 2023

DrahtBot commented Dec 12, 2023 • edited

Code Coverage

Reviews

achow101 commented Dec 12, 2023

aureleoules commented Dec 12, 2023

kevkevinpal commented Dec 13, 2023 • edited

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

achow101 commented Dec 13, 2023

aureleoules commented Dec 13, 2023

maflcko commented Dec 13, 2023

aureleoules commented Dec 13, 2023 • edited

DrahtBot commented Dec 12, 2023 •

edited

kevkevinpal commented Dec 13, 2023 •

edited

aureleoules commented Dec 13, 2023 •

edited