Skip to content

Commit 629e250

Browse files
author
MacroFake
committed
Merge bitcoin/bitcoin#25148: refactor: Remove NO_THREAD_SAFETY_ANALYSIS from non-test/benchmarking code
a55db4e Add more proper thread safety annotations (Hennadii Stepanov) 8cfe93e Add proper thread safety annotation to `CWallet::GetTxConflicts()` (Hennadii Stepanov) ca446f2 Add proper thread safety annotation to `CachedTxGetAvailableCredit()` (Hennadii Stepanov) Pull request description: In non-test/benchmarking code, there are three cases of the `NO_THREAD_SAFETY_ANALYSIS` annotation which are accompanied with `TODO` comments. This PR adds proper thread safety annotations instead of `NO_THREAD_SAFETY_ANALYSIS`. ACKs for top commit: laanwj: Code review ACK a55db4e Tree-SHA512: 806d72eebc1edf088bfa435c8cd11465be0de6789798dd92abd008425516768acb864a73d834a49d412bb10f7fccfb47473f998cb72739dab6caeef6bcfaf191
2 parents 139f789 + a55db4e commit 629e250

File tree

6 files changed

+44
-33
lines changed

6 files changed

+44
-33
lines changed

‎src/wallet/interfaces.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,10 @@ WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx)
8080

