qt: Remove redundant locks #11733

practicalswift · 2017-11-20T12:52:04Z

Remove redundant locks:

~~FindNode(...) is locking cs_vNodes internally~~
SetAddressBook(...) is locking cs_wallet internally
DelAddressBook(...) is locking cs_wallet internally

Note to reviewers: From what I can tell these locks are redundantly held from a data integrity perspective (guarding specific variables), and they do not appear to be needed from a data consistency perspective (ensuring a consistent state at the right points). Review thoroughly and please let me know if I'm mistaken :-)

fanquake · 2017-11-20T13:28:01Z

The changes to net.cpp remove 2/3 LOCKs introduced in 3c37dc4, part of #9626.

"This also ensures that if we return a CNode* from FindNode, we are still holding cs_vNodes if we use it for anything aside from existance-checking, fixing a stupid-unlikely race where it might be deleted out from under us."

practicalswift · 2017-11-20T13:52:28Z

@fanquake Thanks for reviewing. Good point. Would it be possible to state that locking requirement more explicitly using annotations (see #11226)? Perhaps one way to do it would be if the two uses were separated: CNode* FindNode(…) would require holding cs_vNodes (which would be clearly stated using EXCLUSIVE_LOCKS_REQUIRED(cs_vNodes)) whereas say bool NodeExists(…) wouldn't? Worth doing?

promag · 2017-11-20T15:35:17Z

src/net.cpp

@@ -399,7 +399,6 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
            // In that case, drop the connection that was just created, and return the existing CNode instead.
            // Also store the name we used to connect in that CNode, so that future FindNode() calls to that
            // name catch this early.
-            LOCK(cs_vNodes);


I think this is necessary. cc @theuni

Yes, see @fanquake's comment above regarding 3c37dc4 :-)

Alright 😄

promag · 2017-11-20T15:36:23Z

src/net.cpp

@@ -2552,7 +2551,6 @@ void CConnman::GetNodeStats(std::vector<CNodeStats>& vstats)

 bool CConnman::DisconnectNode(const std::string& strNode)
 {
-    LOCK(cs_vNodes);


Same as above.

* SetAddressBook(...) is locking cs_wallet internally * DelAddressBook(...) is locking cs_wallet internally

practicalswift · 2017-11-21T09:18:43Z

@fanquake @promag Please re-review :-)

practicalswift · 2018-01-31T12:55:36Z

Anyone willing to review? :-)

maflcko · 2018-01-31T14:45:27Z

Any measurable speedup by removing those? Just asking.

practicalswift · 2018-01-31T15:06:21Z

@MarcoFalke I would assume no. My goal with this PR was to achieve correct locking, so any measurable speed-up would be an unintended bonus :-)

ryanofsky

utACK d6f3a73. This could help performance since it releases the lock while writing to disk. Anyway it's an obvious improvement for code simplicity & consistency.

laanwj · 2018-02-14T16:26:45Z

utACK d6f3a73, indeed makes no sense to do this locking internally as well as externally, and having the locking contained internally is better.

d6f3a73 Remove redundant locks (practicalswift) Pull request description: Remove redundant locks: * ~~`FindNode(...)` is locking `cs_vNodes` internally~~ * `SetAddressBook(...)` is locking `cs_wallet` internally * `DelAddressBook(...)` is locking `cs_wallet` internally **Note to reviewers:** From what I can tell these locks are redundantly held from a data integrity perspective (guarding specific variables), and they do not appear to be needed from a data consistency perspective (ensuring a consistent state at the right points). Review thoroughly and please let me know if I'm mistaken :-) Tree-SHA512: 7e3ca2d52fecb16385dc65051b5b20d81b502c0025d70b0c489eb3881866bdd57947a9c96931f7b213f5a8a76b6d2c7b084dff0ef2028a1e9ca9ccfd83e5b91e

