rpc: Fix response time grow over time #3438

unclezoro · 2019-03-17T13:25:46Z

/validators rpc response time grow with time.

Steps To Reproduce:

start a new node from hight 1
2.time curl 127.0.0.1:26657/validators, it takes 0.005s
let the chain run for serveral days, and change no validators.in my scenario the height is : 1584840
time curl 127.0.0.1:26857/validators, it takes 1.25s

The api consumes cpu resouces, in the long run, a small scale of DDOS can make the node
a zombie process.

Acturally the logic to do IncrementProposerPriority by height - valInfo.LastHeightChanged times will not replay the exactly ProposerPriority once there are two or more round.

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

melekes · 2019-03-18T06:29:26Z

state/store.go

 					valInfo.LastHeightChanged,
 					height,
 				),
 			)
 		}
-		valInfo2.ValidatorSet.IncrementProposerPriority(int(height - valInfo.LastHeightChanged)) // mutate
 		valInfo = valInfo2


A much simpler solution here might be saving/memoizing valInfo2 saveValidatorsInfo(db, height, valInfo2) instead of the whole shebang with the blockstore. Thoughts?

A much simpler solution here might be saving/memoizing valInfo2 saveValidatorsInfo(db, height, valInfo2) instead of the whole shebang with the blockstore. Thoughts?

Ofcourse, if we can save all validatorInfo. The previous implement only save whole validatorInfo of latest height.

A much simpler solution here might be saving/memoizing valInfo2 saveValidatorsInfo(db, height, valInfo2) instead of the whole shebang with the blockstore. Thoughts?

so should I modify code to saveValidatorsInfo(db, height, valInfo2), since it store redundant information ? @melekes updated. you may have a review.

codecov-io · 2019-03-18T11:21:01Z

Codecov Report

Merging #3438 into develop will increase coverage by 0.01%.
The diff coverage is 57.14%.

@@             Coverage Diff             @@
##           develop    #3438      +/-   ##
===========================================
+ Coverage     64.2%   64.21%   +0.01%     
===========================================
  Files          213      213              
  Lines        17362    17325      -37     
===========================================
- Hits         11147    11126      -21     
+ Misses        5290     5278      -12     
+ Partials       925      921       -4

Impacted Files	Coverage Δ
state/store.go	`68.53% <57.14%> (-0.76%)`	⬇️
libs/db/remotedb/remotedb.go	`35.89% <0%> (-4.94%)`	⬇️
libs/events/events.go	`93.2% <0%> (-4.86%)`	⬇️
blockchain/pool.go	`80.26% <0%> (-1.98%)`	⬇️
blockchain/reactor.go	`71.49% <0%> (-0.97%)`	⬇️
rpc/client/httpclient.go	`65.62% <0%> (-0.9%)`	⬇️
consensus/reactor.go	`71.54% <0%> (-0.12%)`	⬇️
libs/db/remotedb/grpcdb/client.go	`0% <0%> (ø)`	⬆️
p2p/pex/addrbook.go	`67.89% <0%> (+0.49%)`	⬆️
p2p/pex/pex_reactor.go	`79.87% <0%> (+0.62%)`	⬆️
... and 2 more

melekes · 2019-03-18T11:45:14Z

state/store.go

-			)
-		}
-		valInfo2.ValidatorSet.IncrementProposerPriority(int(height - valInfo.LastHeightChanged)) // mutate
-		valInfo = valInfo2


I thought we'd save valInfo2 here, not save for every height! I.e. we only call save once somebody tries to access it.

I thought we'd save valInfo2 here, not save for every height! I.e. we only call save once somebody tries to access it.

But the replay result is not correct when there are two or more rounds at one height.

How so? If we're using int(height - valInfo.LastHeightChanged) difference

But the replay result is not correct when there are two or more rounds at one height.

Is that because we call IncrementProposerPriority per round for the same height (several times) then? Can you give an example (ideally a test) which clearly shows what you mean by "not correct" here @guagualvcha? That would be awesome.

Note that the per-height proposer selection is independent of the per-round proposer selection. So the proposer at height=1 round=1 is the same as the proposer for height=2 round=0. Ie. see #3044

ebuchman · 2019-03-20T00:18:01Z

Maybe we need a second implementation of the IncrementProposerPriority for when we know the validator set hasn't changed that can be computed much more quickly for this use case. cc @ancazamfir

unclezoro · 2019-03-20T16:02:39Z

Maybe we need a second implementation of the IncrementProposerPriority for when we know the validator set hasn't changed that can be computed much more quickly for this use case. cc @ancazamfir

Great, that would be the final solution.

ancazamfir · 2019-03-26T19:46:42Z

h1 - the height of previous validator set change
h - the height of the status query
h2 - the current height

if h == h2 we could get the set from the state; else we have to do IncrementProposerPriority(h-h1) and I can't think of a much better way to compute it faster and still be accurate wrt proposer priorities.

Maybe something like this:

call:
IncrementProposerPriority(int(height - valInfo.LastHeightChanged)%valInfo.TotalVotingPower())
memoize the valInfo every time we call RescalePriorities(). This happens even without validator set changes although rarely.

May still be costly for high total voting power values. Alternatively we could save the valInfo every N blocks and then call Increment with max N times.

I thought we'd save valInfo2 here, not save for every height! I.e. we only call save once somebody tries to access it.

@melekes - could you please clarify? There may be a lot of accesses for different heights, isn't memory usage a concern?

unclezoro · 2019-04-01T08:56:13Z

h1 - the height of previous validator set change
h - the height of the status query
h2 - the current height

if h == h2 we could get the set from the state; else we have to do IncrementProposerPriority(h-h1) and I can't think of a much better way to compute it faster and still be accurate wrt proposer priorities.

