Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RPC validators calls IncrementAccum if necessary #2808

Merged
merged 1 commit into from Nov 21, 2018
Merged

RPC validators calls IncrementAccum if necessary #2808

merged 1 commit into from Nov 21, 2018

Conversation

jaekwon
Copy link
Contributor

@jaekwon jaekwon commented Nov 11, 2018 •
edited

This isn't a big deal, but good to fix nevertheless.

  • [n/a] Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

@jaekwon jaekwon changed the title validators incr accum RPC validators calls IncrementAccum if necessary Nov 11, 2018
@melekes
Copy link
Contributor

melekes commented Nov 12, 2018

This needs to be rebased against develop.

@melekes melekes force-pushed the jae/rpc_validators_incr_accum branch from 5eda184 to ef0b0f5 Compare November 12, 2018 09:07
@melekes
Copy link
Contributor

melekes commented Nov 12, 2018

Rebased.

melekes
melekes approved these changes Nov 12, 2018
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍓 🍌 🥛

state/store.go
@@ -191,12 +193,14 @@ func LoadValidators(db dbm.DB, height int64) (*types.ValidatorSet, error) {
),
)
}
valInfo2.ValidatorSet.IncrementAccum(int(height - valInfo.LastHeightChanged)) // mutate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we write a test for this?

@codecov-io
Copy link

codecov-io commented Nov 12, 2018 •
edited

Codecov Report

❗ No coverage uploaded for pull request base (develop@2d525bf). Click here to learn what that means.
The diff coverage is 75%.

@@            Coverage Diff             @@
##             develop    #2808   +/-   ##
==========================================
  Coverage           ?   62.12%           
==========================================
  Files              ?      212           
  Lines              ?    17297           
  Branches           ?        0           
==========================================
  Hits               ?    10745           
  Misses             ?     5644           
  Partials           ?      908
Impacted Files Coverage Δ
state/state.go 90.9% <ø> (ø)
state/store.go 69.28% <75%> (ø)

@ebuchman
Copy link
Contributor

Can we please get a quick explanation of how this was found / what it fixes and a corresponding test? Thanks!

@melekes
Copy link
Contributor

melekes commented Nov 20, 2018

I think this was discovered by one of the validators. When we were loading vals, we didn't incrementAccum, so the accum values were incorrect.

I found out that in 0.26, the accum values in validators endpoint from 26657 are not updated by each block. Therefore, the value is not useful to inform me the exact accum situation of the whole validators. Although in dump_consensus_state endpoint, it provides current accum status, the historical data is not available.

@melekes
Copy link
Contributor

melekes commented Nov 20, 2018

wrote a test (TestStoreLoadValidatorsIncrementsAccum)

before

--- FAIL: TestStoreLoadValidatorsIncrementsAccum (0.00s)
        Error Trace:    state_test.go:282
        Error:          Should not be: -10
        Test:           TestStoreLoadValidatorsIncrementsAccum
        Messages:       expected Accum value to change between heights
FAIL
exit status 1
FAIL    github.com/tendermint/tendermint/state  0.055s

after:

PASS
ok      github.com/tendermint/tendermint/state  0.050s

@melekes melekes force-pushed the jae/rpc_validators_incr_accum branch from 646d644 to 0075b8f Compare November 20, 2018 09:00
ebuchman
ebuchman reviewed Nov 20, 2018
View reviewed changes
state/store.go
@@ -191,12 +193,14 @@ func LoadValidators(db dbm.DB, height int64) (*types.ValidatorSet, error) {
),
)
}
valInfo2.ValidatorSet.IncrementAccum(int(height - valInfo.LastHeightChanged)) // mutate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if LoadValidators is called multiple times before the validator set changes? Won't we keep incorrectly changing the accum?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't save valInfo2 as far as I can see, so I think we're fine

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, the result of LoadValidators doesn't seem to be saved anywhere. Excellent! This looks fine to merge then.

@melekes melekes merged commit 42592d9 into develop Nov 21, 2018
@melekes melekes deleted the jae/rpc_validators_incr_accum branch November 21, 2018 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melekes melekes melekes approved these changes

@ebuchman ebuchman ebuchman left review comments

@xla xla Awaiting requested review from xla

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jaekwon @melekes @codecov-io @ebuchman