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] - sync, blocks: request blocks when tortoise crossed local threshold #4826

Closed
wants to merge 4 commits into from

Conversation

dshulyak
Copy link
Contributor

@dshulyak dshulyak commented Aug 12, 2023

closes: #4824

  • first improvement is to register peers that supported blocks, otherwise we will be guessing from whom to request the block
  • and when mesh is called by sync or block builder it may send request to fetch blocks from registered peers. previously it would do so only in sync code path, which is less robust and might be delayed by fork finder

@dshulyak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Aug 12, 2023
@dshulyak dshulyak changed the title sync, blocks: request blocks syncing when tortoise crossed local threshold sync, blocks: request blocks when tortoise crossed local threshold Aug 12, 2023
@bors
Copy link

bors bot commented Aug 12, 2023

try

Build failed:

@codecov
Copy link

codecov bot commented Aug 12, 2023

Codecov Report

Merging #4826 (ab8f892) into develop (fd9046c) will decrease coverage by 0.1%.
Report is 1 commits behind head on develop.
The diff coverage is 96.0%.

@@            Coverage Diff            @@
##           develop   #4826     +/-   ##
=========================================
- Coverage     76.9%   76.9%   -0.1%     
=========================================
  Files          261     261             
  Lines        29621   29637     +16     
=========================================
+ Hits         22783   22792      +9     
- Misses        5392    5394      +2     
- Partials      1446    1451      +5     
Files Changed Coverage Δ
syncer/state_syncer.go 77.4% <0.0%> (-2.0%) ⬇️
mesh/mesh.go 67.0% <85.7%> (-1.2%) ⬇️
node/node.go 65.5% <100.0%> (+<0.1%) ⬆️
proposals/handler.go 86.9% <100.0%> (+0.1%) ⬆️
syncer/blockssync/blocks.go 100.0% <100.0%> (ø)

... and 7 files with indirect coverage changes

@dshulyak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Aug 12, 2023
@bors
Copy link

bors bot commented Aug 12, 2023

try

Build failed:

@dshulyak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Aug 12, 2023
@bors
Copy link

bors bot commented Aug 12, 2023

try

Build failed:

@dshulyak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Aug 12, 2023
@bors
Copy link

bors bot commented Aug 12, 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.

@dshulyak dshulyak marked this pull request as ready for review August 12, 2023 15:59
@dshulyak
Copy link
Contributor Author

@countvonzero need to add a test, but please take a look. i think it is more robust than the previous version

@dshulyak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Aug 12, 2023
@bors
Copy link

bors bot commented Aug 12, 2023

try

Build failed:

@dshulyak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Aug 12, 2023
@bors
Copy link

bors bot commented Aug 12, 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.

syncer/blockssync/blocks.go Show resolved Hide resolved
return &types.ErrorMissing{MissingData: types.MissingData{Blocks: missing}}
select {
case <-ctx.Done():
case msh.missingBlocks <- missing:
Copy link
Contributor

Choose a reason for hiding this comment

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

this approach still relies on ProcessLayer being triggered by some component.

currently there are two components (proposal builder and state syncer) that calls TallyVotes(), which cause the tortoise to verify a layer. only state syncer will call ProcessLayer and cause the block to be fetched.

is it possible to tie the block fetching with TallyVote instead? tho that may delay generating block proposals.
or maybe TallyVote() should be called with OnBallot() (too expensive?) and cause the blocks to be fetched more frequently?

Copy link
Contributor Author

@dshulyak dshulyak Aug 13, 2023

Choose a reason for hiding this comment

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

it is also called by block builder (check ProcessPerHareOutput), i would prefer not to add it to the tortoise. maybe at some point when i remove ProcessLayer alltogether

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it possible to tie the block fetching with TallyVote instead? tho that may delay generating block proposals.
or maybe TallyVote() should be called with OnBallot() (too expensive?) and cause the blocks to be fetched more frequently?

it makes sense to tie with local threshold, it is bigger change though. and this is needed semi-urgently

@dshulyak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Aug 13, 2023
@bors
Copy link

bors bot commented Aug 13, 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.

@dshulyak
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Aug 13, 2023
@bors
Copy link

bors bot commented Aug 13, 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.

@dshulyak
Copy link
Contributor Author

bors merge

bors bot pushed a commit that referenced this pull request Aug 13, 2023
…4826)

closes: #4824

- first improvement is to register peers that supported blocks, otherwise we will be guessing from whom to request the block
- and when mesh is called by sync or block builder it may send request to fetch blocks from registered peers. previously it would do so only in sync code path, which is less robust and might be delayed by fork finder
@bors
Copy link

bors bot commented Aug 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 sync, blocks: request blocks when tortoise crossed local threshold [Merged by Bors] - sync, blocks: request blocks when tortoise crossed local threshold Aug 13, 2023
@bors bors bot closed this Aug 13, 2023
dshulyak added a commit that referenced this pull request Aug 13, 2023
…4826)

closes: #4824

- first improvement is to register peers that supported blocks, otherwise we will be guessing from whom to request the block
- and when mesh is called by sync or block builder it may send request to fetch blocks from registered peers. previously it would do so only in sync code path, which is less robust and might be delayed by fork finder
brunovale91 pushed a commit to swarmbit/go-spacemesh that referenced this pull request Aug 21, 2023
…pacemeshos#4826)

closes: spacemeshos#4824

- first improvement is to register peers that supported blocks, otherwise we will be guessing from whom to request the block
- and when mesh is called by sync or block builder it may send request to fetch blocks from registered peers. previously it would do so only in sync code path, which is less robust and might be delayed by fork finder
brunovale91 pushed a commit to swarmbit/go-spacemesh that referenced this pull request Aug 21, 2023
…pacemeshos#4826)

closes: spacemeshos#4824

- first improvement is to register peers that supported blocks, otherwise we will be guessing from whom to request the block
- and when mesh is called by sync or block builder it may send request to fetch blocks from registered peers. previously it would do so only in sync code path, which is less robust and might be delayed by fork finder
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.

persistent votes against blocks that weren't build locally
2 participants