wallet: Avoid recursive lock in IsTrusted#19773
Merged
meshcollider merged 2 commits intobitcoin:masterfrom Aug 31, 2020
Merged
Conversation
promag
force-pushed
the
2020-08-wallet-istrusted
branch
from
cd9bb59 to
0e71022
Compare
promag
changed the title
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
ryanofsky
approved these changes
Aug 24, 2020
Contributor
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 0e71022306ca06f3f5df7b45a725a183b9409518. Looks good, thanks for following up on previous comments. Good to see this locking become more straightforward
promag
force-pushed
the
2020-08-wallet-istrusted
branch
from
0e71022 to
772ea48
Compare
Contributor
Author
|
Rebased after #19289 merge. |
Contributor
Author
Member
|
Concept ACK. |
35 tasks
hebasto
approved these changes
Aug 30, 2020
Comment on lines
+1973
to
+1975
| std::set<uint256> trusted_parents; | ||
| LOCK(pwallet->cs_wallet); | ||
| return pwallet->IsTrusted(*this, trusted_parents); |
Member
There was a problem hiding this comment.
Suggested change
| std::set<uint256> trusted_parents; | |
| LOCK(pwallet->cs_wallet); | |
| return pwallet->IsTrusted(*this, trusted_parents); | |
| AssertLockNotHeld(pwallet->cs_wallet); | |
| std::set<uint256> trusted_parents; | |
| LOCK(pwallet->cs_wallet); | |
| return pwallet->IsTrusted(*this, trusted_parents); |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 31, 2020
772ea48 wallet: Avoid recursive lock in IsTrusted (João Barbosa) 819f10f wallet, refactor: Immutable CWalletTx::pwallet (João Barbosa) Pull request description: 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. ACKs for top commit: meshcollider: utACK 772ea48 hebasto: ACK 772ea48, reviewed and tested on Linux Mint 20 (x86_64). Tree-SHA512: 702ffd928b2f42a8b90de398790649a5fd04e1ac3877558da928e94cdeb19134883f06c3a73a6826c11c912facf199173375a70200737e164ccaea1bec515b2a
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Sep 14, 2021
Summary: PR description: > 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. This is a backport of [[bitcoin/bitcoin#19773 | core#19773]] [1/2] bitcoin/bitcoin@819f10f Depends on D10097 Test Plan: With TSAN: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10099
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Sep 14, 2021
Summary: This is a backport of [[bitcoin/bitcoin#19773 | core#19773]] [2/2] bitcoin/bitcoin@772ea48 Backport note: `checkFinalTx` was replaced by `ContextualCheckTransactionForCurrentBlock` in D5804 Test Plan: With TSAN: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10100
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change moves
CWalletTx::IsTrustedtoCWalletin order to have TSAN. So nowCWallet::IsTrustedrequirescs_walletand the recursive lock no longer happens.Motivated by https://github.com/bitcoin/bitcoin/pull/19289/files#r473308226.