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

Add v0 signer process_event paths #4788

Merged
merged 20 commits into from
May 22, 2024
Merged

Add v0 signer process_event paths #4788

merged 20 commits into from
May 22, 2024

Conversation

jferrant
Copy link
Collaborator

@jferrant jferrant commented May 13, 2024

Closes #4785
I can't seem to manage to create a valid block to sign (having a hard time understanding the tenure change tx mostly), but at least I have a test for the rejection path.

Summary of the more major changes:

  • Created a SignerEventTrait so that we could reuse a majority of the events.rs logic in libsigner (Enables custom SignerMessage for v0 and v1)
  • Created a SignerMesssage trait and MessageSlotID trait so we could reuse the stackerdb.rs logic
  • Created a Signer trait which enables generic spawning of either a v0 or v1 signer.
  • Made SignerTest generic, taking any struct implementing a Signer trait to enable reuse of some testing logic.
  • Changed the channel for the signers to take an enum which wraps the OperationResult and added a Status result so we can directly query the state of the signers and ensure state is as expected in tests.

Signed-off-by: Jacinta Ferrant <[email protected]>
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.

Looking good so far. Mostly just some concerns about whether or not we need to bother checking the reward cycle when signing (lest the signer accidentally refuses to sign a valid block).

hstove
hstove previously requested changes May 14, 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.

Overall this LGTM, and it's exciting to see a v0 that should be pretty much working (just needs tests). My main point of feedback is around shared StackerDB code.

stacks-signer/src/v0/signer.rs Outdated Show resolved Hide resolved
stacks-signer/src/signerdb.rs Show resolved Hide resolved
stacks-signer/src/v0/signer.rs Show resolved Hide resolved
stacks-signer/src/v0/stackerdb.rs Outdated Show resolved Hide resolved
…st easily spun up with duplicate code minimized

Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
@jferrant jferrant changed the title WIP: Add v0 signer process_event paths Add v0 signer process_event paths May 16, 2024
stacks-signer/src/lib.rs Outdated Show resolved Hide resolved
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!

stacks-signer/src/runloop.rs Show resolved Hide resolved
jcnelson
jcnelson previously approved these changes May 20, 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.

Overall LGTM; just minor things that are easily fixed

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.

Small change, I caught this when writing new signer tests

stacks-signer/src/lib.rs Show resolved Hide resolved
@jferrant jferrant requested a review from hstove May 22, 2024 12:56
Signed-off-by: Jacinta Ferrant <[email protected]>
hstove
hstove previously approved these changes May 22, 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!

jcnelson
jcnelson previously approved these changes May 22, 2024
@jferrant jferrant dismissed stale reviews from jcnelson and hstove via da08ad6 May 22, 2024 17:25
@jferrant jferrant requested review from jcnelson and hstove May 22, 2024 17:25
@jferrant jferrant enabled auto-merge May 22, 2024 17:34
@jferrant jferrant added this pull request to the merge queue May 22, 2024
Merged via the queue into develop with commit 812b4fd May 22, 2024
1 check passed
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Oct 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants