[blockchain] v2-riri Scheduler implementation by brapse · Pull Request #3821 · tendermint/tendermint

brapse · 2019-07-19T15:18:44Z

The blockchain reactor is responsible for fetching and applying blocks from peers in order to catch up a node. #3777 specifies a new architecture architecture which includes a scheduler component which:

Decouples scheduling from IO
strictly deterministic business logic
Uses explicit time instead of timers

This PR is one such scheduler modeled as a block and peer wise finite state machine. Blocks transition from:
unknown -> new -> pending -> received -> processed
and peers transition from:
new -> ready -> removed
based on a synchronous methods.

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

added block requests under peer moved the request trigger in the reactor poolRoutine, triggered now by a ticker in general moved everything required for making block requests smarter in the poolRoutine added a simple map of heights to keep track of what will need to be requested next added a few more tests

send errors (RemovePeer) from switch on a different channel than the one receiving blocks renamed channels added more pool tests

If we tried to send a request to a peer not present in the switch, a missing continue statement caused the request to be blackholed in a peer that was removed and never retried. While this bug was manifesting, the reactor kept asking for other blocks that would be stored and never consumed. Added the number of unconsumed blocks in the math for requesting blocks ahead of current processing height so eventually there will be no more blocks requested until the already received ones are consumed.

blockchain/v2/scheduler.go

golangcibot · 2019-07-19T15:21:02Z

blockchain/v2/scheduler.go

