Page MenuHomePhabricator
Differential D14730

wallet: Avoid locking cs_wallet recursively
ClosedPublic
Actions

Authored by PiRK on Nov 3 2023, 08:07.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC0cb1d1901233: wallet: Avoid locking cs_wallet recursively
Summary

wallet: GetLabelAddresses requires cs_wallet lock

No change in behavior, the lock is already held at call sites.

wallet: AddWalletDescriptor requires cs_wallet lock

No change in behavior, the lock is already held at call sites.

Also remove getLabelAddresses in wallet interface. This was added in D2886, but Bitcoin Core never had this function and it compiles fine without it.

This is a backport of core#19833

Test Plan

debug and tsan build
ninja all check check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK created this revision.Nov 3 2023, 08:07
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 3 2023, 08:07
teamcity edited the summary of this revision. (Show Details)Nov 3 2023, 08:07
PiRK added a comment.Nov 3 2023, 08:07

@bot build-debug build-tsan

teamcity awarded a token.Nov 3 2023, 08:07
Harbormaster completed remote builds in B25522: Diff 42926.Nov 3 2023, 08:09
teamcity added a comment.Nov 3 2023, 08:09
Build Bitcoin ABC Diffs / Diff Testing (build-clang) failed.

Tail of the build log:

[373/543] Building CXX object src/test/CMakeFiles/testutil.dir/util/validation.cpp.o
[374/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[375/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[376/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[377/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[378/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[379/543] 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
[380/543] Linking C executable src/secp256k1/recover-bench
[381/543] Linking C executable src/secp256k1/ecmult-bench
[382/543] Linking C executable src/secp256k1/verify-bench
[383/543] Linking C executable src/secp256k1/sign-bench
[384/543] Linking C executable src/secp256k1/internal-bench
[385/543] Linking CXX static library src/libcommon.a
[386/543] Linking CXX static library src/libscript.a
[387/543] Linking CXX static library src/libbitcoinconsensus.a
[388/543] Linking CXX shared library src/libbitcoinconsensus.so.0.28.2
[389/543] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[390/543] Linking CXX executable src/bitcoin-cli
[391/543] Linking CXX executable src/bitcoin-tx
[392/543] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[393/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[394/543] Building CXX object src/CMakeFiles/bitcoin-wallet.dir/bitcoin-wallet.cpp.o
[395/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o 
/usr/bin/ccache /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 -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 -pthread -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o -c ../../src/wallet/interfaces.cpp
../../src/wallet/interfaces.cpp:140:30: error: calling function 'GetLabelAddresses' requires holding mutex 'm_wallet->cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
            return m_wallet->GetLabelAddresses(label);
                             ^
1 error generated.
[396/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[397/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/transaction.cpp.o
[398/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[399/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[400/543] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[401/543] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[402/543] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[403/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/util.cpp.o
[404/543] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[405/543] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[406/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/signmessage.cpp.o
[407/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[408/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/receive.cpp.o
[409/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/encrypt.cpp.o
[410/543] Building CXX object src/CMakeFiles/server.dir/wallet/init.cpp.o
[411/543] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[412/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[413/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/spend.cpp.o
[414/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o
[415/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[416/543] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[417/543] 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.Nov 3 2023, 08:12
Build Bitcoin ABC Diffs / Diff Testing (build-tsan) failed.

Tail of the build log:

[357/542] Linking CXX static library src/libbitcoinconsensus.a
[358/542] Linking CXX shared library src/libbitcoinconsensus.so.0.28.2
[359/542] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[360/542] Building CXX object src/CMakeFiles/server.dir/torcontrol.cpp.o
[361/542] Linking C executable src/secp256k1/recover-bench
[362/542] Linking C executable src/secp256k1/verify-bench
[363/542] Linking C executable src/secp256k1/sign-bench
[364/542] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockindex.cpp.o
[365/542] Building CXX object src/CMakeFiles/bitcoin-cli.dir/bitcoin-cli.cpp.o
[366/542] Linking CXX executable src/bitcoin-cli
[367/542] Building C object src/secp256k1/CMakeFiles/ecmult-bench.dir/src/bench_ecmult.c.o
[368/542] Building C object src/secp256k1/CMakeFiles/internal-bench.dir/src/bench_internal.c.o
[369/542] Linking C executable src/secp256k1/ecmult-bench
[370/542] Linking C executable src/secp256k1/internal-bench
[371/542] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[372/542] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[373/542] Building CXX object src/CMakeFiles/server.dir/txmempool.cpp.o
[374/542] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[375/542] Building CXX object src/CMakeFiles/server.dir/wallet/init.cpp.o
[376/542] Building CXX object src/CMakeFiles/server.dir/rpc/rawtransaction.cpp.o
[377/542] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[378/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[379/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[380/542] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[381/542] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[382/542] Building CXX object src/CMakeFiles/server.dir/net_processing.cpp.o
[383/542] Linking CXX executable src/bitcoin-tx
[384/542] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[385/542] Building CXX object src/test/CMakeFiles/testutil.dir/util/validation.cpp.o
[386/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[387/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[388/542] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[389/542] Building CXX object src/CMakeFiles/server.dir/rpc/blockchain.cpp.o
[390/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[391/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[392/542] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[393/542] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[394/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[395/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o 
/usr/bin/ccache /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/miniupnpc -Werror -g -O2 -fPIC -fvisibility=hidden -fsanitize=thread -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 -pthread -std=gnu++17 -MD -MT src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o -c ../../src/wallet/interfaces.cpp
../../src/wallet/interfaces.cpp:140:30: error: calling function 'GetLabelAddresses' requires holding mutex 'm_wallet->cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
            return m_wallet->GetLabelAddresses(label);
                             ^
1 error generated.
[396/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[397/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/receive.cpp.o
[398/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/transaction.cpp.o
[399/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[400/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/util.cpp.o
[401/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/signmessage.cpp.o
[402/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/encrypt.cpp.o
[403/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[404/542] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[405/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/spend.cpp.o
[406/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[407/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o
[408/542] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
ninja: build stopped: subcommand failed.
Build build-tsan failed with exit code 1
PiRK updated this revision to Diff 42927.Nov 3 2023, 09:03
PiRK edited the summary of this revision. (Show Details)
PiRK edited the test plan for this revision. (Show Details)

removing unneeded function from wallet interface

PiRK added a comment.Nov 3 2023, 09:03

@bot build-debug build-tsan

teamcity updated a token.Nov 3 2023, 09:03
PiRK published this revision for review.Nov 3 2023, 09:20
Harbormaster completed remote builds in B25523: Diff 42927.Nov 3 2023, 09:22
Fabien accepted this revision.Nov 3 2023, 13:57
This revision is now accepted and ready to land.Nov 3 2023, 13:57
Closed by commit rABC0cb1d1901233: wallet: Avoid locking cs_wallet recursively (authored by João Barbosa <joao.paulo.barbosa@gmail.com>, committed by PiRK). · Explain WhyNov 3 2023, 14:40
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABC0cb1d1901233: wallet: Avoid locking cs_wallet recursively.

Revision Contents
Changeset List

PathSize
src/
interfaces/
4 lines
wallet/
4 lines
6 lines
5 lines

Diff 42935

View Options

src/interfaces/wallet.h

Loading...
View Options

src/wallet/interfaces.cpp

Loading...
View Options

src/wallet/wallet.h

Loading...
View Options

src/wallet/wallet.cpp

Loading...