Deadlocks #1168

UdjinM6 · 2016-11-25T16:37:44Z

moving cs_main lock from masternodes to upper lever i.e. mnodeman etc to make sure it's locked first
fixing all kind of other (potential) deadlocks reported in DEBUG_LOCKORDER mode
less locks for some rpc commands
CGovernanceManager::ConfirmInventoryRequest cs deadlock - not really fixed, lock is disabled for now
CGovernanceTriggerManager::AddNewTrigger governance.cs assert fails in Read on Dump, can potentially lead to governance data corruption but chances are low imo - not fixed, no changes

tgflynn

Please see line comments.

Also, the observed deadlock was caused by cs_main and CGovernanceMan::cs being locked out of order in code called from DoFullVerificationStep. I'm not very clear on how this was fixed. I do see AssertLockHeld calls for cs_main and cs in SendVerifyRequest but I didn't see where the locking was changed to ensure this, I'd appreciate it if you could point that out.

tgflynn · 2016-11-25T21:15:20Z

src/governance.cpp

@@ -164,6 +162,8 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, std::string& strCommand, C
            return;
        }

+        LOCK2(cs_main, cs);


I'd rather see CountEnabled() changed to not call Check() than adding all these locks. I think that is the only function we call from these governance functions that locks cs_main.

Same comment for the other governance functions and rpcgovernance.cpp

tgflynn · 2016-11-25T21:22:18Z

src/masternode.cpp

@@ -852,6 +837,7 @@ void CMasternodePing::Relay()

 void CMasternode::AddGovernanceVote(uint256 nGovernanceObjectHash)
 {
+    LOCK(cs);


These shouldn't be needed because these functions can only be called via CMasternodeMan wrapper functions. These accesses are hence protected by CMasternodeMan::cs.

Since it's never safe to access a masternode without locking CMasternodeMan::cs, CMasternode doesn't really need its own cs. But since such unsafe accesses still exist (via Find, GetNextMasternodeInQueueForPayment, etc) I didn't remove it when I added these methods.

Maybe now would be a good time to finally get rid of these unsafe access methods to CMasternodeMan and also remove CMasternode::cs (not saying that necessarily should be done in this PR).

tgflynn · 2016-11-25T21:28:49Z

src/governance.cpp

@@ -542,7 +542,8 @@ void CGovernanceManager::NewBlock()

 bool CGovernanceManager::ConfirmInventoryRequest(const CInv& inv)
 {
-    LOCK(cs);
+    // FIXME:
+    // LOCK(cs);


What's the problem with this ?

This is called only from AlreadyHave() which is called from 2 places in main.cpp. I don't see any locks held there. Should be OK if cs_main is held on entry since that would be the correct order.

tgflynn

This commit looks good but you should remove the Check() calls from CountEnabled(), GetFullMasternodeVector() and probably a few other places.

tgflynn · 2016-11-26T13:12:16Z

Why closed ?

UdjinM6 · 2016-11-26T15:03:57Z

I don't like the mess I created here, will rework

8f96448 qt: Remove Transactionview Edit Label Action (Jarol Rodriguez) Pull request description: This PR removes the `Edit Label` action from the `transactionview` context menu. Since the `Edit Label` action will no longer be utilized in the `transactionview`, the `Edit Label` function logic is also removed. | Master | PR | | ----------- | ----------- | |<img width="248" alt="Screen Shot 2021-02-17 at 8 34 34 PM" src="https://user-images.githubusercontent.com/23396902/108292189-9b86c800-7161-11eb-9e80-6238523bc27e.png">|<img width="248" alt="Screen Shot 2021-02-17 at 8 35 10 PM" src="https://user-images.githubusercontent.com/23396902/108292204-a17ca900-7161-11eb-8582-7f33d3e2ba8f.png">| Among the context menu actions for each transaction in the `transactionview` is the `Edit Label` action. While all other actions apply directly to the selected transaction, the `Edit Label` action applies to the selected transaction's address. As documented in issue dashpay#209 and [dashpay#1168](bitcoin#1168) , this is an "unfortunate" placement for such an action. The current placement creates a confusing UX scenario where the outcome of the action is ambiguous. **Example of Ambiguous Behavior:** The context menu gives the wrong impression that the `Edit Label` action will edit a `Label` for the specific transaction that has been right-clicked on. This impression can be because all other actions in this menu will relate to the specific transaction and the misconception between `Comment` and `Label`. <img width="1062" alt="editlabel-start" src="https://user-images.githubusercontent.com/23396902/108296385-6da48200-7167-11eb-89f0-b21ccc58f6f4.png"> Let's say I wanted to give the transaction selected in the screenshot above a comment of "2-17[17:43]". Given all the context clues, it will be reasonable to assume that the `Edit Label` function will give a label to this transaction. Instead, it edits the `Label` for the address behind this transaction. Thus, changing the `Label` for all transactions associated with this address. <img width="971" alt="editlabel-end" src="https://user-images.githubusercontent.com/23396902/108297179-e35d1d80-7168-11eb-86a9-0d2796c51829.png"> **Maintaining `Edit Label` Functionality:** The action of Editing a Label should instead be reserved for the respective address tables of the `Send` and `Receive` tabs. As documented in this [comment](bitcoin-core/gui#209 (comment)), `Edit Label` is currently implemented in the `Send` tab and is missing in the `Receive` tab. A follow-up PR can add the `Edit Label` functionality to the `Receive` tab. ACKs for top commit: MarcoFalke: review ACK 8f96448 Talkless: tACK 8f96448, tested on Debian Sid. Tree-SHA512: 70bbcc8be3364b0d4f476a9760aa14ad1ad1f53b0b130ce0ffe75190d76c386e6e26c530c0a55d1742402fe2b45c68a2af6dbfaf58ee9909ad93b06f0b6559d4

fix locks

de00f7e

tgflynn reviewed Nov 25, 2016

View reviewed changes

tgflynn reviewed Nov 26, 2016

View reviewed changes

UdjinM6 added 2 commits November 26, 2016 12:29

centralize CMasternode[Man]::Check() calls, remove related cs_main locks

6a8b153

revert CGovernanceManager::ConfirmInventoryRequest lock removal

1975324

UdjinM6 closed this Nov 26, 2016

UdjinM6 mentioned this pull request Nov 27, 2016

Fix (dead)locks #1169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deadlocks #1168

Deadlocks #1168

UdjinM6 commented Nov 25, 2016

tgflynn left a comment

tgflynn Nov 25, 2016

tgflynn Nov 25, 2016

tgflynn Nov 25, 2016

tgflynn left a comment •

edited

Loading

tgflynn commented Nov 26, 2016

UdjinM6 commented Nov 26, 2016

Deadlocks #1168

Deadlocks #1168

Conversation

UdjinM6 commented Nov 25, 2016

tgflynn left a comment

Choose a reason for hiding this comment

tgflynn Nov 25, 2016

Choose a reason for hiding this comment

tgflynn Nov 25, 2016

Choose a reason for hiding this comment

tgflynn Nov 25, 2016

Choose a reason for hiding this comment

tgflynn left a comment • edited Loading

Choose a reason for hiding this comment

tgflynn commented Nov 26, 2016

UdjinM6 commented Nov 26, 2016

tgflynn left a comment •

edited

Loading