[R4R]supoort hot sync reactor #97
Conversation
unclezoro
force-pushed
the
hotreactor
branch
13 times, most recently
from
c8e4ebc
to
73072a9
Compare
unclezoro
requested review from
ackratos and
darren-liu
and removed request for
ackratos
unclezoro
requested review from
yutianwu,
rickyyangz,
ackratos,
abelliumnt and
notatestuser
| switch msg := msg.(type) { | ||
| case *blockSubscribeMessage: | ||
| hbcR.Logger.Debug("receive blockSubscribeMessage from peer", "peer", src.ID(), "from_height", msg.FromHeight, "to_height", msg.ToHeight) | ||
| hbcR.pool.handleSubscribeBlock(msg.FromHeight, msg.ToHeight, src) |
There was a problem hiding this comment.
the handleSubscribeBlock seems to be a costly operation, it may be an attack point.
also since currently the Receive method runs in sync mode and will block other msg handlers, we need to reconsider this.
There was a problem hiding this comment.
To avoid memory leak, only the subscribe height is small than currentHeight +maxPublishForesight will be accepted. When the height is small and load two blocks from db takes time, fast_sync have the same concern, only the p2p layer has a send rate limit to defend this.
fix peer lost issue fix metrics labels change parameters and log add more log try to answer even mute reduce redundant peers for candidate remove debug code do not deliver commit if miss blockId change sync pattern description avoid mix use of lock avoid timer leak decrease lock use fix current map read and write avoid message bust add more logs keep blockchain package stay fix import change default config fix review
| // the time interval to correct current state. | ||
| tryRepairInterval = 1 * time.Second | ||
|
|
||
| maxCachedSealedBlock = 100 |
There was a problem hiding this comment.
better make these configurable
There was a problem hiding this comment.
I rethink this, it looks like no impetus to change these configures, these configure can't affect something important. I hope to keep a thin config file.
|
|
||
| // most of the usage have locked. Only newBlockStateAtHeight do not, but the usage can't | ||
| // currently excuted with incCurrentHeight, it is safe. | ||
| func (pool *BlockPool) expectingHeight() int64 { |
There was a problem hiding this comment.
have lock where called expectingHeight
| hbcR.Logger.Info("hot sync switching to consensus sync") | ||
| conR, ok := hbcR.Switch.Reactor("CONSENSUS").(consensusReactor) | ||
| if ok { | ||
| hbcR.pool.SwitchToConsensusSync(nil) |
There was a problem hiding this comment.
why not pass in hbcR.pool.state and get rid of stateCopy within SwitchToConsensusSync function
There was a problem hiding this comment.
Because both fast sync reactor and hot sync reactor are possible to call SwitchToConsensusSync. Because state will be used by consensus reactor, hot sync reactor do not want to hold the same lock, so use copy.
| return pool.st | ||
| } | ||
|
|
||
| func (pool *BlockPool) verifyCommit(blockID types.BlockID, commit *types.Commit) error { |
There was a problem hiding this comment.
I suggest add a "Guarded" suffix to all methods in this file who is called with guardance of pool.mtx. For this method, I have to trace his 3 level parents to reason about it is correctly guarded.
There was a problem hiding this comment.
Add with "Guarded" prefix will decrease the original intention of the function. Even I add "Guarded" suffix, maybe the func is not locked by pool.mtx.
blockchain/hot/candidate.go
Outdated
| section = section + (1 - float64(m.average)/total) | ||
| diceSection = append(diceSection, section) | ||
| } | ||
| diceValue := rand.Float64() * diceSection[len(diceSection)-1] |
blockchain/hot/candidate.go
Outdated
| func newPeerMetrics() peerMetrics { | ||
| return peerMetrics{ | ||
| samples: list.New(), | ||
| sampleSequence: rand.Int63n(recalculateInterval), |
* add index recovery * fix import order * add err judge * fix review sugest * add default abciResponses * change indexHeight to indexHeightKey * remove debug log improve performance of IsPersistent trim p2p metrics add BroadcastFromNonePersistent config to mempool remove unnecessary metrics rename config fix testcase * fix review * change log * [R4R]supoort hot sync reactor (#97) * supoort hot reactor fix peer lost issue fix metrics labels change parameters and log add more log try to answer even mute reduce redundant peers for candidate remove debug code do not deliver commit if miss blockId change sync pattern description avoid mix use of lock avoid timer leak decrease lock use fix current map read and write avoid message bust add more logs keep blockchain package stay fix import change default config fix review * use a single goroutine for switch * fix review suggestion * fix test case * fix pick error * move to decayed once picked freshset * update peerstate * fix pool test case * change issue num of pending log * use random with seed * use seed random * prepare for release v0.31.5-binance.2 (#112)
Description
introduce a new block sync protocol to reduce network and CPU resources for full node/witness.
more detail in:
hotsync.pptx
Rationale
SyncPattern is the work pattern of BlockPool.
The viable transitions are:
Example
Under consensus pattern, the hight diff between validator and seed:
Under hotSYnc pattern, the hight diff between validator and seed:
Changes
Preflight checks
make build)make test)make integration_test)Already reviewed by
Related issues
... reference related issue #'s here ...