Use peermanager scores for blocksync peers and don't error out on block mismatch #162
Conversation
| @@ -4,7 +4,9 @@ import ( | |||
| "context" | |||
| "errors" | |||
| "fmt" | |||
| "github.com/tendermint/tendermint/internal/p2p" | |||
internal/blocksync/pool.go
Outdated
| for _, peer := range pool.peers { | ||
| // Generate a sorted list | ||
| sortedPeers := pool.getSortedPeers(pool.peers) | ||
| fmt.Printf("PSUDEBUG - block sync with sorted peers: %v\n", sortedPeers) |
There was a problem hiding this comment.
Shall we remove this debug log?
internal/blocksync/pool.go
Outdated
| fmt.Printf("PSUDEBUG - block sync with sorted peers: %v\n", sortedPeers) | ||
| for _, nodeId := range sortedPeers { | ||
| peer := pool.peers[nodeId] | ||
| pool.peerManager.Score(peer.id) |
There was a problem hiding this comment.
why do we get the score out here without using it?
| // Generate a sorted list | ||
| sortedPeers := pool.getSortedPeers(pool.peers) | ||
| fmt.Printf("PSUDEBUG - block sync with sorted peers: %v\n", sortedPeers) | ||
| for _, nodeId := range sortedPeers { |
There was a problem hiding this comment.
A few further optimizations we can do:
- We should probably avoid using the low score peers, for example the ones with score 0
- We should do some random shuffling so that we don't always targeting the same few top peers
internal/blocksync/pool.go
Outdated
| @@ -47,7 +50,7 @@ const ( | |||
| maxDiffBetweenCurrentAndReceivedBlockHeight = 100 | |||
| ) | |||
|
|
|||
| var peerTimeout = 15 * time.Second // not const so we can override with tests | |||
| var peerTimeout = 3 * time.Second // not const so we can override with tests | |||
There was a problem hiding this comment.
waiting 15s for timeout is too long given we expect blocks much more frequently
philipsu522
changed the title
internal/blocksync/pool.go
Outdated
| @@ -315,7 +320,9 @@ func (pool *BlockPool) AddBlock(peerID types.NodeID, block *types.Block, extComm | |||
| } | |||
| } else { | |||
| err := errors.New("requester is different or block already exists") | |||
| pool.sendError(err, peerID) | |||
| // Original behavior is to error out when there is a mismatch, which shuts down the entire reactor. | |||
There was a problem hiding this comment.
Overall LGTM, one question: do we understand why the original behavior wants to error this out and shutdown the entire reactor? Is there going to be any side effect if we change this behavior?
There was a problem hiding this comment.
let's revert this now, and when the standalone rpc falls behind again (currently it is healthly, and was upgraded to the new version), we can re-apply this patch and see if it helps
Describe your changes and provide context
Import peermanager scores to blocksync reactor and use that to determine which peers to pick for blocksync. We also don't error out on a mismatch of block height from peer who sent it
Testing performed to validate your change