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.

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

Make quorum unreachable if validator list expires (RIPD-1539) #2240

Closed
wants to merge 1 commit into from

Conversation

wilsonianb
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Oct 4, 2017

Codecov Report

Merging #2240 into develop will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2240      +/-   ##
===========================================
- Coverage    70.11%   70.09%   -0.02%     
===========================================
  Files          689      689              
  Lines        50800    50802       +2     
===========================================
- Hits         35617    35611       -6     
- Misses       15183    15191       +8
Impacted Files Coverage Δ
src/ripple/app/misc/impl/ValidatorList.cpp 83.5% <100%> (+0.16%) ⬆️
src/ripple/app/misc/ValidatorList.h 98.27% <100%> (ø) ⬆️
src/ripple/basics/DecayingSample.h 77.77% <0%> (-8.34%) ⬇️
src/ripple/app/misc/SHAMapStoreImp.cpp 78.92% <0%> (-1.03%) ⬇️
src/ripple/app/main/Application.cpp 60.42% <0%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cafe18c...b7aa316. Read the comment docs.

Copy link
Collaborator

@bachase bachase left a comment

Choose a reason for hiding this comment

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

👍

@@ -377,7 +377,8 @@ ValidatorList::onConsensusStart (
list.second.expiration <=
timeKeeper_.now().time_since_epoch().count())
removePublisherList (list.first);
else if (! list.second.available)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are all published lists required to have an expiration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -395,6 +395,9 @@ ValidatorList::removePublisherList (PublicKey const& publisherKey)
--iVal->second;
}

iList->second.list.clear();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Without this call to clear, would that mean if a published list with keys K1,K2 expired, then was republished with key K1, the keyListsings_ count for K2 would have been decremented twice? If so, is it worth adding a unit test to cover that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good observation. This added test fails without the clear
3bb20f9

@bachase
Copy link
Collaborator

bachase commented Oct 4, 2017

Great test, still 👍 on the PR.

Copy link
Collaborator

@JoelKatz JoelKatz left a comment

Choose a reason for hiding this comment

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

:neckbeard:👍

@wilsonianb wilsonianb added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Oct 9, 2017
@wilsonianb
Copy link
Contributor Author

Squashed and rebased on 0.80.0

@bachase
Copy link
Collaborator

bachase commented Nov 30, 2017

Merged as 02059a2

@bachase bachase closed this Nov 30, 2017
@wilsonianb wilsonianb deleted the expire-val-list branch May 15, 2018 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants