Skip to content

governance: add threading annotations, add default values, adjust constructors #4465

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
PastaPastaPasta wants to merge 10 commits into from
Closed

governance: add threading annotations, add default values, adjust constructors #4465

PastaPastaPasta wants to merge 10 commits into from

Conversation

@PastaPastaPasta
Copy link
Member

@PastaPastaPasta PastaPastaPasta commented Sep 26, 2021

No description provided.

@PastaPastaPasta PastaPastaPasta added this to the 18 milestone Sep 26, 2021
@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Sep 27, 2021

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

@PastaPastaPasta PastaPastaPasta marked this pull request as draft September 28, 2021 18:07
pravblockc
pravblockc reviewed Sep 28, 2021
View reviewed changes
@PastaPastaPasta PastaPastaPasta marked this pull request as ready for review September 28, 2021 21:23
UdjinM6
UdjinM6 requested changes Sep 30, 2021
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Oct 1, 2021

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
Copy link

github-actions bot commented Oct 1, 2021

This pull request has conflicts, please rebase.

@github-actions
Copy link

github-actions bot commented Oct 1, 2021

This pull request has conflicts, please rebase.

@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Oct 1, 2021

rebased

@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Oct 1, 2021

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

@github-actions
Copy link

github-actions bot commented Oct 5, 2021

This pull request has conflicts, please rebase.

@PastaPastaPasta
fixes
d843109 
Signed-off-by: pasta <pasta@dashboost.org>
@PastaPastaPasta
fixes
0c31b32
@PastaPastaPasta
fix
9325e6d
UdjinM6
UdjinM6 requested changes Nov 1, 2021
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mac build complains (werror) + see below

src/governance/object.cpp
void CGovernanceObject::UpdateLocalValidity()
{
LOCK(cs_main);
LOCK(cs);
Copy link

@UdjinM6 UdjinM6 Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

src/governance/object.cpp
return false;
}
if (fCheckCollateral && !IsCollateralValid(strError, fMissingConfirmations)) {
if (fCheckCollateral && !WITH_LOCK(cs_main, return IsCollateralValid(strError, fMissingConfirmations))) {
Copy link

@UdjinM6 UdjinM6 Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's probably a deadlock

src/governance/object.cpp
// RETRIEVE TRANSACTION IN QUESTION

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

@UdjinM6 UdjinM6 Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. that's also a deadlock potentially
  2. 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

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))) {
Copy link

@UdjinM6 UdjinM6 Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

github-actions bot commented Dec 30, 2021

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 github-actions bot mentioned this pull request Dec 30, 2021
Merged
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

This pull request has conflicts, please rebase.

@UdjinM6 UdjinM6 removed this from the 18 milestone Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UdjinM6 UdjinM6 UdjinM6 requested changes

+1 more reviewer

@pravblockc pravblockc pravblockc left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs rebase

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@PastaPastaPasta @UdjinM6 @pravblockc