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

Debug/dkg results #4660

Merged
merged 7 commits into from
Apr 16, 2024
Merged

Debug/dkg results #4660

merged 7 commits into from
Apr 16, 2024

Conversation

jferrant
Copy link
Collaborator

@jferrant jferrant commented Apr 8, 2024

We needlessly were triggering DKG rounds due to a timing issue between our first check to see if a key was approved and the later threshold check being exceeded.
Now if we have not locally stored an approved key, we will always attempt the DKG queue checks. It will always follow this check by a final loading of whatever is set in the contract. Not bothering to queue a command if it finds an approved key. This should mitigate needless triggering of DKG for the obvious case.

However, we still need the later update to retrieve DKG shares based on round ID in #4654 as we can never be sure that our view of the contract is correct at the time of triggering a DKG round. (we could be milliseconds off when checking the approved key is set still).

xoloki
xoloki previously approved these changes Apr 9, 2024
Copy link
Collaborator

@xoloki xoloki left a comment

Choose a reason for hiding this comment

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

LGTM

@obycode
Copy link
Contributor

obycode commented Apr 9, 2024

Just realized we should retarget this to develop. I will rebase and force push that change here.

@obycode obycode changed the base branch from next to develop April 9, 2024 13:40
@jferrant jferrant requested a review from xoloki April 9, 2024 18:16
xoloki
xoloki previously approved these changes Apr 10, 2024
Copy link
Collaborator

@xoloki xoloki left a comment

Choose a reason for hiding this comment

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

LGTM, trivial question about if matches!()

stacks-signer/src/signer.rs Outdated Show resolved Hide resolved
hstove
hstove previously approved these changes Apr 10, 2024
Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

LGTM - small comment about a log message that might just be me misreading something

stacks-signer/src/signer.rs Show resolved Hide resolved
jcnelson
jcnelson previously approved these changes Apr 10, 2024
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

Small nits but LGTM

@obycode obycode dismissed stale reviews from jcnelson, hstove, and xoloki via 344388b April 12, 2024 14:57
kantai
kantai previously approved these changes Apr 12, 2024
hstove
hstove previously approved these changes Apr 16, 2024
@obycode
Copy link
Contributor

obycode commented Apr 16, 2024

@jferrant can you double check my conflict resolution in the last commit here?

Copy link
Collaborator Author

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

Can't approve, but looks good to me :D

Copy link
Collaborator

@xoloki xoloki left a comment

Choose a reason for hiding this comment

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

LGTM

@obycode obycode merged commit 71abebc into develop Apr 16, 2024
1 check passed
@obycode obycode deleted the debug/dkg-results branch April 16, 2024 21:06
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.

6 participants