-
Notifications
You must be signed in to change notification settings - Fork 697
Feat/signer state machine rollout #6215
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
base: develop
Are you sure you want to change the base?
Feat/signer state machine rollout #6215
Conversation
…te accordingly Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
…event if capitulation should occur Signed-off-by: Jacinta Ferrant <[email protected]>
…nd add get_burn_block_received_time_from_signers Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
… into feat/signer-state-machine-rollout
…ew infallible Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
58f8f12
to
0d44833
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6215 +/- ##
===========================================
+ Coverage 83.26% 83.40% +0.14%
===========================================
Files 541 544 +3
Lines 397695 398981 +1286
Branches 323 323
===========================================
+ Hits 331132 332771 +1639
+ Misses 66555 66202 -353
Partials 8 8
... and 35 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Signed-off-by: Jacinta Ferrant <[email protected]>
…meout change Signed-off-by: Jacinta Ferrant <[email protected]>
…fork Signed-off-by: Jacinta Ferrant <[email protected]>
35ad183
to
ea69d33
Compare
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]>
Signed-off-by: Jacinta Ferrant <[email protected]>
…dateContent::V2 Signed-off-by: Jacinta Ferrant <[email protected]>
6fd42cb
to
4d199ef
Compare
… into feat/signer-state-machine-rollout
Signed-off-by: Jacinta Ferrant <[email protected]>
… into feat/signer-state-machine-rollout
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'm still reviewing the new tests, but I wanted to submit this now since I caught the signerdb migration issue
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 looks good to me once the migration changes are handled. I haven't reviewed the test code yet, but wanted to get my comments in on the codebase first.
stacks-signer/src/v0/signer_state.rs
Outdated
pub static SUPPORTED_SIGNER_PROTOCOL_VERSION: u64 = 1; | ||
pub static SUPPORTED_SIGNER_PROTOCOL_VERSION: u64 = 2; | ||
/// The version at which global signer state activates | ||
pub static GLOBAL_SIGNER_STATE_ACTIVATION_VERSION: u64 = 2; |
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 think we should bump this to u64::MAX
at the moment. We still haven't confirmed that the mainnet signers will perform as well as the do today, so we want there to be a release with all these changes in it that doesn't "activate", we'll just monitor the updates and confirm that in all cases, the signer set would have continued signing as well as they are currently. Then in the next release, we could bump the support signer protocol version to 3 and change the ACTIVATION_VERSION to 3 as well.
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.
Done in a971450
Would you expect the SUPPORTED_SIGNER_PROTOCOL_VERSION stay at 1 then? Version 1 and Version 2 are identical. if we don't plan to do anything with it, would just keep SUPPORTED_SIGNER_PRTOCOL_VERSION as 1 and then when we want to activate it...set it to 2 and GLOBAL_SIGNER_STATE_ACTIVATION_VERSION to 2 as well.
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.
Due to this change, I will have to revert almost all test changes. OR I can force all those tests to have a pinned version but htat is dangerous as they should only be pinned until we rollover the test version.
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.
Would you expect the SUPPORTED_SIGNER_PROTOCOL_VERSION stay at 1 then? Version 1 and Version 2 are identical. if we don't plan to do anything with it, would just keep SUPPORTED_SIGNER_PRTOCOL_VERSION as 1 and then when we want to activate it...set it to 2 and GLOBAL_SIGNER_STATE_ACTIVATION_VERSION to 2 as well.
Oh, yes, lets do that. Obviously, keep all the signer state machine versioning logic in place, but we can just set:
pub static SUPPORTED_SIGNER_PROTOCOL_VERSION: u64 = 1;
/// The version at which global signer state activates
pub static GLOBAL_SIGNER_STATE_ACTIVATION_VERSION: u64 = 2;
And then, for testing, I think we should do something like set the supported_signer_protocol_version
in the signer config, and read it from the config rather than the static. It shouldn't be modifiable in non-testing environments, so a check like:
if !cfg!(feature(testing), test) {
assert_eq!(config.supported_signer_protocol_version, SUPPORTED_SIGNER_PROTOCOL_VERSION);
}
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.
Don't actually have to revert much. Just a few that expect a specific reject code. Thankfully everything else works regardless.
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.
First pass, will add some more comments in the next pass.
Signed-off-by: Jacinta Ferrant <[email protected]>
…fix typos, remove duplicate code in make_miner_state Signed-off-by: Jacinta Ferrant <[email protected]>
47c22a5
to
a971450
Compare
Signed-off-by: Jacinta Ferrant <[email protected]>
…overwritten in a test Signed-off-by: Jacinta Ferrant <[email protected]>
28d6160
to
8a8c650
Compare
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]>
… into feat/signer-state-machine-rollout
Signed-off-by: Jacinta Ferrant <[email protected]>
…v_miner as global signer state is not active yet Signed-off-by: Jacinta Ferrant <[email protected]>
Closes #6092
Note disallow_reorg_within_first_proposal_burn_block_timing_secs_but_more_than_one_block_scenario will be flaky until #6221 is fixed