+	return Skip{}
+}
+
+type scPrunePeerEv struct {


U1000: type scPrunePeerEv is unused (from unused)

golangcibot · 2019-07-19T15:21:02Z

blockchain/v2/scheduler.go

+	reason error
+}
+
+func (sdr *Scheduler) handleBlockProcessError(peerID p2p.ID, height int64) Event {


U1000: func (*Scheduler).handleBlockProcessError is unused (from unused)

golangcibot · 2019-07-19T15:21:02Z

blockchain/v2/scheduler.go

+	return Skip{}
+}
+
+type scSchedulerFailure struct {


U1000: type scSchedulerFailure is unused (from unused)

golangcibot · 2019-07-19T15:21:02Z

blockchain/v2/scheduler.go

+	reason error
+}
+
+func (sdr *Scheduler) handleTimeCheck(now time.Time) Events {


U1000: func (*Scheduler).handleTimeCheck is unused (from unused)

ancazamfir

Some comments on scheduler.go. Will do the tests later.

blockchain/v2/scheduler.go

ancazamfir · 2019-07-23T12:26:19Z

blockchain/v2/scheduler.go

+	return &sc
+}
+
+func (sc *schedule) addPeer(peerID p2p.ID) error {


Should addPeer have a height parameter? What triggers a peer addition the way it is right now?
In the older version it is the first StatusResponseMessage that creates a new peer with a height.

This would most likely be pass passed an event form reactor.AddPeer

ancazamfir · 2019-07-23T12:28:51Z

blockchain/v2/scheduler.go

+
+func (sc *schedule) addPeer(peerID p2p.ID) error {
+	if _, ok := sc.peers[peerID]; ok {
+		return fmt.Errorf("Cannot add duplicate peer %s", peerID)


Should a check for removed state be added here? What happens if a peer was removed and later re-added?

That's a good question! what does @milosevic think?

ancazamfir · 2019-07-23T12:48:30Z

blockchain/v2/scheduler.go

+
+	peer.height = height
+	peer.state = peerStateReady
+	for i := sc.minHeight(); i <= height; i++ {


Asssume we start at height 1 and we have a peer that declares a very high height. This loop may take a long time and also add a lot of entries to blockStates.

Yes this on the surface seems pretty inefficient. The goal is to ensure correctness first and then look at performance it once the spec is somehow well defined.

It's not only a performance factor but also a safety/ security concern. If a "bad" node sends a huge height, setPeerHeight() will hog the CPU potentially forever. Try for example TestPeerHeight() with a height of 2^40 and observe blockchain.test process CPU and memory usage.

ancazamfir · 2019-07-23T13:00:34Z

blockchain/v2/scheduler.go

+}
+
+func (sc *schedule) allBlocksProcessed() bool {
+	for _, state := range sc.blockStates {


I assume this is called every time a block is processed. The range loop is expensive given the amount of entries in blockStates map.

This would be called at some interval. The same comment as above, let's make sure it's correct and then profile to figure out where best to optimize .

ancazamfir · 2019-07-23T13:11:43Z

blockchain/v2/scheduler.go

+		return schedulerErrorEv{peerID, err}
+	}
+
+	return scBlockReceivedEv{peerID}


Who receives this event and wouldn't the height be needed?

Good catch! Yes this will be sent to the processor and require the height, blockData, etc.

blockchain/v2/scheduler.go

brapse · 2019-07-30T16:56:08Z

This PR was replaced by #3848

fork contains updated dependecies  --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments

Anca Zamfir added 30 commits March 7, 2019 18:39

go routines in blockchain reactor

a3ba548

Added reference to the go routine diagram

eb1573c

Initial commit

6428cba

cleanup

fd9b10f

Merge branch 'develop' into ancaz/blockchain_reactor_reorg

1278d06

Undo testing_logger change, committed by mistake

82903a9

Fix the test loggers

9f25600

pulled some fsm code into pool.go

6b10988

added pool tests

70c7038

send errors to FSM in a different channel than blocks

8e4b250

send errors (RemovePeer) from switch on a different channel than the one receiving blocks renamed channels added more pool tests

more pool tests

07e951a

lint errors

fa9137a

more tests

5787f78

more tests

4d54cce

switch fast sync to new implementation

0c84d78

sync with develop

4f70567

fixed data race in tests

0541ecb

cleanup

bb36184

finished fsm tests

f9f4819

address golangci comments :)

a446473

address golangci comments :)

6cf8035

Added timeout on next block needed to advance

8ef2f6b

updating docs and cleanup

02c3499

fix issue in test from previous cleanup

61a722a

cleanup

af90cb4

Added termination scenarios, tests and more cleanup

299a7f3

small fixes to adr, comments and cleanup

714ac4f

remove bpPeer's didTimeout field

78f4f42

brapse added 15 commits July 12, 2019 14:13

flesh out scheduler

122ff60

add some standin error handling

6bce04e

WIP scheduler test

1b95954

WIP: progress on converting pseudocode

c91ab0c

at least it compiles

d69e280

at least it runs

88f142f

all tests pass

e6c5d2c

Add slow peer testing

18fcda8

Simplify error handling

5f84e3e

Peer pruning

10ff7bd

add missing events

38eb781

Cleanup peer removal and block resetting

1828f86

Test cleanup

a790f14

Better testing for markPending

8e940ad

Split up large FSM test into per-transition tests

ac151b6

brapse requested a review from ancazamfir July 19, 2019 15:18

golangcibot reviewed Jul 19, 2019

View reviewed changes

ancazamfir reviewed Jul 23, 2019

View reviewed changes

brapse added 2 commits July 25, 2019 16:01

fixes based on comments

1103036

fix peer ID

ffcf430

golangcibot reviewed Jul 25, 2019

View reviewed changes

blockchain/v2/scheduler.go Outdated Show resolved Hide resolved

brapse marked this pull request as ready for review July 30, 2019 09:55

brapse requested review from ebuchman, melekes and xla as code owners July 30, 2019 09:55

brapse changed the base branch from ancaz/blockchain_reactor_reorg to master July 30, 2019 15:30

brapse mentioned this pull request Jul 30, 2019

[blockchain] v2 riri Schedule composit data structure #3848

Merged

5 tasks

brapse closed this Jul 30, 2019

Conversation

brapse commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ancazamfir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brapse commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brapse commented Jul 19, 2019 •

edited

Loading

brapse commented Jul 30, 2019 •

edited

Loading