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.

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

[clarity] Cast aggregated public key vote #4239

Merged
merged 21 commits into from
Jan 31, 2024

Conversation

friedger
Copy link
Collaborator

@friedger friedger commented Jan 10, 2024

Description

This PR adds clarity code to tally a vote for the aggregated key for each cycle.

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@friedger friedger mentioned this pull request Jan 10, 2024
5 tasks
@friedger friedger changed the base branch from master to next January 10, 2024 13:45
@friedger friedger changed the base branch from next to chore/pox-4-single-file January 10, 2024 13:49
@friedger friedger changed the title Tally aggregated public key vote [clarity] Cast aggregated public key vote Jan 10, 2024
@jferrant
Copy link
Collaborator

jferrant commented Jan 23, 2024

Is this meant ot be built off of next? I don't see a corresponding PR for chore/pox-4-single-file

Also make sure it is reabsed off of next and does not contain merge commits. I am seeing lots of unrelated commits / diffs and not sure if that is just because the target branch is not rebased properly on next or if this one was not properly rebased off of next or both.

@friedger
Copy link
Collaborator Author

It is meant to be off the code that contains .signer

@friedger friedger changed the base branch from chore/pox-4-single-file to next January 24, 2024 10:12
@friedger friedger force-pushed the feat/pox-4-cast-aggr-pubkey-vote branch from a9bd714 to 3461d20 Compare January 24, 2024 15:56
@friedger friedger changed the base branch from next to feat/signers-stackerdb-contract January 24, 2024 15:56
@kantai kantai force-pushed the feat/signers-stackerdb-contract branch from 8056694 to e9fd4a9 Compare January 25, 2024 15:23
@friedger friedger force-pushed the feat/pox-4-cast-aggr-pubkey-vote branch from 987af2b to b758988 Compare January 29, 2024 15:16
@friedger friedger changed the base branch from feat/signers-stackerdb-contract to feat/stacker-bitvec January 29, 2024 15:17
@friedger friedger marked this pull request as ready for review January 29, 2024 16:15
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Attention: 55 lines in your changes are missing coverage. Please review.

Comparison is base (d9c1aa1) 52.88% compared to head (0cc9c45) 83.38%.
Report is 8 commits behind head on next.

Files Patch % Lines
...src/chainstate/stacks/boot/signers_voting_tests.rs 85.57% 46 Missing ⚠️
libsigner/src/tests/mod.rs 90.69% 4 Missing ⚠️
stackslib/src/clarity_vm/clarity.rs 89.18% 4 Missing ⚠️
stacks-signer/src/runloop.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             next    #4239       +/-   ##
===========================================
+ Coverage   52.88%   83.38%   +30.50%     
===========================================
  Files         434      435        +1     
  Lines      308750   309245      +495     
===========================================
+ Hits       163277   257873    +94596     
+ Misses     145473    51372    -94101     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hstove
Copy link
Contributor

hstove commented Jan 30, 2024

This PR should be updated to merge into next

@friedger friedger changed the base branch from feat/stacker-bitvec to next January 30, 2024 15:43
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

Just had one comment in the clarity code.

Otherwise, LGTM -- this contract will need to be updated as the stackerdb/signer work changes to support more signers, but that can be handled in a follow-up PR.

stackslib/src/chainstate/stacks/boot/signers-voting.clar Outdated Show resolved Hide resolved
@friedger
Copy link
Collaborator Author

Comments addressed

@friedger friedger self-assigned this Jan 31, 2024
/// Alice can vote successfully.
/// Bob is out of the voting window.
#[test]
fn vote_for_aggregate_public_key_in_last_block() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kantai aren't we able to vote for DKG at any point in a reward cycle ? @friedger

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My understanding was that we need to know the number of slots. This can happen only in the prepare phase. We check that the cycle was set as and registered in .signers.

Copy link
Collaborator

@jferrant jferrant left a comment

Choose a reason for hiding this comment

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

Aside from the one change potentially about voting window, I see no issue. We can also always address these sorts of comments in a follow up PR if its better to just merge :)

@saralab saralab merged commit f698448 into next Jan 31, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Clarity] DKG vote registration
8 participants