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

[Merged by Bors] - use atx grading for block proposal #4717

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - use atx grading for block proposal #4717

wants to merge 4 commits into from

Conversation

countvonzero
Copy link
Contributor

Motivation

part of #4089

Changes

  • grade atxs as described in https://community.spacemesh.io/t/grading-atxs-for-the-active-set/335, let s be the start of the epoch, and δ the network propagation time.

    • grade 0/evil: ATX was received at time t >= s-3δ, or an equivocation proof was received by time s-δ.
    • grade 1/acceptable: ATX was received at time t < s-3δ before the start of the epoch, and no equivocation proof was received by time s-δ.
    • grade 2/good: ATX was received at time t < s-4δ, and no equivocation proof was received for that id until time s.

  • miner only include grade 2/good ATXs in its active set for its first ballot of the epoch

  • newly sync'ed node uses the first block of epoch for active set in ref ballot, since it doesn't have accurate received timestamp for atxs or malfeasance proof

@countvonzero countvonzero mentioned this pull request Jul 17, 2023
Closed
@codecov
Copy link

codecov bot commented Jul 17, 2023 •
edited

Codecov Report

Merging #4717 (5cac034) into develop (21b4768) will increase coverage by 0.0%.
The diff coverage is 83.7%.

@@           Coverage Diff           @@
##           develop   #4717   +/-   ##
=======================================
  Coverage     77.1%   77.2%           
=======================================
  Files          255     257    +2     
  Lines        28886   28975   +89     
=======================================
+ Hits         22294   22371   +77     
- Misses        5200    5211   +11     
- Partials      1392    1393    +1
Files Changed Coverage Δ
sql/atxs/atxs.go 83.5% <ø> (ø)
syncer/syncer.go 92.8% <0.0%> (-0.6%) ⬇️
datastore/store.go 69.9% <60.0%> (ø)
miner/util.go 71.4% <71.4%> (ø)
miner/atx_grader.go 75.0% <75.0%> (ø)
miner/oracle.go 73.9% <86.9%> (+5.4%) ⬆️
beacon/beacon.go 79.5% <100.0%> (ø)
hare/eligibility/oracle.go 73.3% <100.0%> (+0.5%) ⬆️
miner/proposal_builder.go 84.0% <100.0%> (+0.1%) ⬆️
node/node.go 65.5% <100.0%> (+<0.1%) ⬆️
... and 2 more

... and 4 files with indirect coverage changes

fasmat
fasmat reviewed Jul 25, 2023
View reviewed changes
miner/interface.go Show resolved Hide resolved
miner/util.go
"github.com/spacemeshos/go-spacemesh/sql/certificates"
)

func ActiveSetFromBlock(db sql.Executor, epoch types.EpochID) ([]types.ATXID, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the naming doesn't fit. It generates the ActiveSet from the first block in the provided epoch. Maybe EpochActiveSet or similar is a better name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EpochActiveSet means many things. it can mean the active set is derived from

  • hare active set (determined by hare rules)
  • tortoise active set (all epoch atx ids)
  • block active set (derived from block)
  • fallback (overwritten manually)

i think the naming fits here.

Copy link
Member

Choose a reason for hiding this comment

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

It is confusing to me because instead of passing in a Block ID, I have to pass in an Epoch ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i guess i can rename it to ActiveSetFromFirstBlockInEpoch?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I cannot think of a better name and you already stated that EpochActiveSet would be confusing for other reasons.

miner/util.go
"github.com/spacemeshos/go-spacemesh/sql/certificates"
)

func ActiveSetFromBlock(db sql.Executor, epoch types.EpochID) ([]types.ATXID, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to have a dedicated test for this function? It is indirectly covered by other tests, but maybe crucial enough to have a few test cases directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#4757

datastore/store.go Show resolved Hide resolved
miner/oracle_test.go Show resolved Hide resolved
fasmat
fasmat approved these changes Jul 25, 2023
View reviewed changes
Copy link
Member

@fasmat fasmat left a comment

Choose a reason for hiding this comment

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

LGTM.

@countvonzero countvonzero mentioned this pull request Jul 31, 2023
Closed
@countvonzero
Copy link
Contributor Author

bors merge

@countvonzero
Copy link
Contributor Author

bors merge

1 similar comment
@countvonzero
Copy link
Contributor Author

bors merge

bors bot pushed a commit that referenced this pull request Jul 31, 2023
@countvonzero
use atx grading for block proposal (#4717)
a975106 
## Motivation
<!-- Please mention the issue fixed by this PR or detailed motivation -->
part of #4089
<!-- `Closes #XXXX, closes #XXXX, ...` links mentioned issues to this PR and automatically closes them when this it's merged -->

## Changes
- grade atxs as described in https://community.spacemesh.io/t/grading-atxs-for-the-active-set/335, let s be the start of the epoch, and δ the network propagation time.

  - grade 0/evil: ATX was received at time t >= s-3δ, or an equivocation proof was received by time s-δ.
  - grade 1/acceptable: ATX was received at time t < s-3δ before the start of the epoch, and no equivocation proof was received by time s-δ.
  - grade 2/good: ATX was received at time t < s-4δ, and no equivocation proof was received for that id until time s.

- miner only include grade 2/good ATXs in its active set for its first ballot of the epoch

- newly sync'ed node uses the first block of epoch for active set in ref ballot, since it doesn't have accurate received timestamp for atxs or malfeasance proof
@bors
Copy link

bors bot commented Jul 31, 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 atx grading for block proposal [Merged by Bors] - use atx grading for block proposal Jul 31, 2023
@bors bors bot closed this Jul 31, 2023
@countvonzero countvonzero mentioned this pull request Aug 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fasmat fasmat fasmat approved these changes

@dshulyak dshulyak dshulyak approved these changes

@poszu poszu Awaiting requested review from poszu poszu is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@countvonzero @dshulyak @fasmat