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

Fix: correct burn view for miner block broadcast #5515

Open
wants to merge 39 commits into
base: develop
Choose a base branch
from

Conversation

jcnelson
Copy link
Member

This fixes the burn view calculation for the Nakamoto miner when accepting and broadcasting a block.

It builds atop #5508, so don't bother merging until #5508 is in develop.

@jcnelson jcnelson requested a review from a team as a code owner November 27, 2024 20:15
@aldur
Copy link
Contributor

aldur commented Dec 3, 2024

  1. Merge hotfix: remove chain stall race condition #5508 to master
  2. After release merge master into develop.
  3. Merge this to develop.

jferrant
jferrant previously approved these changes Dec 4, 2024
@jcnelson
Copy link
Member Author

jcnelson commented Dec 5, 2024

This now contains the hotfix to the miner @obycode @jferrant

/// sortition, make sure we first mine a tenure-change block and then a tenure-extend block.
#[test]
#[ignore]
fn test_tenure_change_and_extend_from_flashblocks() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am prob just misunderstanding the test, but I don't see any confirmation about a tenure change/extend being issued in the correct order in this test. Is the fact the follower boots up and the chain advances proof enough?

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.

Just a few questions.

@aldur aldur added this to the 3.1.0.0.2 milestone Dec 11, 2024
@jcnelson jcnelson requested a review from a team as a code owner December 15, 2024 04:48
@@ -960,6 +980,7 @@ impl BlockMinerThread {
self.burn_election_block.sortition_hash.as_bytes(),
)
} else {
// TODO: shouldn't this be self.burn_block.sortition_hash?
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's resolve this TODO in this PR. I'm not immediately sure what the correct answer is.

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.

This looks good. Just had some minor comments. I want to run this through some more live network testing before feeling 100% confident.

testnet/stacks-node/src/nakamoto_node/relayer.rs Outdated Show resolved Hide resolved
testnet/stacks-node/src/nakamoto_node/relayer.rs Outdated Show resolved Hide resolved
testnet/stacks-node/src/nakamoto_node/relayer.rs Outdated Show resolved Hide resolved
jcnelson and others added 7 commits December 20, 2024 14:47
…und before extending the ongoing tenure if the miner produced the ongoing tenure
… the miner is "late" in doing so -- e.g. because a flashblock arrived
…f the miner produced the ongoing tenure (but only after a deadline), and to stop the tenure thread if the miner did not win sortition (even if continuation is later possible). If continuation is possible, then start a continuation thread if the ongoing tenure is still active.
@jferrant
Copy link
Collaborator

jferrant commented Jan 8, 2025

We should run this against tenure_extend_after_failed_miner. In theory the signer side is already done and this ticket should enable it to pass.
See #5361 (comment)

@@ -489,6 +504,7 @@ impl BlockMinerThread {
// update mined-block counters and mined-tenure counters
self.globals.counters.bump_naka_mined_blocks();
if self.last_block_mined.is_some() {
// TODO: reviewers: should this be .is_none()?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think it should be is_none().

@obycode
Copy link
Contributor

obycode commented Jan 10, 2025

The flakiness in develop has been resolved on miner_forking. I have verified that the problem here is definitely related to these new changes.

@jcnelson
Copy link
Member Author

Working on fixing tenure_extend_after_failed_miner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: In Review
Development

Successfully merging this pull request may close these issues.

4 participants