Differential D14023

Rewrite OutputGroups to be clearer and to use scriptPubKeys
ClosedPublic
Actions

Authored by Fabien on Jun 8 2023, 21:04.

Details

Reviewers

PiRK

Group Reviewers

Restricted Project

Commits

rABC5726657eaaf7: Rewrite OutputGroups to be clearer and to use scriptPubKeys

Summary

Rewrite OutputGroups so that the logic is easier to follow and understand.

There is a slight behavior change as OutputGroups will be grouped by scriptPubKey rather than CTxDestination as before. This should have no effect on users as all addresses are a CTxDestination. However by using
scriptPubKeys, we can correctly group outputs which fall into the NoDestination case. But we also shouldn't have any NoDestination outputs.

Completes backport of core#20040:
https://github.com/bitcoin/bitcoin/pull/20040/commits/5d4597666d589e39354e0d8dd5b2afbe1a5d7d8e

Depends on D14022.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.Jun 8 2023, 21:04

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 8 2023, 21:04

Fabien requested review of this revision.Jun 8 2023, 21:04

teamcity edited the summary of this revision. (Show Details)Jun 8 2023, 21:04

Fabien added a child revision: D14024: Remove tx chained limit from the coin selection algorithm.Jun 8 2023, 21:27

Harbormaster completed remote builds in B23959: Diff 40673.Jun 8 2023, 21:39

Build Bitcoin ABC Diffs / Diff Testing (build-clang-tidy) failed.

Tail of the build log:

[375/537] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[376/537] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[377/537] Building CXX object src/CMakeFiles/server.dir/wallet/init.cpp.o
[378/537] Linking CXX executable src/bitcoin-tx
[379/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[380/537] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[381/537] Building CXX object src/test/CMakeFiles/testutil.dir/util/validation.cpp.o
[382/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[383/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[384/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[385/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[386/537] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[387/537] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[388/537] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[389/537] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[390/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[391/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[392/537] 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-12;-warnings-as-errors=*;--extra-arg-before=--driver-mode=g++" --source=../../src/wallet/spend.cpp -- /usr/bin/clang++ -DBOOST_ALL_NO_LIB -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../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -g -O2 -fPIC -fvisibility=hidden -fstack-protector-all -Wstack-protector -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 -pthread -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 ../../src/wallet/spend.cpp
/work/abc-ci-builds/build-clang-tidy/../../src/wallet/spend.cpp:599:36: error: argument name 'include_partial_groups' in comment does not match parameter name 'include_partial' [bugprone-argument-comment,-warnings-as-errors]
                                   /*include_partial_groups=*/true),
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
                                   /*include_partial=*/
../../src/./wallet/coinselection.h:81:32: note: 'include_partial' declared here
                          bool include_partial)
                               ^
/work/abc-ci-builds/build-clang-tidy/../../src/wallet/spend.cpp:607:36: error: argument name 'include_partial_groups' in comment does not match parameter name 'include_partial' [bugprone-argument-comment,-warnings-as-errors]
                                   /*include_partial_groups=*/true),
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
                                   /*include_partial=*/
../../src/./wallet/coinselection.h:81:32: note: 'include_partial' declared here
                          bool include_partial)
                               ^
3250 warnings generated.
Suppressed 3248 warnings (3248 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.
2 warnings treated as errors
[393/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/receive.cpp.o
[394/537] Linking CXX static library src/zmq/libzmq.a
[395/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/signmessage.cpp.o
[396/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[397/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/util.cpp.o
[398/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/encrypt.cpp.o
[399/537] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[400/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/transaction.cpp.o
[401/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[402/537] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[403/537] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[404/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o
[405/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[406/537] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[407/537] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[408/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[409/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o
[410/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[411/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[412/537] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1

Fabien planned changes to this revision.Jun 9 2023, 05:51

Rebase

PiRK accepted this revision.Jun 9 2023, 06:27

This revision is now accepted and ready to land.Jun 9 2023, 06:27

Harbormaster completed remote builds in B23965: Diff 40683.Jun 9 2023, 06:50

Closed by commit rABC5726657eaaf7: Rewrite OutputGroups to be clearer and to use scriptPubKeys (authored by Andrew Chow <achow101-github@achow101.com>, committed by Fabien). · Explain WhyJun 9 2023, 13:24

This revision was automatically updated to reflect the committed changes.

Fabien added a commit: rABC5726657eaaf7: Rewrite OutputGroups to be clearer and to use scriptPubKeys.