8181
//! Construct wallet tx status struct.
8282
WalletTxStatus MakeWalletTxStatus(const CWallet& wallet, const CWalletTx& wtx)
83+
EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
8384
{
85+
AssertLockHeld(wallet.cs_wallet);
86+
8487
WalletTxStatus result;
8588
result.block_height =
8689
wtx.state<TxStateConfirmed>() ? wtx.state<TxStateConfirmed>()->confirmed_block_height :

‎src/wallet/receive.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,8 @@ static CAmount GetCachableAmount(const CWallet& wallet, const CWalletTx& wtx, CW
123123

124124
CAmount CachedTxGetCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter)
125125
{
126+
AssertLockHeld(wallet.cs_wallet);
127+
126128
// Must wait until coinbase is safely deep enough in the chain before valuing it
127129
if (wallet.IsTxImmatureCoinBase(wtx))
128130
return 0;
@@ -164,6 +166,8 @@ CAmount CachedTxGetChange(const CWallet& wallet, const CWalletTx& wtx)
164166

165167
CAmount CachedTxGetImmatureCredit(const CWallet& wallet, const CWalletTx& wtx, bool fUseCache)
166168
{
169+
AssertLockHeld(wallet.cs_wallet);
170+
167171
if (wallet.IsTxImmatureCoinBase(wtx) && wallet.IsTxInMainChain(wtx)) {
168172
return GetCachableAmount(wallet, wtx, CWalletTx::IMMATURE_CREDIT, ISMINE_SPENDABLE, !fUseCache);
169173
}
@@ -173,6 +177,8 @@ CAmount CachedTxGetImmatureCredit(const CWallet& wallet, const CWalletTx& wtx, b
173177

174178
CAmount CachedTxGetImmatureWatchOnlyCredit(const CWallet& wallet, const CWalletTx& wtx, const bool fUseCache)
175179
{
180+
AssertLockHeld(wallet.cs_wallet);
181+
176182
if (wallet.IsTxImmatureCoinBase(wtx) && wallet.IsTxInMainChain(wtx)) {
177183
return GetCachableAmount(wallet, wtx, CWalletTx::IMMATURE_CREDIT, ISMINE_WATCH_ONLY, !fUseCache);
178184
}
@@ -182,6 +188,8 @@ CAmount CachedTxGetImmatureWatchOnlyCredit(const CWallet& wallet, const CWalletT
182188

183189
CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx, bool fUseCache, const isminefilter& filter)
184190
{
191+
AssertLockHeld(wallet.cs_wallet);
192+
185193
// Avoid caching ismine for NO or ALL cases (could remove this check and simplify in the future).
186194
bool allow_cache = (filter & ISMINE_ALL) && (filter & ISMINE_ALL) != ISMINE_ALL;
187195

‎src/wallet/receive.h

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,17 +24,17 @@ bool OutputIsChange(const CWallet& wallet, const CTxOut& txout) EXCLUSIVE_LOCKS_
2424
CAmount OutputGetChange(const CWallet& wallet, const CTxOut& txout) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
2525
CAmount TxGetChange(const CWallet& wallet, const CTransaction& tx);
2626

27-
CAmount CachedTxGetCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter);
27+
CAmount CachedTxGetCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter)
28+
EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
2829
//! filter decides which addresses will count towards the debit
2930
CAmount CachedTxGetDebit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter);
3031
CAmount CachedTxGetChange(const CWallet& wallet, const CWalletTx& wtx);
31-
CAmount CachedTxGetImmatureCredit(const CWallet& wallet, const CWalletTx& wtx, bool fUseCache = true);
32-
CAmount CachedTxGetImmatureWatchOnlyCredit(const CWallet& wallet, const CWalletTx& wtx, const bool fUseCache = true);
33-
// TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct
34-
// annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The
35-
// annotation "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid
36-
// having to resolve the issue of member access into incomplete type CWallet.
37-
CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx, bool fUseCache = true, const isminefilter& filter = ISMINE_SPENDABLE) NO_THREAD_SAFETY_ANALYSIS;
32+
CAmount CachedTxGetImmatureCredit(const CWallet& wallet, const CWalletTx& wtx, bool fUseCache = true)
33+
EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
34+
CAmount CachedTxGetImmatureWatchOnlyCredit(const CWallet& wallet, const CWalletTx& wtx, const bool fUseCache = true)
35+
EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
36+
CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx, bool fUseCache = true, const isminefilter& filter = ISMINE_SPENDABLE)
37+
EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
3838
struct COutputEntry
3939
{
4040
CTxDestination destination;

‎src/wallet/rpc/transactions.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ using interfaces::FoundBlock;
1515

1616
namespace wallet {
1717
static void WalletTxToJSON(const CWallet& wallet, const CWalletTx& wtx, UniValue& entry)
18+
EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
1819
{
1920
interfaces::Chain& chain = wallet.chain();
2021
int confirms = wallet.GetTxDepthInMainChain(wtx);

‎src/wallet/wallet.cpp

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1841,6 +1841,8 @@ void CWallet::ReacceptWalletTransactions()
18411841

18421842
bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string, bool relay) const
18431843
{
1844+
AssertLockHeld(cs_wallet);
1845+
18441846
// Can't relay if wallet is not broadcasting
18451847
if (!GetBroadcastTransactions()) return false;
18461848
// Don't relay abandoned transactions
@@ -1869,12 +1871,11 @@ bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string
18691871

18701872
std::set<uint256> CWallet::GetTxConflicts(const CWalletTx& wtx) const
18711873
{
1872-
std::set<uint256> result;
1873-
{
1874-
uint256 myHash = wtx.GetHash();
1875-
result = GetConflicts(myHash);
1876-
result.erase(myHash);
1877-
}
1874+
AssertLockHeld(cs_wallet);
1875+
1876+
const uint256 myHash{wtx.GetHash()};
1877+
std::set<uint256> result{GetConflicts(myHash)};
1878+
result.erase(myHash);
18781879
return result;
18791880
}
18801881

@@ -3128,15 +3129,20 @@ int CWallet::GetTxDepthInMainChain(const CWalletTx& wtx) const
31283129

31293130
int CWallet::GetTxBlocksToMaturity(const CWalletTx& wtx) const
31303131
{
3131-
if (!wtx.IsCoinBase())
3132+
AssertLockHeld(cs_wallet);
3133+
3134+
if (!wtx.IsCoinBase()) {
31323135
return 0;
3136+
}
31333137
int chain_depth = GetTxDepthInMainChain(wtx);
31343138
assert(chain_depth >= 0); // coinbase tx should not be conflicted
31353139
return std::max(0, (COINBASE_MATURITY+1) - chain_depth);
31363140
}
31373141

31383142
bool CWallet::IsTxImmatureCoinBase(const CWalletTx& wtx) const
31393143
{
3144+
AssertLockHeld(cs_wallet);
3145+
31403146
// note GetBlocksToMaturity is 0 for non-coinbase tx
31413147
return GetTxBlocksToMaturity(wtx) > 0;
31423148
}

‎src/wallet/wallet.h

Lines changed: 11 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -415,36 +415,28 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
415415

416416
const CWalletTx* GetWalletTx(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
417417

418-
// TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct
419-
// annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The annotation
420-
// "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid having to
421-
// resolve the issue of member access into incomplete type CWallet. Note
422-
// that we still have the runtime check "AssertLockHeld(pwallet->cs_wallet)"
423-
// in place.
424-
std::set<uint256> GetTxConflicts(const CWalletTx& wtx) const NO_THREAD_SAFETY_ANALYSIS;
418+
std::set<uint256> GetTxConflicts(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
425419

426420
/**
427421
* Return depth of transaction in blockchain:
428422
* <0 : conflicts with a transaction this deep in the blockchain
429423
* 0 : in memory pool, waiting to be included in a block
430424
* >=1 : this many blocks deep in the main chain
431425
*/
432-
// TODO: Remove "NO_THREAD_SAFETY_ANALYSIS" and replace it with the correct
433-
// annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The annotation
434-
// "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid having to
435-
// resolve the issue of member access into incomplete type CWallet. Note
436-
// that we still have the runtime check "AssertLockHeld(pwallet->cs_wallet)"
437-
// in place.
438-
int GetTxDepthInMainChain(const CWalletTx& wtx) const NO_THREAD_SAFETY_ANALYSIS;
439-
bool IsTxInMainChain(const CWalletTx& wtx) const { return GetTxDepthInMainChain(wtx) > 0; }
426+
int GetTxDepthInMainChain(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
427+
bool IsTxInMainChain(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)
428+
{
429+
AssertLockHeld(cs_wallet);
430+
return GetTxDepthInMainChain(wtx) > 0;
431+
}
440432

441433
/**
442434
* @return number of blocks to maturity for this transaction:
443435
* 0 : is not a coinbase transaction, or is a mature coinbase transaction
444436
* >0 : is a coinbase transaction which matures in this many blocks
445437
*/
446-
int GetTxBlocksToMaturity(const CWalletTx& wtx) const;
447-
bool IsTxImmatureCoinBase(const CWalletTx& wtx) const;
438+
int GetTxBlocksToMaturity(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
439+
bool IsTxImmatureCoinBase(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
448440

449441
//! check whether we support the named feature
450442
bool CanSupportFeature(enum WalletFeature wf) const override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); return IsFeatureSupported(nWalletVersion, wf); }
@@ -584,7 +576,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
584576
void CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm);
585577

586578
/** Pass this transaction to node for mempool insertion and relay to peers if flag set to true */
587-
bool SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string, bool relay) const;
579+
bool SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string, bool relay) const
580+
EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
588581

589582
bool DummySignTx(CMutableTransaction &txNew, const std::set<CTxOut> &txouts, const CCoinControl* coin_control = nullptr) const
590583
{

0 commit comments

Comments
 (0)