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

Allow Initial Sync to Work with Simulator #669

Merged
merged 62 commits into from
Nov 21, 2018

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Oct 17, 2018

This would allow initial sync to function with a simulator. By allowing the simulator to respond to requests by block slot number instead of block hash it will allow a node to be able to sync up to the latest chain head. This should also help to close out #661.

  • Modify simulator so that it responds to requests by slot number.
  • Modify the p2p message sent by the simulator, so that it can be used by either a node performing normal sync or initial sync
  • Add tests

@nisdas nisdas added the Priority: High High priority item label Oct 17, 2018
@nisdas nisdas added this to the Ruby - v0.1 milestone Oct 17, 2018
@nisdas nisdas self-assigned this Oct 17, 2018
@nisdas nisdas added In Progress Priority: Medium Medium priority item and removed Priority: High High priority item labels Oct 17, 2018
@codecov
Copy link

codecov bot commented Oct 20, 2018

Codecov Report

Merging #669 into master will decrease coverage by 0.37%.
The diff coverage is 68.25%.

@@            Coverage Diff             @@
##           master     #669      +/-   ##
==========================================
- Coverage   73.59%   73.22%   -0.38%     
==========================================
  Files          67       69       +2     
  Lines        4367     4575     +208     
==========================================
+ Hits         3214     3350     +136     
- Misses        835      895      +60     
- Partials      318      330      +12

@nisdas nisdas added Priority: High High priority item and removed Priority: Medium Medium priority item labels Nov 8, 2018
@terencechain
Copy link
Member

@nisdas sorry there are a few more issues.

Looking at the run log from your last commit, BlockChain service never ran. After slot 5, it's only p2p service and simulator.
Log: https://gist.github.com/terenc3t/bc3f6dd89b321a09d9aa1af1eefb5e57

Here is the normal log:
https://gist.github.com/terenc3t/0374442ec7b48ab9f5100590fbad7f7c

@prestonvanloon
Copy link
Member

Blocked by #787

@prestonvanloon
Copy link
Member

Lint issues:


beacon-chain/simulator/service.go:271:26: error strings should not be capitalized or end with punctuation or a newline
--
  | beacon-chain/simulator/service.go:276:26: error strings should not be capitalized or end with punctuation or a newline
  | beacon-chain/simulator/service.go:281:26: error strings should not be capitalized or end with punctuation or a newline
  | beacon-chain/simulator/service.go:286:26: error strings should not be capitalized or end with punctuation or a newline


Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

Lgtm

@@ -44,8 +46,9 @@ type Config struct {
// CrystallizedStateBufferSize determines the buffer size of thhe `crystallizedStateBuf` channel.
func DefaultConfig() Config {
return Config{
SyncPollingInterval: 1 * time.Second,
SyncPollingInterval: time.Duration(int64(params.BeaconConfig().SlotDuration)) * time.Second * 4,
Copy link
Member

Choose a reason for hiding this comment

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

What is this constant 4? Can we predefined this constant?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed into a config var

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High High priority item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants