Conversation
tgflynn
left a comment
tgflynn
left a comment
There was a problem hiding this comment.
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.
| return; | ||
| } | ||
|
|
||
| LOCK2(cs_main, cs); |
There was a problem hiding this comment.
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
|
|
||
| void CMasternode::AddGovernanceVote(uint256 nGovernanceObjectHash) | ||
| { | ||
| LOCK(cs); |
There was a problem hiding this comment.
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).
src/governance.cpp
Outdated
| { | ||
| LOCK(cs); | ||
| // FIXME: | ||
| // LOCK(cs); |
There was a problem hiding this comment.
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.
|
Why closed ? |
|
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="proxy.php?url=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="proxy.php?url=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="proxy.php?url=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="proxy.php?url=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
![https://user-images.githubusercontent.com/23396902/108292189-9b86c800-7161-11eb-9e80-6238523bc27e.png">|<img](proxy.php?url=https://user-images.githubusercontent.com/23396902/108292189-9b86c800-7161-11eb-9e80-6238523bc27e.png">|<img){kind=link}
![https://user-images.githubusercontent.com/23396902/108292204-a17ca900-7161-11eb-8582-7f33d3e2ba8f.png">|](proxy.php?url=https://user-images.githubusercontent.com/23396902/108292204-a17ca900-7161-11eb-8582-7f33d3e2ba8f.png">|){kind=link}
2 participants
cs_mainlock from masternodes to upper lever i.e. mnodeman etc to make sure it's locked firstDEBUG_LOCKORDERmodeCGovernanceManager::ConfirmInventoryRequestcsdeadlock - not really fixed, lock is disabled for nowCGovernanceTriggerManager::AddNewTriggergovernance.csassert fails inReadonDump, can potentially lead to governance data corruption but chances are low imo - not fixed, no changes