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

feat: generalize ehf activation #5824

Merged
merged 2 commits into from
Feb 29, 2024
Merged

feat: generalize ehf activation #5824

merged 2 commits into from
Feb 29, 2024

Conversation

panleone
Copy link

@panleone panleone commented Jan 14, 2024 •
edited by knst
Loading

Issue being fixed or feature implemented

Try to sign/mine any ehf deployment and not only mn_rr. As asked in the review this decouples (and improves) commit e24cb23 from PR #5799

What was done?

See commit description

How Has This Been Tested?

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

knst
knst reviewed Jan 14, 2024
View reviewed changes
src/llmq/ehf_signals.cpp Show resolved Hide resolved
src/llmq/ehf_signals.cpp Outdated Show resolved Hide resolved
@knst knst added this to the 20.1 milestone Jan 14, 2024
@knst
Copy link
Collaborator

knst commented Jan 14, 2024 •
edited
Loading

you can add also changes from test/functional/test_framework/test_framework.py related to s/activate_mn_rr/activate_ehf/ to this PR

knst
knst previously approved these changes Jan 14, 2024
View reviewed changes
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

utACK

UdjinM6
UdjinM6 requested changes Jan 14, 2024
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.

LGTM in general 👍 however I'd like to squash few lines to make no-whitespaces diff look a bit more compact

src/llmq/ehf_signals.cpp Outdated Show resolved Hide resolved
src/llmq/ehf_signals.cpp Outdated Show resolved Hide resolved
src/llmq/ehf_signals.cpp Outdated Show resolved Hide resolved
@panleone panleone force-pushed the generalized_ehf branch 2 times, most recently from 778d2bc to 721770e Compare January 15, 2024 07:58
UdjinM6
UdjinM6 previously approved these changes Jan 15, 2024
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

knst
knst previously approved these changes Jan 15, 2024
View reviewed changes
Copy link
Collaborator

@knst knst 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

PastaPastaPasta
PastaPastaPasta requested changes Jan 28, 2024
View reviewed changes
src/llmq/ehf_signals.cpp Outdated Show resolved Hide resolved
src/llmq/ehf_signals.cpp Outdated Show resolved Hide resolved
@panleone panleone dismissed stale reviews from knst and UdjinM6 via 0fc3599 January 30, 2024 14:14
UdjinM6
UdjinM6 previously approved these changes Jan 30, 2024
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.

re-utACK

@PastaPastaPasta
Copy link
Member

@knst Can you please help to coordinate a devnet test with this build and 20.0.4 ensuring that both activate EHF as expected and no issues are detected?

@DashCoreAutoGuix
Copy link

Guix Automation has failed due to the HEAD commit not being signed by an authorized core-team member. Please rebase and sign or push a new empty signed commit to allow Guix build to happen.

@PastaPastaPasta
Copy link
Member

Can't push a commit here due to lack of permission issues @panleone; please check this box; or I'd have to create a new PR in order to get a build here
image

@panleone
Copy link
Author

Can't push a commit here due to lack of permission issues @panleone; please check this box; or I'd have to create a new PR in order to get a build here image

hmm looks this option is not available since I forked dash from an organisation (https://stackoverflow.com/questions/75596018/allow-edits-from-maintainers-does-not-work-when-the-pr-is-created-by-a-group)

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 3, 2024

This pull request has conflicts, please rebase.

@panleone panleone mentioned this pull request Feb 3, 2024
Open
5 tasks
@panleone
feat(consensus): Generalize ehf activation
de38dca 
Try to sign any ehf deployment that can be activated and that hasn't been mined on chain yet.
Also when receiving a new recovered signature try to match it with any ehf deployment which hasn't been mined on chain yet
@panleone
test: add activate_ehf_by_name
1821d92 
To avoid copy and pasting future ehf deployments activation functions
@DashCoreAutoGuix
Copy link

Guix Automation has failed due to the HEAD commit not being signed by an authorized core-team member. Please rebase and sign or push a new empty signed commit to allow Guix build to happen.

@panleone
Copy link
Author

panleone commented Feb 8, 2024

had to rebase since there was a conflict
relevant diff is here
https://github.com/dashpay/dash/compare/0fc3599cf65ef3fc6c191e6345604d13de34354b..1821d92b66ad570508b9c4318283fba7e21ecfc7
(in the file ehf_signals.cpp )

UdjinM6
UdjinM6 approved these changes Feb 8, 2024
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.

rebase looks clean, re-utACK

knst
knst approved these changes Feb 16, 2024
View reviewed changes
Copy link
Collaborator

@knst knst left a comment •
edited
Loading

Choose a reason for hiding this comment

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

tACK

Details here: #5857

@knst knst mentioned this pull request Feb 16, 2024
Closed
5 tasks
@knst knst requested a review from PastaPastaPasta February 16, 2024 22:10
PastaPastaPasta
PastaPastaPasta approved these changes Feb 29, 2024
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 for merge via merge commit

@PastaPastaPasta PastaPastaPasta merged commit 7701948 into dashpay:develop Feb 29, 2024
8 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knst knst knst approved these changes

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

Assignees
No one assigned
Labels
guix-build
Projects
None yet
Milestone
20.1
Development

Successfully merging this pull request may close these issues.
5 participants
@panleone @knst @PastaPastaPasta @DashCoreAutoGuix @UdjinM6