Page MenuHomePhabricator
Differential D17954

wallet, refactor: return out-params of CreateTransaction() as optional struct
ClosedPublic
Actions

Authored by Fabien on Sat, Apr 19, 14:28.

Details

Reviewers
PiRK
Group Reviewers
Restricted Project
Commits
rABC4d8d31e7604a: wallet, refactor: return out-params of CreateTransaction() as optional struct
Summary
The method CWallet::CreateTransaction currently returns several values in the form of out-parameters:

    the actual newly created transaction (CTransactionRef& tx)
    its required fee (CAmount& nFeeRate)
    the position of the change output (int& nChangePosInOut) -- as the name suggests, this is both an in- and out-param

By returning these values in an optional structure (which returns no value a.k.a. std::nullopt if an error occured), the interfaces is shorter, cleaner (requested change position is now in-param and can be passed by value) and callers don't have to create dummy variables for results that they are not interested in.

Note that the names of the replaced out-variables were kept in CreateTransactionInternal to keep the diff minimal. Also, the fee calculation data (FeeCalculation& fee_calc_out) would be another candidate to put into the structure, but FeeCalculation is currently an opaque data type in the wallet interface and I think it should stay that way.

Backport of core#20640.

Depends on D17953.

Test Plan
ninja all check-all

Diff Detail

Event Timeline

Fabien created this revision.Sat, Apr 19, 14:28
Herald added a reviewer: Restricted Project. · View Herald TranscriptSat, Apr 19, 14:28
Fabien requested review of this revision.Sat, Apr 19, 14:28
teamcity edited the summary of this revision. (Show Details)Sat, Apr 19, 14:28
Harbormaster completed remote builds in B33018: Diff 53559.Sat, Apr 19, 14:30
teamcity added a comment.Sat, Apr 19, 14:30
Build Bitcoin ABC Diffs / Diff Testing (build-clang) failed.

Tail of the build log:

