-
Notifications
You must be signed in to change notification settings - Fork 698
Feat/signer two phase commit impl #6319
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 two phase commit impl #6319
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]>
…e throughout Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
… into feat/signer-two-phase-commit-impl
… into feat/signer-two-phase-commit-impl
… into feat/signer-two-phase-commit-impl
Signed-off-by: Jacinta Ferrant <[email protected]>
… into feat/signer-two-phase-commit-impl
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.
LGTM! Just a few nits. I found your logic very easy to follow
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
…lock Signed-off-by: Jacinta Ferrant <[email protected]>
bd04283
to
e304a48
Compare
… than Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
… into feat/signer-two-phase-commit-impl
Signed-off-by: Jacinta Ferrant <[email protected]>
… wait for block commit at correct height Signed-off-by: Jacinta Ferrant <[email protected]>
…n block Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
d46d4ec
to
0375c72
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.
Overall LGTM! Just one tiny nit about some debug logs
Signed-off-by: Jacinta Ferrant <[email protected]>
… into feat/signer-two-phase-commit-impl
Signed-off-by: Jacinta Ferrant <[email protected]>
stacks-signer/src/v0/signer.rs
Outdated
let block_response = self.create_block_acceptance(&block_info.block); | ||
self.send_block_pre_commit(signer_signature_hash); | ||
// have to save the signature _after_ the block info | ||
self.handle_block_signature(stacks_client, block_response.as_block_accepted()?); | ||
Some(block_response) | ||
let address = self.stacks_address; | ||
self.handle_block_pre_commit(stacks_client, &address, &signer_signature_hash); |
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 behavior has to be gated by the signer protocol version, right? Otherwise, when signers upgrade, they won't sign blocks until 70% of signers upgrade.
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.
They still work with unupgraded signers. They just treat signatures as pre commits if they haven't seen a pre commit from the signer yet. (I have tested this exact scenario in a docker container successfully)
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.
It does mean though that upgraded signers send a message that unupgraded signers ignore. Eventualyl though upgraded signers see 70% weight signatures (which they have treated as commits) and issue their signature which unupgraded signers accept no problem.
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.
Added an integration test to demonstrate how it works with unupgraded vs upgraded signers: see a584982
… into feat/signer-two-phase-commit-impl
Signed-off-by: Jacinta Ferrant <[email protected]>
Closes #6099