Conversation
|
Good start, but we need to check < 1/3 with respect to voting power, not just number of validators. Also, the change in voting power must be strictly less than 1/3, not <= 1/3 |
|
So, the algo should look more like: ? |
|
We don't even have to check the number of validators, just the voting power. Basically we should sum the voting power changes and if thats >=1/3 of the current total, then error. |
Codecov Report
@@ Coverage Diff @@
## develop #1004 +/- ##
===========================================
- Coverage 59.95% 59.65% -0.31%
===========================================
Files 109 109
Lines 10197 10225 +28
===========================================
- Hits 6114 6100 -14
- Misses 3523 3556 +33
- Partials 560 569 +9 |
29d5cf0 to
0093f98
Compare
ebuchman
left a comment
There was a problem hiding this comment.
Looks great. But should have more tests
| assert := assert.New(t) | ||
|
|
||
| // change vals at these heights | ||
| changeHeights := []int64{1, 2, 4, 5, 10, 15, 16, 17, 20} |
There was a problem hiding this comment.
because it was written with an assumption that there is only one validator. I can try and rewrite it of course
There was a problem hiding this comment.
right. I think it's ok since we're specifically testing the "save/load at height" behaviour, and having just one validator was an easy way to write the test, and we have other tests of multiple validators. So we should defntly keep it. If you have a nice way to write it with multiple validators, that'd of course be welcome, though now we have to be mindful that it can only change by < 1/3 :)
state/state_test.go
Outdated
| Height: height, | ||
| // 1 val (vp: 10) => less than 3 is ok | ||
| EndBlock: &abci.ResponseEndBlock{ValidatorUpdates: []*abci.Validator{ | ||
| {PubKey: crypto.GenPrivKeyEd25519().PubKey().Bytes(), Power: 3}, |
There was a problem hiding this comment.
we should have more test cases
6ee11f2 to
b8215d8
Compare
|
@ebuchman done. feel free to add more test cases if you want (https://github.com/tendermint/tendermint/pull/1004/files#diff-a1153b96e079da8da43231ad48de57a0R248) |
This is backport of three PRs originally merged against main: * spec/p2p: Specify the operation of a Reactor (tendermint#714) * spec/p2p: document the p2p API used by Reactors (tendermint#851) * spec/p2p: new structure for the p2p specification (tendermint#966) Co-authored-by: Josef Widder <44643235+josef-widder@users.noreply.github.com> Co-authored-by: Lasaro <lasaro@informal.systems> Co-authored-by: Sergio Mena <sergio@informal.systems>
Refs #950