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

[Merged by Bors] - use first applied block in epoch for new miner active set #4997

Closed
wants to merge 3 commits into from

Conversation

countvonzero
Copy link
Contributor

@countvonzero countvonzero commented Sep 11, 2023

Motivation

Closes #4912

Changes

  • for new miner use the first applied block for ref ballot active set

@countvonzero countvonzero marked this pull request as draft September 11, 2023 21:24
@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #4997 (384e246) into develop (a1412f7) will increase coverage by 0.0%.
The diff coverage is 90.6%.

@@           Coverage Diff           @@
##           develop   #4997   +/-   ##
=======================================
  Coverage     77.1%   77.1%           
=======================================
  Files          254     254           
  Lines        30355   30385   +30     
=======================================
+ Hits         23430   23457   +27     
- Misses        5404    5405    +1     
- Partials      1521    1523    +2     
Files Changed Coverage Δ
sql/layers/layers.go 74.7% <86.3%> (+1.3%) ⬆️
hare/eligibility/oracle.go 75.0% <100.0%> (ø)
miner/util.go 79.3% <100.0%> (+7.8%) ⬆️

... and 3 files with indirect coverage changes

@countvonzero countvonzero changed the title FirstInEpoch: do not insist on cert to exist fallback on first applied block in epoch for new miner active set Sep 11, 2023
@countvonzero countvonzero marked this pull request as ready for review September 11, 2023 22:31
miner/util.go Outdated
}

func ActiveSetFromEpochFirstBlock(db sql.Executor, epoch types.EpochID) ([]types.ATXID, error) {
result, err := ActiveSetFromEpochFirstCertifiedBlock(db, epoch)
Copy link
Contributor

Choose a reason for hiding this comment

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

this would be a different block for synced node, i think it should be always first applied regardless of certificate

@dshulyak
Copy link
Contributor

i think it is this bug #4912

@countvonzero
Copy link
Contributor Author

i think it is this bug #4912

not aware of this. thanks!

@countvonzero countvonzero changed the title fallback on first applied block in epoch for new miner active set use first applied block in epoch for new miner active set Sep 12, 2023
@countvonzero
Copy link
Contributor Author

bors merge

bors bot pushed a commit that referenced this pull request Sep 12, 2023
## Motivation
Closes #4912 

## Changes
- for new miner use the first applied block for ref ballot active set
@bors
Copy link

bors bot commented Sep 12, 2023

Build failed:

@countvonzero
Copy link
Contributor Author

bors merge

bors bot pushed a commit that referenced this pull request Sep 13, 2023
## Motivation
Closes #4912 

## Changes
- for new miner use the first applied block for ref ballot active set
@bors
Copy link

bors bot commented Sep 13, 2023

Pull request successfully merged into develop.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title use first applied block in epoch for new miner active set [Merged by Bors] - use first applied block in epoch for new miner active set Sep 13, 2023
@bors bors bot closed this Sep 13, 2023
@bors bors bot deleted the relax-first-in-epoch branch September 13, 2023 02:54
bors bot pushed a commit that referenced this pull request Sep 14, 2023
## Motivation
follow up on #4997 doing the same for hare active set
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

miner.ActiveSetFromEpochFirstBlock will use different block depending on synced certificate
2 participants