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

Nakamoto: Allow miner to reorg in poorly timed tenures #4956

Merged
merged 7 commits into from
Jul 12, 2024

Conversation

kantai
Copy link
Member

@kantai kantai commented Jul 10, 2024

This implements a fix for #4844, by adding a check to the signer's chainstate validation checks. This check allows reorgs to occur in the case that the reorged tenure's first block occurred very near the time that the next sortition occurred.

Currently, this check compares the proposal's arrival time with the time that the signer learned about the next sortition. This is a pretty conservative duration -- something more aggressive would compare the proposals "sign off" time with the sortition's arrival time. I'm not sure which approach would be better actually, so this PR just implements the simplest one for now.

This is tested via two new unit tests in the stacks-signer package.

@kantai kantai requested review from a team as code owners July 10, 2024 20:34
* track sortition timing
* track proposal timing
* make proposal / sortition timing configurable
Copy link
Collaborator

@jferrant jferrant left a comment

Choose a reason for hiding this comment

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

Is it possible to get an integration test that checks this reorg? I think I only see the local chainstate tests.

@kantai kantai requested a review from a team as a code owner July 11, 2024 21:45
@kantai
Copy link
Member Author

kantai commented Jul 12, 2024

Is it possible to get an integration test that checks this reorg? I think I only see the local chainstate tests.

Yep -- added 2 integration tests in 62aac67 which test the reorg handling. The reason there's two integration tests is that I want to test both the behavior when a reorg should be allowed and when it shouldn't. To do that, I change the configured allowable time between the proposal being approved and the next sortition. It would be possible to do this in a single test with just 1 setting of that configuration, but at that point, the test would become much more sensitive to timing, which would probably make it flap in CI.

obycode
obycode previously approved these changes Jul 12, 2024
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

LGTM!

obycode
obycode previously approved these changes Jul 12, 2024
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

👍

@obycode
Copy link
Contributor

obycode commented Jul 12, 2024

Before I approve again, looks like there are still some rust format issues to resolve.

@jcnelson jcnelson self-requested a review July 12, 2024 18:26
jcnelson
jcnelson previously approved these changes Jul 12, 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.

LGTM; just a nit about the DB schema

@kantai kantai requested a review from jcnelson July 12, 2024 19:49
@kantai kantai requested a review from a team as a code owner July 12, 2024 20:07
@kantai kantai enabled auto-merge July 12, 2024 20:08
@kantai kantai added this pull request to the merge queue Jul 12, 2024
Merged via the queue into develop with commit e70fe0a Jul 12, 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 29, 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