blocksync: respect the maxDiffBetweenCurrentAndReceivedBlockHeight limit
#2465
Comments
…imit Before this change, the number of requesters was unlimited! Now it's constant = 100. Closes #2465
|
Based on my analysis, we respect the limit of max N of requesters (1 requester per block; 600 blocks downloaded in parallel). Lowering the max N of blocks to 100 seems prudent (especially for big blocks). For smaller blocks, it would be great to benchmark this somehow (ie. given the network setup of N (max in + out) peers, how many blocks we should request to saturate the network). |
|
I think its ok to go to higher block bounds FWIW. Osmosis right now is getting to 3.6 BPS, and CPU side seems like in our next state break we should be at 5BPS if theres no p2p issues. (And AFAIU Osmosis has one of the most expensive state machines in cosmos rn) 100 blocks fetched in advance is just 20 seconds of syncing then! Would rather have a greater buffer if possible |
fixed my any issue
…cks to {peersCount * 20} (#2467)
where 20 is the new max requests per peer.
Previously, the number of requesters (and consequently the number of
requested blocks) was 600 and did not depend on the number of peers.
There's no point in creating more than that since N of requests per peer
is capped and we know the number of peers.
Closes #2465
Fixes #1283
[api-breaking]
---
#### PR checklist
- [x] Tests written/updated
- [x] 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
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
…cks to {peersCount * 20} (#2467)
where 20 is the new max requests per peer.
Previously, the number of requesters (and consequently the number of
requested blocks) was 600 and did not depend on the number of peers.
There's no point in creating more than that since N of requests per peer
is capped and we know the number of peers.
Closes #2465
Fixes #1283
[api-breaking]
---
#### PR checklist
- [x] Tests written/updated
- [x] 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
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
(cherry picked from commit d58acd8)
# Conflicts:
# internal/blocksync/reactor.go
Report
Osmosis team: "My understanding of this value is that this implies we should only be fetching blocks 100 blocks in advance from where we currently are. I added prints and saw that even when this value was set as low as 10, we were still fetching blocks 2000+ blocks out while being starved of blocks 2 away from where we currently were
And I might be wrong about this second part, but looking at where this value is used https://github.com/osmosis-labs/cometbft/blob/490dd752eb8a1a934542f70fdc8960594e06973b/blocksync/pool.go#L264-L268, it seems like because we are fetching blocks very far out, we just error and disregard them, so its completely wasted cycles AFAICT."
Preliminary discussion
@jmalicevic: "it seems that maxDiffBetweenCurrentAndReceivedBlockHeight is only checked when we receive blocks, it should probably also be checked when we request blocks. Otherwise it is not the peers fault we get blocks too far ahead. Perhaps avoid creating requesters too far ahead in makeRequestersRoutine() by adapting when then pool.makeNextRequester is called could help."
@cason: "Yes, but only for blocks for which we don't have a tracked request.
It is still to be understood if we don't have a tracked request because it is too "old" (i.e., we already committed this block) or too "new" (i.e., too far in the future). I still can't see how the latter could occur, as: (i) a correct peer only sends us a block upon receiving our request; (ii) we track all requests we send."
@jmalicevic: "So effectively this variable is never checked unless it is an error (we don’t have a requester for it ) or we removed it at some point. But we are never enforcing not requesting blocks too far ahead with this variable it seems"
The text was updated successfully, but these errors were encountered: