governance: add threading annotations, add default values, adjust constructors by PastaPastaPasta · Pull Request #4465 · dashpay/dash

PastaPastaPasta · 2021-09-26T21:23:51Z

No description provided.

PastaPastaPasta · 2021-09-27T22:24:06Z

I don't understand these test failures... Help wanted :)

src/governance/governance.h

UdjinM6

mac build fails https://gitlab.com/dashpay/dash/-/jobs/1631527804

PastaPastaPasta · 2021-10-01T18:45:23Z

We'll see how the mac build likes that. Pay special attention to 21abbc7 (will try to resolve todo in a near future PR), would like your feedback / ensure I'm not crazy when it comes to livecycles / saftey there

github-actions · 2021-10-01T19:21:41Z

This pull request has conflicts, please rebase.

github-actions · 2021-10-01T19:21:42Z

This pull request has conflicts, please rebase.

PastaPastaPasta · 2021-10-01T19:27:27Z

rebased

PastaPastaPasta · 2021-10-01T19:32:37Z

I figured e80b67f would resolve these compilation errors, but I guess I'm missing something...

github-actions · 2021-10-05T21:28:58Z

This pull request has conflicts, please rebase.

Signed-off-by: pasta <[email protected]>

…dVotes

Signed-off-by: pasta <[email protected]>

UdjinM6

mac build complains (werror) + see below

UdjinM6 · 2021-11-01T00:24:03Z

src/governance/object.cpp

 void CGovernanceObject::UpdateLocalValidity()
 {
-    LOCK(cs_main);
+    LOCK(cs);


UdjinM6 · 2021-11-01T00:24:35Z

src/governance/object.cpp

            return false;
        }
-        if (fCheckCollateral && !IsCollateralValid(strError, fMissingConfirmations)) {
+        if (fCheckCollateral && !WITH_LOCK(cs_main, return IsCollateralValid(strError, fMissingConfirmations))) {


that's probably a deadlock

UdjinM6 · 2021-11-01T00:26:41Z

src/governance/object.cpp

    // RETRIEVE TRANSACTION IN QUESTION

-    if (!GetTransaction(nCollateralHash, txCollateral, Params().GetConsensus(), nBlockHash)) {
+    if (LOCK(cs); !GetTransaction(nCollateralHash, txCollateral, Params().GetConsensus(), nBlockHash)) {


that's also a deadlock potentially

cs should be held already as per EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main) in .h, maybe add AssertLockHelds for both locks at the start of the method

UdjinM6 · 2021-11-01T00:43:37Z

src/governance/governance.cpp

                govobj.GetDataAsPlainString(), govobj.GetObjectType());

-    if (govobj.GetObjectType() == GOVERNANCE_OBJECT_TRIGGER && !triggerman.AddNewTrigger(nHash)) {
+    if (govobj.GetObjectType() == GOVERNANCE_OBJECT_TRIGGER && !WITH_LOCK(governance.cs, return triggerman.AddNewTrigger(nHash))) {


for all governance.cs locks for triggerman here and below in this file - we hold cs already, no need to lock twice (I know you did this to silence werror probably but it feels like a wrong way to fix it)

github-actions · 2021-12-30T19:04:59Z

This Pull Request may conflict if the Pull Requests below are merged first.

#4649
conflictable files: src/governance/governance.cpp,src/governance/governance.h

github-actions · 2022-02-02T09:10:24Z

This pull request has conflicts, please rebase.

PastaPastaPasta added this to the 18 milestone Sep 26, 2021

PastaPastaPasta requested a review from UdjinM6 September 26, 2021 21:23

PastaPastaPasta marked this pull request as draft September 28, 2021 18:07

pravblockc reviewed Sep 28, 2021

View reviewed changes

src/governance/governance.h Outdated Show resolved Hide resolved

PastaPastaPasta force-pushed the governance-adjustments branch from 4b39759 to a01374a Compare September 28, 2021 21:23

PastaPastaPasta marked this pull request as ready for review September 28, 2021 21:23

UdjinM6 requested changes Sep 30, 2021

View reviewed changes

PastaPastaPasta force-pushed the governance-adjustments branch from a01374a to 97c3c7a Compare October 1, 2021 18:43

github-actions bot added the needs rebase label Oct 1, 2021

PastaPastaPasta force-pushed the governance-adjustments branch from 97c3c7a to bcf64f9 Compare October 1, 2021 19:27

github-actions bot removed the needs rebase label Oct 1, 2021

github-actions bot added the needs rebase label Oct 5, 2021

PastaPastaPasta closed this Oct 5, 2021

PastaPastaPasta force-pushed the governance-adjustments branch from 458532e to 8d5c586 Compare October 5, 2021 21:29

PastaPastaPasta added 7 commits October 5, 2021 17:34

governance: use more atomic, adjust mutexes

751077d

fix

a5b6114

Signed-off-by: pasta <[email protected]>

std atomic

4b51df0

return by value instead of by reference, as they are protected

3592b2d

Lock cs as needed, adjust cs_main locking to hopefully avoid deadlock

cd2275b

use .load in constructor initializer list

72967c9

make nTimeLastDiff, nCachedBlockHeight atomic, lock cs for cmapInvali…

cc35ad2

…dVotes

PastaPastaPasta reopened this Oct 5, 2021

github-actions bot removed the needs rebase label Oct 5, 2021

PastaPastaPasta added 3 commits October 11, 2021 13:16

fixes

d843109

Signed-off-by: pasta <[email protected]>

fixes

0c31b32

fix

9325e6d

UdjinM6 requested changes Nov 1, 2021

View reviewed changes

github-actions bot mentioned this pull request Dec 30, 2021

Avoid using raw pointers in GetAllNewerThan results #4649

Merged

github-actions bot added the needs rebase label Feb 2, 2022

UdjinM6 removed this from the 18 milestone Oct 17, 2022

PastaPastaPasta closed this Sep 4, 2024

Conversation

PastaPastaPasta commented Sep 26, 2021

Uh oh!

PastaPastaPasta commented Sep 27, 2021

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented Oct 1, 2021

Uh oh!

github-actions bot commented Oct 1, 2021

Uh oh!

github-actions bot commented Oct 1, 2021

Uh oh!

PastaPastaPasta commented Oct 1, 2021

Uh oh!

PastaPastaPasta commented Oct 1, 2021

Uh oh!

github-actions bot commented Oct 5, 2021

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 30, 2021

Uh oh!

github-actions bot commented Feb 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants