-
Notifications
You must be signed in to change notification settings - Fork 667
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
Feat/signers read stackerdb #4658
Conversation
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
…overwritten so its impossible to do properly Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code LGTM! I'm curious about the slow test, though. It doesn't seem like restarting the signer should require re-syncing the stacks-node.
Yeah, I am not 100% sure why this is the case. I actually can't merge this until #4654 is done. So will keep this as is for now and will look into it further when that PR is completed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's in draft, but I had already started a review, so I figured I'd just finish it.
The base branch was changed.
… into feat/signers-read-stackerdb
Signed-off-by: Jacinta Ferrant <[email protected]>
… follow Signed-off-by: Jacinta Ferrant <[email protected]>
… into feat/signers-read-stackerdb
Signed-off-by: Jacinta Ferrant <[email protected]>
… into feat/signers-read-stackerdb
…ger next steps Signed-off-by: Jacinta Ferrant <[email protected]>
32b548b
to
ff96f8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! So exciting to get this in, we've talked about it forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job. A little nit regarding comments / logs, and that may be to me misunderstanding things. It shouldn't be blocking a merge, but a few more comments around why this state happened in refresh/update DKG would be help.
Also, I was going to comment "nice job on that test" but I already did 😂
Closes #4636
Attempted to do for sign as well, but this would require substantial changes to the way a miner stores its messages as we require every single sign message to replicate state and miners overwrite their own messages as they use the same slot for everything. Sign rounds are also quite quick and a miner already has timeout logic in place for triggering a new sign request, so this is less of an issue anyway.