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

Ensure miner builds on its own blocks when reorging due to badly timed blocks #5691

Merged
merged 10 commits into from
Jan 22, 2025
Merged

Ensure miner builds on its own blocks when reorging due to badly timed blocks #5691

merged 10 commits into from
Jan 22, 2025
+537 −2

Conversation

obycode
Copy link
Contributor

@obycode obycode commented Jan 14, 2025

In this test, I attempted to reproduce the scenario we saw in mainnet, in which the miner mines a tenure change block in this reorg scenario, but then fails to mine another block building off of that one. I was unable to reproduce that behavior, but this still seems like a useful test to have.

@obycode obycode requested review from a team as code owners January 14, 2025 15:12
@obycode
Copy link
Contributor Author

obycode commented Jan 14, 2025

I may have an update to this test that does replicate the problem. Will push another commit after further review. Please hold off reviews for now.

@obycode
test: improvements to `allow_reorg_within_first_proposal_burn_block_t…
1607145 
…iming_secs`

This now seems to reproduce the problem that we saw on mainnet. The
problem persists in `develop`.
@aldur aldur added this to the 3.1.0.0.4 milestone Jan 14, 2025
@obycode
Copy link
Contributor Author

obycode commented Jan 14, 2025

I may have an update to this test that does replicate the problem. Will push another commit after further review. Please hold off reviews for now.

1607145 seems to reproduce this issue. The problem remains on develop.

@obycode
Copy link
Contributor Author

obycode commented Jan 14, 2025

This also still fails with #5515 applied.

@obycode
fix: ensure miner continues mining off its own block
978fddc 
Without this change, when mining a fork (caused by unfortunate timing),
the miner will never build off of its tenure change block. This change
resolves this issue.
@obycode obycode requested review from jcnelson and kantai January 14, 2025 20:09
@obycode
Copy link
Contributor Author

obycode commented Jan 14, 2025

I've adjusted the miner to pass this test, but it uses NakamotoChainState::get_highest_known_block_header_in_tenure which has the comment "DO NOT USE IN CONSENSUS CODE." I think it is okay in this scenario, but would like some more eyes on it.

@aldur aldur requested a review from rdeioris January 15, 2025 15:39
@obycode obycode requested a review from jcnelson January 15, 2025 22:48
@obycode obycode changed the title Add test for allowing reorg within first proposal burn block timing secs Ensure miner builds on its own blocks when reorging due to badly timed blocks Jan 16, 2025
kantai
kantai previously approved these changes Jan 17, 2025
View reviewed changes
@obycode
test: further improvements to integration test
994bc83 
Ensure that each block has the expected parent block in
`allow_reorg_within_first_proposal_burn_block_timing_secs`.
@obycode obycode requested review from kantai and jcnelson January 17, 2025 20:43
@obycode obycode requested a review from a team as a code owner January 18, 2025 13:47
@jferrant jferrant self-requested a review January 21, 2025 18:18
jferrant
jferrant previously approved these changes Jan 21, 2025
View reviewed changes
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.

LGTM I think. Does this mean though that if a miner was unlucky in its timing...it basically makes its own tenure invalid by reorging the last block but then is unable to actually build any more blocks?

@obycode
Copy link
Contributor Author

obycode commented Jan 21, 2025

LGTM I think. Does this mean though that if a miner was unlucky in its timing...it basically makes its own tenure invalid by reorging the last block but then is unable to actually build any more blocks?

That is currently the case, where it reorgs the previous tenure with a tenure change block, but is then unable to build any more blocks. This PR makes it so that it will still reorg its own last tenure, but now it will be able to build more blocks.

@obycode obycode requested a review from jferrant January 21, 2025 21:59
@obycode obycode added this pull request to the merge queue Jan 22, 2025
Merged via the queue into develop with commit 53e670f Jan 22, 2025
181 checks passed
@obycode obycode deleted the test/allow_reorg_within_first_proposal_burn_block_timing_secs branch January 22, 2025 16:09
@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 Jan 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@moodmosaic moodmosaic moodmosaic left review comments

@kantai kantai kantai approved these changes

@jcnelson jcnelson jcnelson approved these changes

@rdeioris rdeioris Awaiting requested review from rdeioris

@jferrant jferrant Awaiting requested review from jferrant

Assignees

@obycode obycode

Labels
locked
Projects
Stacks Core Eng
Status: Status: ✅ Done
Milestone
3.1.0.0.4
Development

Successfully merging this pull request may close these issues.
7 participants
@obycode @blockstack-devops @moodmosaic @jcnelson @kantai @jferrant @aldur