Summary: * SetAddressBook(...) is locking cs_wallet internally * DelAddressBook(...) is locking cs_wallet internally Backport of PR11733 bitcoin/bitcoin#11733 Test Plan: make check test_runner.py Reviewers: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2700

@luke-jr

* test: Replace remaining sprintf with snprintf Summary: Use of `sprintf` is seen as a red flag as many of its uses are insecure. OpenBSD warns about it while compiling, and some modern platforms, e.g. [cloudlibc from cloudabi](https://github.com/NuxiNL/cloudlibc) don't even provide it anymore. Backport of core PR9867 Test Plan: make check Reviewers: #bitcoin_abc, deadalnix, jasonbcox Reviewed By: #bitcoin_abc, deadalnix, jasonbcox Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2697 * Merge #10783: [RPC] Various rpc argument fixes Summary: 4dc1915 check for null values in rpc args and handle appropriately (Gregory Sanders) 999ef20 importmulti options are optional (Gregory Sanders) a70d025 fixup some rpc param counting for rpc help (Gregory Sanders) Pull request description: Audited where named args will fail to use correct default values or may fail when additional optional arguments are added. Previously for these parameters, it was fine to omit them as positional arguments, but it would trigger UniValue runtime errors to set them to null, or to omit them while passing named parameters with greater positions (which would internally set earlier missing arguments to null). Now null values are treated the same as missing values so these errors do not occur. Included a few other small fixes while working on it. I didn't bother fixing account-based rpc calls. Tree-SHA512: 8baf781a35bd48de7878d4726850a580dab80323d3416c1c146b4fa9062f8a233c03f37e8ae3f3159e9d04a8f39c326627ca64c14e1cb7ce72538f934ab2ae1e Backport of Core PR 10783 https://github.com/bitcoin/bitcoin/pull/10783/files Completes T552 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2688 * Merge #10775: nCheckDepth chain height fix Summary: d9d1bd3 nCheckDepth chain height fix (romanornr) Pull request description: ```` if (nCheckDepth <= 0) nCheckDepth = 1000000000; // suffices until the year 19000 if (nCheckDepth > chainActive.Height()) nCheckDepth = chainActive.Height(); ```` These lines confuse me. Correct me if I am wrong, but we can't check any more blocks than we have right? If someone requests <= 0 it get set it into some huge number and then immediately limit it to the chain height in the following statement. ```` if (nCheckDepth > chainActive.Height()) nCheckDepth = chainActive.Height(); ```` when using ````--checkblocks=Z```` When Z is ````0```` or any other negative number, it will check all blocks. I think it should be changed to this maybe. ```` if (nCheckDepth <= 0 || nCheckDepth > chainActive.Height()) nCheckDepth = chainActive.Height(); ```` Which gets rid of that huge number which is confusing for any other altcoins that have a different block time. Tree-SHA512: 8ee0ae5f33b399fa74dc16926709694ccfe1fc8a043cba2f5d00884220ac1b9b13f2df4588041f4133be634e5c7b14f4eebe24294028dafe91581a97dbe627f3 Backport of Core PR 10775 https://github.com/bitcoin/bitcoin/pull/10775/files Test Plan: ninja check test_runner.py Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2691 * Merge #9622: [rpc] listsinceblock should include lost transactions when parameter is a reorg'd block Summary: 876e92b Testing: listsinceblock should display all transactions that were affected since the given block, including transactions that were removed due to a reorg. (Karl-Johan Alm) f999c46 listsinceblock: optionally find and list any transactions that were undone due to reorg when requesting a non-main chain block in a new 'removed' array. (Karl-Johan Alm) Pull request description: The following scenario will not notify the caller of the fact `tx0` has been dropped: 1. User 1 receives BTC in tx0 from utxo1 in block aa1. 2. User 2 receives BTC in tx1 from utxo1 (same) in block bb1 3. User 1 sees 2 confirmations at block aa3. 4. Reorg into bb chain. 5. User 1 asks `listsinceblock aa3` and does not see that tx0 is now invalidated. See `listsinceblock.py` commit for related test. The proposed fix is to iterate from the given block down to the fork point, and to check each transaction in the blocks against the wallet, in addition to including all transactions from the fork point to the active chain tip (the current behavior). Any transactions that were present will now also be listed in the `listsinceblock` output in a new `replaced` array. This operation may be a bit heavy but the circumstances (and perceived frequency of occurrence) warrant it, I believe. Example output: ```Python { 'transactions': [], 'replaced': [ { 'walletconflicts': [], 'vout': 1, 'account': '', 'timereceived': 1485234857, 'time': 1485234857, 'amount': '1.00000000', 'bip125-replaceable': 'unknown', 'trusted': False, 'category': 'receive', 'txid': 'ce673859a30dee1d2ebdb3c05f2eea7b1da54baf68f93bb8bfe37c5f09ed22ff', 'address': 'miqEt4kWp9zSizwGGuUWLAmxEcTW9bFUnQ', 'label': '', 'confirmations': -7 } ], 'lastblock': '7a388f27d09e3699102a4ebf81597d974fc4c72093eeaa02adffbbf7527f6715' } ``` I believe this addresses the comment by @luke-jr in bitcoin/bitcoin#9516 (comment) but I could be wrong.. Tree-SHA512: 607b5dcaeccb9dc0d963d3de138c40490f3e923050b29821e6bd513d26beb587bddc748fbb194503fe618cfe34a6ed65d95e8d9c5764a882b6c5f976520cff35 Backport of Core PR 9622 https://github.com/bitcoin/bitcoin/pull/9622/files Completes T551 Test Plan: make check test_runner.py wallet_listsinceblock Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2689 * Merge #11039: Avoid second mapWallet lookup Summary: 8f2f1e0 wallet: Avoid second mapWallet lookup (João Barbosa) Pull request description: All calls to `mapWallet.count()` have the intent to detect if a `txid` exists and most are followed by a second lookup to retrieve the `CWalletTx`. This PR replaces all `mapWallet.count()` calls with `mapWallet.find()` to avoid the second lookup. Tree-SHA512: 96b7de7f5520ebf789a1aec1949a4e9c74e13683869cee012f717e5be8e51097d068e2347a36e89097c9a89f1ed1a1529db71760dac9b572e36a3e9ac1155f29 Backport of Core PR 11039 https://github.com/bitcoin/bitcoin/pull/11039/files Completes T550 Depends on D2689 Test Plan: ninja check test_runner.py Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2690 * Merge #11027: [RPC] Only return hex field once in getrawtransaction Summary: 6bbdafc Pass serialization flags and whether to include hex to TxToUniv (Andrew Chow) e029c6e Only return hex field once in getrawtransaction (Andrew Chow) Pull request description: The hex is already returned in `TxToUniv()`, no need to give it out a second time in getrawtransaction itself. Tree-SHA512: 270289f2d6dea37f51f5a42db3dae5debdbe83c6b504fccfd3391588da986ed474592c6655d522dc51022d4b08fa90ed1ebb249afe036309f95adfe3652cb262 Backport of Core PR 11027 bitcoin/bitcoin#11027 Completes T564 Test Plan: ninja check test_runner.py Run `bitcoin-cli <txid> 1` and ensure that there is a single top-level `"hex"` field in the returned JSON. Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2709 * Merge #11565: Make listsinceblock refuse unknown block hash Summary: 659b206 Make listsinceblock refuse unknown block hash (Russell Yanofsky) Pull request description: Change suggested by @theuni who noticed listsinceblock would ignore invalid block hashes causing it to return a completely unfiltered list of transactions. Tree-SHA512: 3c8fb160265780d1334e856e853ab48e2e18372b8f1fc71ae480c3f45317048cc1fee0055d5c58031981a91b9c2bdbeb8e49a889d04ecba61729ce8109f2ce3f Backport of Core PR 11565 https://github.com/bitcoin/bitcoin/pull/11565/files Completes T563 Test Plan: ninja check wallet_listsinceblock.py Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2703 * Merge #11618: rpc: Lock cs_main in blockToJSON/blockheaderToJSON Summary: a9b6ba0b7 Add missing cs_main locks when calling blockToJSON/blockheaderToJSON (practicalswift) Pull request description: `blockToJSON(...)` and `blockheaderToJSON(...)` read the variable `chainActive` which requires holding the mutex `cs_main`. So does `GetDifficulty(...)`. Tree-SHA512: bfb94f5e3238accbf6a4daddde49d53f1891c38ae9b07e25b3098c485747159258f64bb66a50e147b32beac601de89d9d04ff717b6c4f1460d329c90a53d3333 Backport of Core PR 11618 https://github.com/bitcoin/bitcoin/pull/11618/files Test Plan: make check test_runner.py bitcoin-cli getblock 000000000933ea01ad0ee984209779baaec3ced90fa3f408719526f8d77f4943 Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2684 * Remove redundant locks Summary: * SetAddressBook(...) is locking cs_wallet internally * DelAddressBook(...) is locking cs_wallet internally Backport of PR11733 bitcoin/bitcoin#11733 Test Plan: make check test_runner.py Reviewers: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2700 * Merge #12327: [gui] Defer coin control instancing Summary: 6558f8acc [gui] Defer coin control instancing (João Barbosa) Pull request description: Defer the GUI coin control instancing so that argument processing is taken into account for the default coin control values. Fixes #12312 Tree-SHA512: ecda28b94f4709319e9484b01afe763c7c3569097d2afb89db79da8a195c46d20ea77166df7edce0c8ab77627b295def01c072148714503436d27675d5e75d99 Backport of Core PR 12327 https://github.com/bitcoin/bitcoin/pull/12327/files Completes T556 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2693 * [LINTER] Improve check-doc regex Summary: Improve the regex: - Use Perl type regex in grep to remove duplication. Previously there was 2 alsmost identical regex, one posix compliant for egrep and one Perl compliant for python re. - Exclude the test folder from the source path rather than subtracting its matching output This improvements make the `-h` now detectable; it is added to the undocumented exceptions. Inspired from core PR12820. Note that GNU `grep` is used instead of `git grep` to allow for multiline regex (required due to our code format). Test Plan: Should return no error: arc lint ./test/lint/check-doc.py There should be 1 more used argument than before this patch (`-h`), otherwise the output should be identical. Reviewers: #bitcoin_abc, deadalnix, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2694 * [LINTER] Fix check doc incompatibility with BSD grep Summary: BSD grep on OSX and FreeBSD does not provide the same options as GNU grep which prevent using them for multiline search. This diff uses only python regex to avoid this incompatibility. As a bonus, it runs about 3x faster. Depends on D2694 Test Plan: Should return no error: arc lint In `src/init.cpp` delete the line 1279: ``` logger.m_print_to_console = gArgs.GetBoolArg("-printtoconsole", false); ``` Then run `arc lint` and check the linter outputs an Unknown argument error: '-printtoconsole' is documented but not used' Restore the previously deleted line in `src/init.cpp` and now delete the lines 792 to 794: ``` strUsage += HelpMessageOpt( "-printtoconsole", _("Send trace/debug info to console instead of debug.log file")); ``` Then run `arc lint` and check the linter outputs an Undocumented argument error: '-printtoconsole' is undocumented' Reviewers: #bitcoin_abc, deadalnix, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2695 * Backport RetFormat enum class Summary: Partial backport of Core PR 10742 (RetFormat only) https://github.com/bitcoin/bitcoin/pull/10742/files Progess towards T549 Test Plan: ninja check Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2712 * Backport BlockSource enum class Summary: Partial backport of Core PR 10742 (BlockSource only) https://github.com/bitcoin/bitcoin/pull/10742/files Progress towards T549 Test Plan: ninja check Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2713 * Backport HelpMessageMode enum class Summary: Partial backport of Core PR 10742 (HelpMessageMode only) https://github.com/bitcoin/bitcoin/pull/10742/files Progress towards T549 Test Plan: ninja check Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2714 * Merge #8665: Assert all the things! Summary: 4d51e9b Assert ConnectBlock block and pIndex are the same block (NicolasDorier) 972714c pow: GetNextWorkRequired never called with NULL pindexLast (Daniel Cousens) cc44c8f ContextualCheckBlockHeader should never have pindexPrev to NULL (NicolasDorier) Tree-SHA512: 7cc568bf9417267c335f21ec3d1505b26e56e5b3d5f4d3dbb555279489800aaa65a3bcd7bc376e274dd102912aec16ddbb18de2e2060b2667b41eb979cd9321e Backport of Core PR 8665 https://github.com/bitcoin/bitcoin/pull/8665/files Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2722 * Merge #11028: Avoid masking of difficulty adjustment errors by checkpoints Summary: 85c82b5 Avoid masking of difficulty adjustment errors by checkpoints (Pieter Wuille) Pull request description: Currently difficulty adjustment violations are not reported for chains that branch off before the last checkpoint. Change this by moving the checkpoint check after the difficulty check. Tree-SHA512: 33666f2c3459151b28c42041a463779e6df18f61d3dd5b1879a0af4e5b199ef74d1e33e06af68bebfdfb211569ad5fb56556bfebe9d63b5688d910ea211b839a Backport of Core PR 11028 https://github.com/bitcoin/bitcoin/pull/11028/files Depends on D2722 Test Plan: ``` make check test_runner.py ``` Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2723 * Avoid slow transaction search with txindex enabled Summary: Backport of PR11529: bitcoin/bitcoin#11529 Completes T565 Test Plan: make check test_runner.py Reviewers: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2710 * [secp256k1] fix java secp256k1 test Summary: fix java secp256k1 test Test Plan: to run the java secp256k1 tests (including this one): ``` $ cd src/secp256k1 $ ./autogen.sh $ ./configure --enable-jni --enable-experimental --enable-module-ecdh $ make check-java ``` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien, jasonbcox Subscribers: jasonbcox, teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2685 * Backport FlushStateMode enum class Summary: Partial backport of Core PR 10742 (FlushStateMode only) https://github.com/bitcoin/bitcoin/pull/10742/files Progress towards T549 Test Plan: ninja check Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2716 * Allow for running secp256k1 java build/tests out of tree Summary: As per title Test Plan: mkdir -p src/secp256k1/build cd src/secp256k1 mkdir -p src/java/guava wget https://search.maven.org/remotecontent?filepath=com/google/guava/guava/18.0/guava-18.0.jar -O src/java/guava/guava-18.0.jar ./autogen.sh cd build ../configure --enable-jni --enable-experimental --enable-module-ecdh make check-java (`make check-java` may pass or fail depending of D2685) Use `make clean` to remove the .class files generated by `javac`. Reviewers: #bitcoin_abc, deadalnix, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2717 * Backport VerifyResult enum class Summary: Partial backport of Core PR 10742 (VerifyResult only) https://github.com/bitcoin/bitcoin/pull/10742/files Progress towards T549 Test Plan: ninja check Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2718 * Remove unused seeder/compat.h Summary: This file is no longer used in the seeder since it is included in the main tree (src/compat.h is used). Test Plan: ./autogen.sh mkdir build && cd build ../configure make ./src/bitcoin-seeder Ensure the software runs as expected cd .. mkdir buildcmake && cd buildcmake cmake -GNinja .. ninja ./src/bitcoin-seeder Ensure the software runs as expected Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2728 * [secp256k1] remove guava dep Summary: remove overkill depencency to guava Test Plan: execute previous tests successfully the JNI part can be built and tested by simply doing ``` $ ./autogen.sh $ ./configure --enable-jni --enable-experimental --enable-module-ecdh $ make check-java ``` Once https://reviews.bitcoinabc.org/D2686 is passed we can do this automatically Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien Subscribers: Fabien, teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2681 * [secp256k1] remove unused byte array Summary: remove unused bytearray in verify Test Plan: re run tests Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Subscribers: schancel Differential Revision: https://reviews.bitcoinabc.org/D2734 * Fixup merge issue * Remove seeder.h from make

d6f3a73 Remove redundant locks (practicalswift) Pull request description: Remove redundant locks: * ~~`FindNode(...)` is locking `cs_vNodes` internally~~ * `SetAddressBook(...)` is locking `cs_wallet` internally * `DelAddressBook(...)` is locking `cs_wallet` internally **Note to reviewers:** From what I can tell these locks are redundantly held from a data integrity perspective (guarding specific variables), and they do not appear to be needed from a data consistency perspective (ensuring a consistent state at the right points). Review thoroughly and please let me know if I'm mistaken :-) Tree-SHA512: 7e3ca2d52fecb16385dc65051b5b20d81b502c0025d70b0c489eb3881866bdd57947a9c96931f7b213f5a8a76b6d2c7b084dff0ef2028a1e9ca9ccfd83e5b91e

fanquake added the Refactoring label Nov 20, 2017

promag reviewed Nov 20, 2017

View reviewed changes

Remove redundant locks

d6f3a73

* SetAddressBook(...) is locking cs_wallet internally * DelAddressBook(...) is locking cs_wallet internally

practicalswift force-pushed the redundant-locks branch from cf6c726 to d6f3a73 Compare November 21, 2017 09:18

maflcko changed the title ~~Remove redundant locks~~ qt: Remove redundant locks Nov 21, 2017

ryanofsky reviewed Jan 31, 2018

View reviewed changes

laanwj merged commit d6f3a73 into bitcoin:master Feb 14, 2018

practicalswift mentioned this pull request Apr 30, 2018

Meta-issue: Add Clang thread safety analysis annotations #13129

Closed

30 tasks

practicalswift deleted the redundant-locks branch April 10, 2021 19:33

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qt: Remove redundant locks #11733

qt: Remove redundant locks #11733

practicalswift commented Nov 20, 2017 •

edited

fanquake commented Nov 20, 2017

practicalswift commented Nov 20, 2017 •

edited

promag Nov 20, 2017

practicalswift Nov 20, 2017 •

edited

promag Nov 20, 2017

promag Nov 20, 2017

practicalswift commented Nov 21, 2017

practicalswift commented Jan 31, 2018

maflcko commented Jan 31, 2018

practicalswift commented Jan 31, 2018 •

edited

ryanofsky left a comment

laanwj commented Feb 14, 2018

qt: Remove redundant locks #11733

qt: Remove redundant locks #11733

Conversation

practicalswift commented Nov 20, 2017 • edited

fanquake commented Nov 20, 2017

practicalswift commented Nov 20, 2017 • edited

promag Nov 20, 2017

Choose a reason for hiding this comment

practicalswift Nov 20, 2017 • edited

Choose a reason for hiding this comment

promag Nov 20, 2017

Choose a reason for hiding this comment

promag Nov 20, 2017

Choose a reason for hiding this comment

practicalswift commented Nov 21, 2017

practicalswift commented Jan 31, 2018

maflcko commented Jan 31, 2018

practicalswift commented Jan 31, 2018 • edited

ryanofsky left a comment

Choose a reason for hiding this comment

laanwj commented Feb 14, 2018

practicalswift commented Nov 20, 2017 •

edited

practicalswift commented Nov 20, 2017 •

edited

practicalswift Nov 20, 2017 •

edited

practicalswift commented Jan 31, 2018 •

edited