Maybe something like this:

call:
IncrementProposerPriority(int(height - valInfo.LastHeightChanged)%valInfo.TotalVotingPower())

memoize the valInfo every time we call RescalePriorities(). This happens even without validator set changes although rarely.

May still be costly for high total voting power values. Alternatively we could save the valInfo every N blocks and then call Increment with max N times.

I thought we'd save valInfo2 here, not save for every height! I.e. we only call save once somebody tries to access it.

@melekes - could you please clarify? There may be a lot of accesses for different heights, isn't memory usage a concern?

@melekes @ancazamfir updated as ancazamfir said. Please have a review.

melekes · 2019-04-09T10:08:59Z

could you please clarify? There may be a lot of accesses for different heights, isn't memory usage a concern?

yes, but not big though comparing to DoS attack vector.

melekes · 2019-04-09T10:11:58Z

@guagualvcha please refrain from doing a rebase & force-push next time. When/if you do git rebase and force-push, 1) we can't see commit history 2) we can't review only new changes and forced to review everything every time. Thank you.

ancazamfir · 2019-04-09T12:12:58Z

@guagualvcha Have you tested this? What is the response time now? What about with 1<<14 or 15?

melekes · 2019-04-09T14:21:45Z

Closing in favor of #3537

melekes · 2019-04-09T13:34:44Z

state/store.go

@@ -9,6 +9,10 @@ import (
 	"github.com/tendermint/tendermint/types"
 )

+const (
+	ValidatorSetStoreInterval = 1 << 10


I don't think we need to expose it

let's rename to valSetStoreInterval and add a comment persist validator set every valSetStoreInterval blocks to avoid LoadValidators taking too much time. Refs https://github.com/tendermint/tendermint/pull/3438

melekes · 2019-04-09T13:35:55Z

state/store.go

 	if valInfo.ValidatorSet == nil {
-		valInfo2 := loadValidatorsInfo(db, valInfo.LastHeightChanged)
+		queryHeight := valInfo.LastHeightChanged


let's create a var called lastStoreHeight := height - height%valSetStoreInterval

unclezoro requested review from ebuchman, melekes and zramsay as code owners March 17, 2019 13:25

unclezoro changed the base branch from master to develop March 17, 2019 13:26

melekes reviewed Mar 18, 2019

View reviewed changes

unclezoro mentioned this pull request Mar 20, 2019

[R4R]validator rpc api consume mutch time bnb-chain/bnc-tendermint#61

Merged

4 tasks

xla changed the title ~~fix rpc response time grow with time.~~ rpc: Fix response time grow over time Mar 20, 2019

unclezoro mentioned this pull request Mar 22, 2019

Rollback data by cmd #3332

Closed

rpc: store validator info periodly

2d78f2a

unclezoro force-pushed the validator-rpc branch from 631d202 to 2d78f2a Compare April 1, 2019 08:54

unclezoro requested a review from xla as a code owner April 1, 2019 08:54

melekes mentioned this pull request Apr 9, 2019

rpc: Fix response time grow over time #3537

Merged

melekes closed this Apr 9, 2019

melekes reviewed Apr 9, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: Fix response time grow over time #3438

rpc: Fix response time grow over time #3438

unclezoro commented Mar 17, 2019

melekes Mar 18, 2019

unclezoro Mar 18, 2019 •

edited

unclezoro Mar 18, 2019 •

edited

codecov-io commented Mar 18, 2019 •

edited

melekes Mar 18, 2019

unclezoro Mar 18, 2019 •

edited

melekes Mar 19, 2019

liamsi Mar 19, 2019 •

edited

ebuchman Mar 20, 2019

ebuchman commented Mar 20, 2019

unclezoro commented Mar 20, 2019

ancazamfir commented Mar 26, 2019

unclezoro commented Apr 1, 2019

melekes commented Apr 9, 2019

melekes commented Apr 9, 2019

ancazamfir commented Apr 9, 2019

melekes commented Apr 9, 2019

melekes Apr 9, 2019

melekes Apr 9, 2019

melekes Apr 9, 2019

rpc: Fix response time grow over time #3438

rpc: Fix response time grow over time #3438

Conversation

unclezoro commented Mar 17, 2019

melekes Mar 18, 2019

Choose a reason for hiding this comment

unclezoro Mar 18, 2019 • edited

Choose a reason for hiding this comment

unclezoro Mar 18, 2019 • edited

Choose a reason for hiding this comment

codecov-io commented Mar 18, 2019 • edited

Codecov Report

melekes Mar 18, 2019

Choose a reason for hiding this comment

unclezoro Mar 18, 2019 • edited

Choose a reason for hiding this comment

melekes Mar 19, 2019

Choose a reason for hiding this comment

liamsi Mar 19, 2019 • edited

Choose a reason for hiding this comment

ebuchman Mar 20, 2019

Choose a reason for hiding this comment

ebuchman commented Mar 20, 2019

unclezoro commented Mar 20, 2019

ancazamfir commented Mar 26, 2019

unclezoro commented Apr 1, 2019

melekes commented Apr 9, 2019

melekes commented Apr 9, 2019

ancazamfir commented Apr 9, 2019

melekes commented Apr 9, 2019

melekes Apr 9, 2019

Choose a reason for hiding this comment

melekes Apr 9, 2019

Choose a reason for hiding this comment

melekes Apr 9, 2019

Choose a reason for hiding this comment

unclezoro Mar 18, 2019 •

edited

unclezoro Mar 18, 2019 •

edited

codecov-io commented Mar 18, 2019 •

edited

unclezoro Mar 18, 2019 •

edited

liamsi Mar 19, 2019 •

edited