Skip to content
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

Whitelist nodes in llmq_dkgerrors.py #3112

Merged
merged 1 commit into from
Oct 1, 2019
Merged

Whitelist nodes in llmq_dkgerrors.py #3112

merged 1 commit into from
Oct 1, 2019

Conversation

codablock
Copy link

@codablock codablock commented Sep 25, 2019 •
edited
Loading

This fixes a few rare test failures

@codablock codablock added this to the 14.1 milestone Sep 25, 2019
@UdjinM6
Copy link

UdjinM6 commented Sep 25, 2019

Can't we just add -whitelist=127.0.0.1 in llmq-dkgerrors.py instead?

@codablock
Copy link
Author

Hmm yeah, that's also an option and ensures that we also test invalid sigs here. I'll change this PR later

@codablock codablock force-pushed the pr_dkg_noquroumsiglie branch from a80daef to 5c946d4 Compare September 26, 2019 13:47
@codablock codablock changed the title Remove simulation of invalid signatures in CDKGSession::SendCommitment Whitelist nodes in llmq_dkgerrors.py Sep 26, 2019
@codablock
Copy link
Author

Updated PR to simply whitelist nodes

UdjinM6
UdjinM6 approved these changes Sep 26, 2019
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.

utACK

@codablock
Copy link
Author

Current test failure is unrelated to this PR

PastaPastaPasta
PastaPastaPasta reviewed Sep 26, 2019
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

Hmm.

Wouldn't this mean that even if node B was doing something which would (and should) get it banned it wouldn't be banned? This could result in us not catching a bug where nodes (on real nets) do something wrong/bannable and get banned and we didn't catch it here because it was whitelisted.

@codablock
Copy link
Author

@PastaPastaPasta llmq_dkgerrors.py is not supposed to test p2p banning, but the DKG's behavior on wrong behavior (which is not skipped by whitelisting).

PastaPastaPasta
PastaPastaPasta previously approved these changes Sep 28, 2019
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK

@PastaPastaPasta
Copy link
Member

needs rebase

UdjinM6
UdjinM6 requested changes Sep 29, 2019
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.

Needs rebase

nmarley
nmarley reviewed Sep 29, 2019
View reviewed changes
Copy link

@nmarley nmarley left a comment

Choose a reason for hiding this comment

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

utACK, pending rebase

@codablock
Copy link
Author

Rebased on develop

@codablock codablock force-pushed the pr_dkg_noquroumsiglie branch from 5c946d4 to 900d22c Compare September 30, 2019 06:31
@codablock
Whitelist nodes in llmq-dkgerrors.py
be11b83 
This avoids banning due to invalid sigs
@codablock codablock force-pushed the pr_dkg_noquroumsiglie branch from 900d22c to be11b83 Compare September 30, 2019 06:32
UdjinM6
UdjinM6 approved these changes Sep 30, 2019
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.

utACK

PastaPastaPasta
PastaPastaPasta approved these changes Sep 30, 2019
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

re-utACK

@codablock codablock merged commit 2ca2138 into dashpay:develop Oct 1, 2019
@codablock codablock deleted the pr_dkg_noquroumsiglie branch October 1, 2019 07:25
barrystyle pushed a commit to PACGlobalOfficial/PAC that referenced this pull request Jan 22, 2020
@codablock
Whitelist nodes in llmq-dkgerrors.py (dashpay#3112)
27f06ca 
This avoids banning due to invalid sigs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nmarley nmarley nmarley left review comments

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
15
Development

Successfully merging this pull request may close these issues.
4 participants
@codablock @UdjinM6 @PastaPastaPasta @nmarley