[409/582] Linking C static library src/secp256k1/libsecp256k1.a
[410/582] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[411/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[412/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[413/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/receive.cpp.o
[414/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[415/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/signmessage.cpp.o
[416/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/encrypt.cpp.o
[417/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/util.cpp.o
[418/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[419/582] Installing component secp256k1
-- Install configuration: "RelWithDebInfo"
-- Install component: "secp256k1"
-- Installing: /results/artifacts/lib/libsecp256k1.a
-- Installing: /results/artifacts/include/secp256k1.h
-- Installing: /results/artifacts/include/secp256k1_preallocated.h
-- Installing: /results/artifacts/include/secp256k1_recovery.h
-- Installing: /results/artifacts/include/secp256k1_schnorr.h
[420/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[421/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[422/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/transaction.cpp.o
[423/582] Linking CXX static library src/libbitcoinconsensus.a
[424/582] Linking C executable src/secp256k1/sign-bench
[425/582] Linking C executable src/secp256k1/internal-bench
[426/582] Linking C executable src/secp256k1/verify-bench
[427/582] Linking C executable src/secp256k1/ecmult-bench
[428/582] Linking C executable src/secp256k1/recover-bench
[429/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[430/582] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[431/582] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[432/582] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[433/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[434/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[435/582] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[436/582] Building CXX object src/iguana/CMakeFiles/iguana.dir/iguana_formatter.cpp.o
[437/582] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[438/582] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[439/582] Building CXX object src/iguana/CMakeFiles/iguana.dir/iguana_interpreter.cpp.o
[440/582] Building CXX object src/iguana/CMakeFiles/iguana.dir/iguana.cpp.o
[441/582] Linking CXX static library src/libscript.a
[442/582] Linking CXX static library src/libcommon.a
[443/582] Linking CXX shared library src/libbitcoinconsensus.so.0.31.2
[444/582] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[445/582] Linking CXX executable src/iguana/iguana
[446/582] Linking CXX executable src/bitcoin-cli
[447/582] Linking CXX executable src/bitcoin-tx
[448/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/spend.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/spend.cpp.o 
/usr/bin/ccache /usr/bin/clang++ -DBOOST_ALL_NO_LIB -DBOOST_NO_CXX98_FUNCTION_BASE -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I/work/src/. -I/work/abc-ci-builds/build-clang/src -I/work/src/univalue/include -I/work/abc-ci-builds/build-clang/src/crypto/.. -I/work/src/secp256k1/include -I/work/src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIC -fvisibility=hidden -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wgnu -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety -Wrange-loop-analysis -Wredundant-decls -Wunreachable-code-loop-increment -Wsign-compare -Wconditional-uninitialized -Wdocumentation -Wformat-security -Wredundant-move -Woverloaded-virtual -Wshadow -Wshadow-field -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/spend.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/spend.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/spend.cpp.o -c /work/src/wallet/spend.cpp
/work/src/wallet/spend.cpp:1010:28: error: not a Doxygen trailing comment [-Werror,-Wdocumentation]
            Amount::zero() //< 0 means non-functional fee rate estimation
                           ^~~
                           ///<
1 error generated.
[449/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[450/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o
[451/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o
[452/582] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang failed with exit code 1
teamcity added a comment.Sat, Apr 19, 14:31
Build Bitcoin ABC Diffs / Diff Testing (build-clang-tidy) failed.

Tail of the build log:

[404/580] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[405/580] Building CXX object src/test/CMakeFiles/testutil.dir/util/random.cpp.o
[406/580] Building CXX object src/test/CMakeFiles/testutil.dir/util/coins.cpp.o
[407/580] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockindex.cpp.o
[408/580] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[409/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[410/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[411/580] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[412/580] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[413/580] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[414/580] Building CXX object src/CMakeFiles/bitcoin-wallet.dir/bitcoin-wallet.cpp.o
[415/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[416/580] Building CXX object src/CMakeFiles/server.dir/wallet/init.cpp.o
[417/580] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[418/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[419/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[420/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[421/580] Building CXX object src/test/CMakeFiles/testutil.dir/util/validation.cpp.o
[422/580] Building CXX object src/test/CMakeFiles/testutil.dir/util/txmempool.cpp.o
[423/580] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[424/580] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[425/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[426/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/transaction.cpp.o
[427/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[428/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/encrypt.cpp.o
[429/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/receive.cpp.o
[430/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[431/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/signmessage.cpp.o
[432/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/util.cpp.o
[433/580] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[434/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[435/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[436/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/spend.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/spend.cpp.o 
/usr/bin/cmake -E __run_co_compile --launcher=/usr/bin/ccache --tidy="/usr/bin/clang-tidy-16;-warnings-as-errors=*;--extra-arg-before=--driver-mode=g++" --source=/work/src/wallet/spend.cpp -- /usr/bin/clang++ -DBOOST_ALL_NO_LIB -DBOOST_NO_CXX98_FUNCTION_BASE -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I/work/src/. -I/work/abc-ci-builds/build-clang-tidy/src -I/work/src/univalue/include -I/work/abc-ci-builds/build-clang-tidy/src/crypto/.. -I/work/src/secp256k1/include -I/work/src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -g -O2 -fPIC -fvisibility=hidden -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wgnu -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety -Wrange-loop-analysis -Wredundant-decls -Wunreachable-code-loop-increment -Wsign-compare -Wconditional-uninitialized -Wdocumentation -Wformat-security -Wredundant-move -Woverloaded-virtual -Wshadow -Wshadow-field -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/spend.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/spend.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/spend.cpp.o -c /work/src/wallet/spend.cpp
/work/src/wallet/spend.cpp:1010:28: error: not a Doxygen trailing comment [clang-diagnostic-documentation,-warnings-as-errors]
            Amount::zero() //< 0 means non-functional fee rate estimation
                           ^~~
                           ///<
2290 warnings generated.
Suppressed 2289 warnings (2289 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
[437/580] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[438/580] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[439/580] Building CXX object src/iguana/CMakeFiles/iguana.dir/iguana_formatter.cpp.o
[440/580] Building CXX object src/iguana/CMakeFiles/iguana.dir/iguana_interpreter.cpp.o
[441/580] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[442/580] Building CXX object src/iguana/CMakeFiles/iguana.dir/iguana.cpp.o
[443/580] Linking CXX executable src/iguana/iguana
[444/580] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[445/580] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[446/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[447/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[448/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[449/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o
[450/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o
[451/580] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1
PiRK requested changes to this revision.Sat, Apr 19, 16:47
PiRK added a subscriber: PiRK.
PiRK added inline comments.
src/wallet/spend.cpp
1010

clang-tidy does not like this comment style

This revision now requires changes to proceed.Sat, Apr 19, 16:47
Fabien updated this revision to Diff 53569.Sun, Apr 20, 19:46

Fix comment

Harbormaster completed remote builds in B33025: Diff 53569.Sun, Apr 20, 20:10
PiRK accepted this revision.Mon, Apr 21, 06:20
PiRK added inline comments.
src/wallet/spend.cpp
1016 ↗(On Diff #53569)

I think this comment still refers to error2 and should go above it.
It is not a terribly useful comment, though.

This revision is now accepted and ready to land.Mon, Apr 21, 06:20
Fabien added inline comments.Mon, Apr 21, 08:06
src/wallet/spend.cpp
1016 ↗(On Diff #53569)

yes it's less obvious with in-out params... It's getting removed soon anyway

Closed by commit rABC4d8d31e7604a: wallet, refactor: return out-params of CreateTransaction() as optional struct (authored by Sebastian Falbesoner <sebastian.falbesoner@gmail.com>, committed by Fabien). · Explain WhyMon, Apr 21, 08:11
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC4d8d31e7604a: wallet, refactor: return out-params of CreateTransaction() as optional struct.

Revision Contents
Changeset List

PathSize
src/
wallet/
12 lines
12 lines
22 lines
94 lines
test/
9 lines

Diff 53559

View Options

src/wallet/interfaces.cpp

Loading...
View Options

src/wallet/rpcwallet.cpp

Loading...
View Options

src/wallet/spend.h

Loading...
View Options

src/wallet/spend.cpp

Loading...
View Options

src/wallet/test/wallet_tests.cpp

Loading...