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

Miners handle signers block responses from the stacker db instance #4281

Merged
merged 5 commits into from
Feb 2, 2024

Conversation

jferrant
Copy link
Collaborator

@jferrant jferrant commented Jan 24, 2024

Description

This is the first stab at miners handling stacker db events from the signers. Progress beyond this point is blocked by the .signers pox 4 contract combined with the ability for signers to broadcast their aggregate keys.

Applicable issues

Additional info (benefits, drawbacks, caveats)

A more complete solution would directly use the event observer trait (avoiding the use of the http server), but for an initial attempt, I just directly access the stacker db instance of the miner. As I am semi blocked, I might try making this change now. However review would be good anyway as this logic will not change much.

Next steps would be to actually append the block when we have .signers / aggregate key correctly set

Copy link

codecov bot commented Jan 24, 2024

Codecov Report

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

Comparison is base (86e3c26) 82.83% compared to head (20f2e0e) 82.60%.

Files Patch % Lines
testnet/stacks-node/src/nakamoto_node/miner.rs 4.47% 128 Missing ⚠️
libsigner/src/messages.rs 94.33% 48 Missing ⚠️
stackslib/src/net/stackerdb/db.rs 0.00% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4281      +/-   ##
==========================================
- Coverage   82.83%   82.60%   -0.23%     
==========================================
  Files         442      443       +1     
  Lines      314970   315904     +934     
==========================================
+ Hits       260898   260963      +65     
- Misses      54072    54941     +869     

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

@jferrant jferrant force-pushed the feat/miners-subscribe-to-signers branch 2 times, most recently from c531924 to aa0acd7 Compare January 24, 2024 18:41
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.

It would be nice to get rid of some of the expect calls, but they were already present so not a blocker for this PR

libsigner/src/events.rs Outdated Show resolved Hide resolved
@jferrant jferrant force-pushed the feat/miners-subscribe-to-signers branch from aa0acd7 to caf7299 Compare January 25, 2024 17:16
@jferrant jferrant force-pushed the feat/miners-subscribe-to-signers branch from 8fe5d80 to af74765 Compare January 25, 2024 19:02
@jferrant jferrant changed the base branch from next to bugfix/signer-signature-hash January 25, 2024 21:43
@jferrant jferrant force-pushed the feat/miners-subscribe-to-signers branch 2 times, most recently from 9b43720 to e11994f Compare January 26, 2024 17:17
@jferrant jferrant changed the base branch from bugfix/signer-signature-hash to next January 26, 2024 17:18
@jferrant
Copy link
Collaborator Author

It would be nice to get rid of some of the expect calls, but they were already present so not a blocker for this PR

Fixed in #4294

@jferrant jferrant force-pushed the feat/miners-subscribe-to-signers branch 2 times, most recently from 1bd4e72 to 55f73de Compare January 30, 2024 23:10
@jferrant jferrant force-pushed the feat/miners-subscribe-to-signers branch 2 times, most recently from 3a2c200 to b526aac Compare January 31, 2024 18:21
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.

This looks mostly good to me -- I just have a couple comments about u8 enums, and a request on the changes in the miner thread.

libsigner/src/messages.rs Outdated Show resolved Hide resolved
stackslib/src/net/api/postblock_proposal.rs Outdated Show resolved Hide resolved
testnet/stacks-node/src/nakamoto_node/miner.rs Outdated Show resolved Hide resolved
@jferrant jferrant force-pushed the feat/miners-subscribe-to-signers branch from fc8fd19 to e2a5478 Compare February 1, 2024 15:05
@jferrant jferrant requested a review from kantai February 1, 2024 15:06
@jferrant jferrant force-pushed the feat/miners-subscribe-to-signers branch from e2a5478 to 2aa3528 Compare February 2, 2024 00:50
libsigner/src/messages.rs Outdated Show resolved Hide resolved
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.

Just one question about whether or not rejection thresholds should be configurable, but otherwise LGTM.

Before merging, please implement extension traits for wsts types and then implement StacksMessageCodec for those.

@jferrant jferrant force-pushed the feat/miners-subscribe-to-signers branch from 2aa3528 to 29301d6 Compare February 2, 2024 18:35
@jferrant jferrant force-pushed the feat/miners-subscribe-to-signers branch from 2b33d4f to 20f2e0e Compare February 2, 2024 22:31
@jferrant jferrant merged commit dd006d4 into next Feb 2, 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.

None yet

4 participants