-
Notifications
You must be signed in to change notification settings - Fork 678
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
jcnelson
wants to merge
55
commits into
develop
Choose a base branch
from
fix/burn-view
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,366
−386
Open
Changes from 39 commits
Commits
Show all changes
55 commits
Select commit
Hold shift + click to select a range
9f3f197
Merge branch 'hotfix/proposal-loads-sortition-view-from-block' into f…
jcnelson 91d0f96
chore: use get_block_burn_view()
jcnelson b0ebc18
Merge branch 'develop' into fix/burn-view
jcnelson 0d7fbfa
Merge branch 'develop' into fix/burn-view
jcnelson c9f72e4
chore: add new integration test
jcnelson 8533269
chore: make `MinerReason` debug-printable, and factor out fault injec…
jcnelson 2f16742
fix: consider the possibility that the miner can neither begin a new …
jcnelson 3b81155
chore: track the number of miner directives
jcnelson 08fa52a
chore: integration test to verify that a continue-tenure might not be…
jcnelson 0996fb1
Merge branch 'develop' into fix/burn-view
jcnelson b110f66
chore: more fixes to differentiate the miner's burn view from the bur…
jcnelson 2d93f24
Merge branch 'fix/burn-view' of https://github.com/stacks-network/sta…
jcnelson 9b53d70
chore: more checks on burn view changes
jcnelson c8974b2
Merge branch 'feat/time-based-tenure-extend' into fix/burn-view
jcnelson 8be6f90
Merge branch 'feat/time-based-tenure-extend' into fix/burn-view
jcnelson 22a9815
Merge branch 'develop' into fix/burn-view
jcnelson 4c9155b
Cargo fmt
jferrant eb62628
chore: record last sortition
jcnelson 93cf523
chore: remove EmptyTenure miner reason, since it shouldn't ever be used
jcnelson a7a0b19
chore: factor logic for checking for a tenure-extend into a single fu…
jcnelson a2f010e
chore; drop needs_initial_block
jcnelson 48e7468
test: finish check that the hotfix ensures that the correct burn view…
jcnelson f488b35
chore: delete old code
jcnelson 73821ad
Merge branch 'fix/burn-view' of https://github.com/stacks-network/sta…
jcnelson 818c064
Merge branch 'develop' into fix/burn-view
jcnelson 06e2764
chore: clean up compile error and warnings
jcnelson fd28d81
Merge branch 'fix/burn-view' of https://github.com/stacks-network/sta…
jcnelson 7abaaca
feat: tenure_extend_wait_secs: a config option to wait for a block-fo…
jcnelson 06096ee
fix: allow a BlockFound to be produced if the Relayer determines that…
jcnelson 7631f41
chore: fix choose_miner_directive() to attempt to continue a tenure i…
jcnelson 0f7ada4
chore: fix tests
jcnelson 6b18429
chore: expect a TenureExtend for a flash block
jcnelson 1819f2a
Merge branch 'develop' into fix/burn-view
jcnelson f146ade
Merge branch 'develop' into fix/burn-view
jcnelson 70833cd
refactor: use `TestFlag` for more flags
obycode 6fe5d2d
fix: pause Stacks mining while mining blocks for miner eligibility
obycode 911560c
test: add wait to ensure tip has advanced
obycode 1c31090
test: add new test for tenure extend
obycode 24193f8
Merge branch 'develop' into fix/burn-view
jcnelson 9de3f84
fix: `won_sortition` calculation in relayer
obycode 5590ec0
chore: get tenure_extend_after_failed_miner to pass
jcnelson 45028e4
Merge branch 'fix/burn-view' of https://github.com/stacks-network/sta…
jcnelson 27519c3
chore: expand test_tenure_extend_from_flashblocks to check that all b…
jcnelson 17d6edc
fix: build issue; fix relayer to always start a new tenure if the cur…
jcnelson 99d3eff
test: change VRF proof calculation to test a comment from @obycode
jcnelson 49d5d65
Merge branch 'develop' into fix/burn-view
jcnelson 262ee7d
chore: revert to LazyStatic
jcnelson 792fcfc
Merge branch 'develop' into fix/burn-view
jcnelson 62c9f13
chore: add docstrings, and (to test) disable the check_burn_view_chan…
jcnelson cc5e2fd
Merge branch 'develop' into fix/burn-view
jcnelson 618c3a0
chore: cargo fmt
jcnelson fa823b1
test: disable check_burn_view_changed()
jcnelson a6a42cf
Merge branch 'develop' into fix/burn-view
jcnelson 8e9303a
fix: remove compile warnings that prevent CI from running
jcnelson 5c2cc18
Merge branch 'develop' into fix/burn-view
jcnelson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 seems pretty aggressive to me. I think what this means is that miners will attempt to extend even if the next sortition has a valid winner after 30 seconds. What percentage of tenures mine their first block within this time period (and have the block processed by a follower node)?
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.
The relevant value in the signer set defaults to 600 seconds: https://github.com/stacks-network/stacks-core/blob/master/stacks-signer/src/config.rs#L37
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.
Ah right, sorry for the confusion earlier. I was thinking
first_proposal_burn_block_timing_secs
was the relevant config here, which defaults to 60s, but you're right.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.
I'm happy to change the timeout, but it doesn't prevent dueling miners from arising. It's possible that in
$TIMEOUT + 1
seconds, the winning miner comes online and signers reject it.We all good with 600 seconds?
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.
My preference would be to remove this behavior from this PR, and then we should start an issue describing this behavior, and we can go from there. I have concerns about he particular timeout chosen, but maybe other concerns as well, and I think its worth separating this behavior from the fix that this PR is trying to address.
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.
To clarify, which behavior do you want to remove from the PR?