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] - refuse old proposals from gossip #5020

Closed
wants to merge 4 commits into from

Conversation

countvonzero
Copy link
Contributor

@countvonzero countvonzero commented Sep 15, 2023

Motivation

there are a lot of old proposals floating in the gossip network.
deleting old proposals in #4993 forced the node to process/save these old proposals repeatedly

Changes

  • refuse proposals older than current layer
  • add metrics/log for time it takes to delete proposals

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #5020 (c03098f) into develop (306c43a) will decrease coverage by 0.1%.
Report is 1 commits behind head on develop.
The diff coverage is 85.7%.

@@            Coverage Diff            @@
##           develop   #5020     +/-   ##
=========================================
- Coverage     77.1%   77.1%   -0.1%     
=========================================
  Files          254     254             
  Lines        30350   30363     +13     
=========================================
+ Hits         23426   23436     +10     
- Misses        5405    5408      +3     
  Partials      1519    1519             
Files Changed Coverage Δ
blocks/metrics.go 65.2% <ø> (ø)
blocks/generator.go 90.2% <75.0%> (+1.5%) ⬆️
proposals/handler.go 87.2% <100.0%> (+0.2%) ⬆️
sql/proposals/proposals.go 79.7% <100.0%> (ø)

... and 1 file with indirect coverage changes

@@ -262,6 +262,11 @@ func (h *Handler) handleProposal(ctx context.Context, expHash types.Hash32, peer
preGenesis.Inc()
return fmt.Errorf("proposal before effective genesis: layer %v", p.Layer)
}
if p.Layer < h.clock.CurrentLayer() {
// old proposals have no use for the node
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this case the enclosed ballot should be fetched via sync, if not already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

another option is to save the ballot only

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about maintaining variable that is updated by block generator after it is done with this layer?

defaults to current layer on startup, but it will allow to stop accepting proposals earlier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a good idea. i chose to use mesh.ProcessedLayer() to be the threshold for the following reasons

  • it's updated every time a block is generated, optimistically or not
  • hare can terminate out of order, so block can be generated out of order as well in the non-optimistic case
  • proposal handler already have mesh interface ready to use

@@ -262,6 +262,11 @@ func (h *Handler) handleProposal(ctx context.Context, expHash types.Hash32, peer
preGenesis.Inc()
return fmt.Errorf("proposal before effective genesis: layer %v", p.Layer)
}
if p.Layer < h.clock.CurrentLayer() {
// old proposals have no use for the node
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about maintaining variable that is updated by block generator after it is done with this layer?

defaults to current layer on startup, but it will allow to stop accepting proposals earlier

@dshulyak
Copy link
Contributor

dshulyak commented Sep 16, 2023

btw i didn't notice that all old proposals are deleted during protocol runtime https://github.com/spacemeshos/go-spacemesh/blob/develop/sql/proposals/proposals.go#L190 . the function looks like that it is limited for the layer

this was visible in write metrics. so it was somewhat risky

image

@countvonzero
Copy link
Contributor Author

btw i didn't notice that all old proposals are deleted during protocol runtime https://github.com/spacemeshos/go-spacemesh/blob/develop/sql/proposals/proposals.go#L190 . the function looks like that it is limited for the layer

this was visible in write metrics. so it was somewhat risky

yes. this was risky. i made the following change

  • delete batch at startup (anything before the layer in state
  • make the delete async
  • renamed Delete to DeleteBefore
    note that the last layer is kept just in case peers want to sync proposals for latest layer due to clock skew.

@countvonzero
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Sep 16, 2023
@bors
Copy link

bors bot commented Sep 16, 2023

try

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.

@countvonzero
Copy link
Contributor Author

bors merge

bors bot pushed a commit that referenced this pull request Sep 16, 2023
## Motivation
there are a lot of old proposals floating in the gossip network.
deleting old proposals in #4993 forced the node to process/save these old proposals repeatedly

## Changes
- refuse proposals older than current layer
- add metrics/log for time it takes to delete proposals
@bors
Copy link

bors bot commented Sep 16, 2023

Build failed:

@countvonzero
Copy link
Contributor Author

bors merge

bors bot pushed a commit that referenced this pull request Sep 16, 2023
## Motivation
there are a lot of old proposals floating in the gossip network.
deleting old proposals in #4993 forced the node to process/save these old proposals repeatedly

## Changes
- refuse proposals older than current layer
- add metrics/log for time it takes to delete proposals
@bors
Copy link

bors bot commented Sep 16, 2023

Build failed:

@countvonzero
Copy link
Contributor Author

bors merge

bors bot pushed a commit that referenced this pull request Sep 16, 2023
## Motivation
there are a lot of old proposals floating in the gossip network.
deleting old proposals in #4993 forced the node to process/save these old proposals repeatedly

## Changes
- refuse proposals older than current layer
- add metrics/log for time it takes to delete proposals
@bors
Copy link

bors bot commented Sep 16, 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 refuse old proposals from gossip [Merged by Bors] - refuse old proposals from gossip Sep 16, 2023
@bors bors bot closed this Sep 16, 2023
@bors bors bot deleted the refuse-old-proposals branch September 16, 2023 20:22
countvonzero added a commit that referenced this pull request Sep 16, 2023
## Motivation
there are a lot of old proposals floating in the gossip network.
deleting old proposals in #4993 forced the node to process/save these old proposals repeatedly

## Changes
- refuse proposals older than current layer
- add metrics/log for time it takes to delete proposals
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.

None yet

2 participants