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.

Sign up for GitHub

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

Jump to bottom

Process coordinator messages to duplicate state between multiple coor… #4186

Merged
merged 3 commits into from
Jan 10, 2024
Merged

Process coordinator messages to duplicate state between multiple coor… #4186

merged 3 commits into from
Jan 10, 2024

Conversation

jferrant
Copy link
Collaborator

@jferrant jferrant commented Dec 18, 2023 •
edited
Loading

Description

Coordinator state should be replicated between signers so that every signer can submit its aggregate key and also so it can pick up where other coordinators leave off if said coordinators go down.

Applicable issues

@jferrant jferrant force-pushed the feat/consistent-state-across-signers branch 3 times, most recently from 1c3e577 to de2844f Compare December 19, 2023 15:37
@jferrant jferrant force-pushed the feat/consistent-state-across-signers branch from de2844f to d4b55ac Compare December 27, 2023 16:23
@codecov Codecov
Copy link

codecov bot commented Jan 2, 2024 •
edited
Loading

Codecov Report

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

Comparison is base (9b28e48) 76.40% compared to head (b1fc834) 82.97%.

Files Patch % Lines
testnet/stacks-node/src/tests/signer.rs 0.00% 30 Missing ⚠️
stacks-signer/src/runloop.rs 0.00% 13 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             next    #4186      +/-   ##
==========================================
+ Coverage   76.40%   82.97%   +6.56%     
==========================================
  Files         430      430              
  Lines      306130   306140      +10     
==========================================
+ Hits       233911   254007   +20096     
+ Misses      72219    52133   -20086

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

@jferrant jferrant force-pushed the feat/consistent-state-across-signers branch from cc7fbd3 to 5b7a772 Compare January 8, 2024 18:00
@jferrant jferrant marked this pull request as ready for review January 8, 2024 18:08
@jferrant jferrant requested review from jcnelson and kantai January 8, 2024 18:08
@jferrant jferrant force-pushed the feat/consistent-state-across-signers branch from 5b7a772 to 4a9c565 Compare January 9, 2024 15:21
kantai
kantai reviewed Jan 9, 2024
View reviewed changes
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 LGTM, just a question about the error handling of process_inbound_messages

stacks-signer/src/runloop.rs Outdated Show resolved Hide resolved
@jferrant jferrant requested a review from kantai January 9, 2024 20:54
@jferrant jferrant force-pushed the feat/consistent-state-across-signers branch from cfda130 to 3910d02 Compare January 9, 2024 21:27
xoloki
xoloki approved these changes Jan 10, 2024
View reviewed changes
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, just one question about inbound processing but this PR isn't responsible for the behavior in question so it's not a blocker.

stacks-signer/src/runloop.rs
let (messages, results) = self
.coordinator
.process_inbound_messages(&inbound_messages)
.unwrap_or_else(|e| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the best behavior? I.e. when processing inbound fails, should we log and continue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think its the currently best behaviour. For now we don't want messages received out of order for example to crash the system. I.e. if a rogue signer starts sending valid but inappropriate responses, the coordinator state could error. I think we might need to introduce some sort of recovery. But for now, I think its better than crashing outright.

@jferrant jferrant merged commit d7e9a80 into next Jan 10, 2024
2 checks 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 Nov 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kantai kantai kantai approved these changes

@xoloki xoloki xoloki approved these changes

@soju-drinker soju-drinker Awaiting requested review from soju-drinker

@jcnelson jcnelson Awaiting requested review from jcnelson

Assignees
No one assigned
Labels
locked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jferrant @blockstack-devops @xoloki @kantai