Rippled Server Software Upgrade Notification #3447
Conversation
There was a problem hiding this comment.
I want to start out by saying that it was really a pleasure reviewing this pull request. There were appropriate block comments describing intent. The code was well organized. Variables were well named. Nice work.
I left some comments. They are all optional, but we should talk about any of the changes you choose not to implement. My take is that they all might be useful improvements. But I was also not watching the design of this feature, so some of my comments may be off base. I'm hoping that you'll set me right when that happens. Thanks.
There was a problem hiding this comment.
👍 Looks great. I really appreciate the added unit tests.
I ran code coverage on your most recent commit. Code coverage looks pretty good. There were three lines that were identified as not exercised. If you feel like going for the extra credit you could see if you can extend the coverage to some of those unexercised lines. But the improved coverage you already got is certainly sufficient.
carlhua
added
the
Passed
|
@ximinez The commit "fix divide by zero" has the fix from Mickey and Ed. It was tested in a local machine network with 10 validators and the amendment waiting period was reduced from 2 weeks to 2 minutes. The test cases were:
All tests passed. In tests 2) and 3), the upgrade warning messages were printed. |
| reportingPercent && | ||
| calculatePercent(higherVersionCount, rippledCount) >= | ||
| cutoffPercent; | ||
| if (higherVersionCount > 0) |
There was a problem hiding this comment.
I'm going to make a post-approval suggestion that you add assert(higherVersionCount <= rippledCount) before this line, and a comment explaining that because higherVersionCount is dependent on rippledCount, we're avoiding a potential divide by 0, AND skipping the math entirely if there's no point because there are no higher versions reporting trusted validations.
There was a problem hiding this comment.
My personal inclination would be to change the if to look like this:
if (higherVersionCount > 0 && rippledCount > 0)
Then I'd add one more condition to the comment: " and (3) the calculation won't cause divide-by-zero."
There was a problem hiding this comment.
@ximinez updated. I took in Scott suggested since that seems more straightforward and easier to understand.
Goal
To urge rippled operators to upgrade their rippled server software, we display a console message when enough validators run a higher version of the rippled software.
Overview
After the hardened validation amendment is activated, the validators will include their software versions in the validation messages at every (flag - 1) ledger. After receiving these validation messages, a node can check how many of its trusted validators run a higher version of the software than itself. If the number is more than x% (e.g. 60%) of its validators, we display a console message:
Majority of your trusted validators run a higher version of rippled server software. Please upgrade your rippled server software.
Implementation Details
When to check?
The software version information is in the validation messages of every (flag - 1) ledger. We check these information when the corresponding flag ledger is fully validated. The extra one ledger time allows the node to accumulate more (and hopefully all) validation messages of the (flag - 1) ledger.
Comparing Software Versions
https://github.com/ripple/rippled/blob/f43aeda49c5362dc83c66507cae2ec71cfa7bfdf/src/ripple/protocol/BuildInfo.h#L48
As shown in the code, the software version is encoded into a 64-bit unsigned integer. There are several fields encoded, for this task, when comparing software versions, we will only compare the major version, minor version, and patch version.
Print to console
We use
std::cerrto display the console message. Alternatively, we could use a warning log message. As long as the log is not configured assilent, the warning message will be printed tostd::cerras well.Test
We started 10 validators, 2 of them run 1.6.0, and the rest run 1.7.0 (faked). We shortened the amendment waiting period. At about ledger 768 (the 3rd flag ledger), we saw the warning message printed by the validators running 1.6.0.