Extract Initial Sync Into its Own Package #404
Conversation
Codecov Report
@@ Coverage Diff @@
## master #404 +/- ##
==========================================
+ Coverage 73.67% 74.56% +0.89%
==========================================
Files 29 30 +1
Lines 1979 2021 +42
==========================================
+ Hits 1458 1507 +49
+ Misses 371 360 -11
- Partials 150 154 +4
Continue to review full report at Codecov.
|
|
This is ready for review. Currently, Although a new node connected to a network would run |
There was a problem hiding this comment.
This is much cleaner and easier to understand now, thanks @rawfalafel !
| } | ||
|
|
||
| // ChainService is the interface for the blockchain package's ChainService struct | ||
| type ChainService interface { |
There was a problem hiding this comment.
Ah I see this was what you meant by defining small interfaces within the packages themselves instead of relying on a single interfaces.go file when we only need a subset of methods. I like this pattern and we can split up the large interfaces we have in the types package into something like this.
| SaveBlock(*types.Block) error | ||
| } | ||
|
|
||
| // SyncService is the interface for the Sync service |
There was a problem hiding this comment.
Punctuation on comments throughout this file and more descriptive godocs.
| for { | ||
| select { | ||
| case <-s.ctx.Done(): | ||
| log.Infof("Exiting goroutine") |
| if err != nil { | ||
| return err | ||
| } | ||
| log.WithField("Block received with hash", fmt.Sprintf("0x%x", h)).Debug("Crystallized state hash exists locally") |
There was a problem hiding this comment.
the field prefix is usually short, something like receivedBlockHash would be good
| } | ||
|
|
||
| if s.currentSlotNumber == uint64(0) { | ||
| return fmt.Errorf("invalid slot number for syncing") |
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| var log = logrus.WithField("prefix", "sync") |
There was a problem hiding this comment.
should this be "initialsync"?
var log = logrus.WithField("prefix", "initialsync")
| @@ -0,0 +1,265 @@ | |||
| package initialsync | |||
There was a problem hiding this comment.
can we add package level comment?
|
|
||
| s.initialCrystallizedStateHash = block.CrystallizedStateHash() | ||
|
|
||
| log.Infof("Saved block with hash 0%x for initial sync", h) |
There was a problem hiding this comment.
is 0 a typo? or should it be 0x?
rauljordan
changed the title
|
|
||
| var log = logrus.WithField("prefix", "sync") | ||
|
|
||
| // Config allows the channel's buffer sizes to be changed. |
There was a problem hiding this comment.
Not sure what this config has to do with buffer sizes
| ) *InitialSync { | ||
| ctx, cancel := context.WithCancel(ctx) | ||
|
|
||
| blockBuf := make(chan p2p.Message) |
There was a problem hiding this comment.
shouldn't these channels be buffered ?
|
|
||
| if stored { | ||
| // TODO: Bail out of the sync service if the chain is only partially synced | ||
| log.Infof("chain state detected, exiting initial sync") |
There was a problem hiding this comment.
First letter should be capitalized for logs
|
Hey @rawfalafel update on this PR? |
|
Going to address comments this afternoon!
2018年8月20日(月) 11:43 Raul Jordan <notifications@github.com>:
… Hey @rawfalafel <https://github.com/rawfalafel> update on this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#404 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMVrcAdaCj9ZsNKCDamEDIi7S40D12Xks5uStkMgaJpZM4V-yaM>
.
|
|
ready for another look |
This addresses #388
Once
InitialSyncServiceis finished, it callsStartonSyncService. Note thatStartfor each sync service checks the state of the chain to decide whether or not to start the goroutine.