wallet: Avoid recursive lock in IsTrusted #19773

pull promag wants to merge 2 commits into bitcoin:master from promag:2020-08-wallet-istrusted changing 2 files +18 −17

promag commented at 12:10 AM on August 21, 2020: member

This change moves CWalletTx::IsTrusted to CWallet in order to have TSAN. So now CWallet::IsTrusted requires cs_wallet and the recursive lock no longer happens.

Motivated by https://github.com/bitcoin/bitcoin/pull/19289/files#r473308226.
promag force-pushed on Aug 21, 2020
promag renamed this:
~~2020 08 wallet istrusted~~
wallet: Avoid recursive lock in IsTrusted
on Aug 21, 2020
fanquake added the label Wallet on Aug 21, 2020
DrahtBot commented at 5:57 AM on August 21, 2020: member



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



Conflicts

No conflicts as of last run.
ryanofsky approved
ryanofsky commented at 10:49 PM on August 24, 2020: member

Code review ACK 0e71022306ca06f3f5df7b45a725a183b9409518. Looks good, thanks for following up on previous comments. Good to see this locking become more straightforward
wallet, refactor: Immutable CWalletTx::pwallet 819f10f671
wallet: Avoid recursive lock in IsTrusted 772ea4844c
promag force-pushed on Aug 28, 2020
promag commented at 9:42 AM on August 28, 2020: member

Rebased after #19289 merge.
promag commented at 9:43 AM on August 28, 2020: member

@meshcollider
meshcollider commented at 11:24 AM on August 28, 2020: contributor

utACK 772ea4844c34ad70d02fd0bd6c0945baa8fff85c
hebasto commented at 7:59 AM on August 30, 2020: member

Concept ACK.

in src/wallet/wallet.cpp:1975 in 772ea4844c

1969 | @@ -1970,37 +1970,38 @@ bool CWalletTx::InMempool() const
1970 |  
1971 |  bool CWalletTx::IsTrusted() const
1972 |  {
1973 | -    std::set<uint256> s;
1974 | -    return IsTrusted(s);
1975 | +    std::set<uint256> trusted_parents;
1976 | +    LOCK(pwallet->cs_wallet);
1977 | +    return pwallet->IsTrusted(*this, trusted_parents);

hebasto commented at 8:27 AM on August 30, 2020:

772ea4844c34ad70d02fd0bd6c0945baa8fff85c

    AssertLockNotHeld(pwallet->cs_wallet);
    std::set<uint256> trusted_parents;
    LOCK(pwallet->cs_wallet);
    return pwallet->IsTrusted(*this, trusted_parents);

hebasto approved
hebasto commented at 8:31 AM on August 30, 2020: member

ACK 772ea4844c34ad70d02fd0bd6c0945baa8fff85c, reviewed and tested on Linux Mint 20 (x86_64).
meshcollider merged this on Aug 31, 2020
meshcollider closed this on Aug 31, 2020
sidhujag referenced this in commit f6b2c75b81 on Aug 31, 2020
Fabcien referenced this in commit 819580a50c on Sep 14, 2021
Fabcien referenced this in commit aca02ec56a on Sep 14, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#19289 wallet: GetWalletTx and IsMine require cs_wallet lock #19303 Replace all of the RecursiveMutex instances with the Mutex ones

wallet: Avoid recursive lock in IsTrusted #19773

Conflicts