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

Miner can continue mining after an empty tenure followed by empty sortition #5411

Merged
merged 21 commits into from
Nov 8, 2024

Conversation

hstove
Copy link
Contributor

@hstove hstove commented Oct 31, 2024

(hopefully eventually)

This PR currently just contains an integration test to try and reproduce the scenario described in #5400.

The basic flow of the test is:

  • boot to nakamoto
  • mine a regular block (A) (burn block with TenureChange)
  • Have signers reject all new block proposals
  • Mine a new BTC block (B) with a sortition
  • Wait for the signers to reject it
  • Mine a new BTC block (C) without a sortition
  • Turn off the block rejection flag

I thought this would reproduce the issue, but things seem fine - the miner creates a TenureExtend off of block A and signers approve it.

JACINTA EDIT:

So I modified the logic to check during continue_tenure to see if we failed to produce any blocks in our tenure which means we really never issued a tenure extend. Now we issue one. I verified this works with multiple empty sortitions, and that we can build off this tenure change payload that shows up late essentially. I also ensured that we do issue a traditional tenure extend in the case where we successfully issued the tenure extend payload. This did require me to chagne the vrf proof generation to use the block election snapshot and not the current burn block to verify. I also found a bug in the way we were passing the election snapshot along (caused miners to never issue a block proposal at all because it would attempt to write to the wrong slot).

Now I know @kantai you wanted the miner to issue a continue tenure after the successful tenure change but I think this is a lot more complicated and has more edge cases that I don't like...Now I can continue to try this though if you think this fix is insufficient / or if there are edge cases that already exist that I am just not seeing.

Will see what CI does...hopefully haven't broken anything

@jferrant
Copy link
Collaborator

I think you might need to have 2 sep miners to get this to work. We need to

  • boot to nakamoto
  • Miner A mines a regular block A (burn block with TenureChange)
  • Mine some stacks blocks ontop of Tenure A
  • Turn on all block rejections
  • Pause commits from Miner A
  • Make Miner B win Tenure B
  • Miner B should fail to mine anything
  • Pause commits from Miner B
  • Mine an EMPTY burn block (NO COMMITS)
  • Turn off the block rejections
  • MINER B SHOULD ATTEMPT TO TENURE EXTEND AND SOMEHOW SUCCEED (this is where we might need to make changes to the code to ensure it does do that.)

@hstove
Copy link
Contributor Author

hstove commented Nov 1, 2024

Yeah you may be right about needing 2 miners, but I swear this situation involved the same miner winning the first two blocks (though I should double check). I think it's also very possible that there was a different "root cause" in that case.

@jferrant jferrant force-pushed the fix/fast-block-test branch from 343bdee to cf1d13c Compare November 6, 2024 23:22
…. Not the tip consensus hash as it may not match and will result in a bad signer slot generation when writing block proposal

Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
@jferrant jferrant marked this pull request as ready for review November 7, 2024 02:42
@jferrant jferrant requested review from a team as code owners November 7, 2024 02:42
@jferrant jferrant changed the title Integration test for #5400 Integration test and fix to ensure a miner can continue mining after fast blocks cause an empty tenure followed by empty sortition Nov 7, 2024
@jferrant
Copy link
Collaborator

jferrant commented Nov 7, 2024

Yeah you may be right about needing 2 miners, but I swear this situation involved the same miner winning the first two blocks (though I should double check). I think it's also very possible that there was a different "root cause" in that case.

It could have been done with one miner . It just was not happening because we were failing to produce an empty tenure as easily in the one miner case because of the boot to nakamoto logic. it def can be done. Having two miners makes it just a bit easier and allows a more expansive test as well :)

@hstove
Copy link
Contributor Author

hstove commented Nov 7, 2024

Nice, this is looking pretty good!

@kantai kantai changed the title Integration test and fix to ensure a miner can continue mining after fast blocks cause an empty tenure followed by empty sortition Miner can continue mining after an empty tenure followed by empty sortition Nov 7, 2024
…ock hash to form the parent_tenure_start

Signed-off-by: Jacinta Ferrant <[email protected]>
@jferrant jferrant requested review from kantai and jcnelson November 8, 2024 00:03
Signed-off-by: Jacinta Ferrant <[email protected]>
@jferrant jferrant self-requested a review November 8, 2024 17:26
kantai
kantai previously approved these changes Nov 8, 2024
Fix miner forking by being strict about sortition winners
jcnelson
jcnelson previously approved these changes Nov 8, 2024
@jferrant jferrant requested a review from kantai November 8, 2024 17:57
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

@jferrant jferrant added this pull request to the merge queue Nov 8, 2024
Merged via the queue into develop with commit 7224b9b Nov 8, 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 Nov 16, 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.

6 participants