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

Add validator list RPC calls (RIPD-1541) #2242

Closed
wants to merge 3 commits into from

Conversation

bachase
Copy link
Collaborator

@bachase bachase commented Oct 9, 2017

In support of dynamic validator list, this changeset

  1. Adds a new validator_list_expires field to server_info that indicates when the current validator list will become stale.
  2. Adds a new admin only validator_lists RPC that returns the current list of known validators and the most recent published validator lists.
  3. Adds a new admin only validator_sites RPC that returns the list of configured validator publisher sites and when they were most recently queried.

This builds on PR #2240, which is the first two commits in this changeset.

@bachase
Copy link
Collaborator Author

bachase commented Oct 9, 2017

@HowardHinnant, I took some of your chrono changes in #2237 for use cdc7537


} // namespace test
} // namespace ripple
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

No newline at end of file

std::forward_as_tuple(local),
std::forward_as_tuple());
// Config listed keys never expire
if (it.second)
Copy link
Contributor

Choose a reason for hiding this comment

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

#UnIndent2x

expiration,
1,
keys);
env.app().validatorSites().start();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm getting the same test failures as Travis:

#32 failed: ValidatorRPC_test.cpp(269)
#33 failed: ValidatorRPC_test.cpp(273)
#34 failed: ValidatorRPC_test.cpp(275)
#35 failed: ValidatorRPC_test.cpp(277)
#37 failed: ValidatorRPC_test.cpp(290)
#38 failed: ValidatorRPC_test.cpp(291)
#39 failed: ValidatorRPC_test.cpp(301)
#41 failed: ValidatorRPC_test.cpp(304)
#45 failed: ValidatorRPC_test.cpp(314)
#46 failed: ValidatorRPC_test.cpp(317)

The site isn't being fetched here because ValidatorSite::start was already called when env was constructed.
The only workaround I've thought of is to use a new Env for this set of tests (and construct it after the site is up).

@codecov-io
Copy link

codecov-io commented Oct 10, 2017

Codecov Report

Merging #2242 into develop will increase coverage by 0.09%.
The diff coverage is 93.2%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #2242      +/-   ##
==========================================
+ Coverage    70.11%   70.2%   +0.09%     
==========================================
  Files          689     691       +2     
  Lines        50800   50890      +90     
==========================================
+ Hits         35617   35726     +109     
+ Misses       15183   15164      -19
Impacted Files Coverage Δ
src/ripple/app/misc/ValidatorSite.h 100% <ø> (ø) ⬆️
src/ripple/rpc/impl/Handler.cpp 97.61% <ø> (ø) ⬆️
src/ripple/app/misc/ValidatorList.h 98.24% <100%> (-0.04%) ⬇️
src/ripple/rpc/handlers/ValidatorListSites.cpp 100% <100%> (ø)
src/ripple/app/misc/NetworkOPs.cpp 62.56% <100%> (+0.81%) ⬆️
src/ripple/rpc/handlers/Validators.cpp 100% <100%> (ø)
src/ripple/app/misc/impl/ValidatorSite.cpp 80.24% <89.47%> (+1.92%) ⬆️
src/ripple/app/misc/impl/ValidatorList.cpp 85.09% <92.3%> (+1.76%) ⬆️
... and 6 more

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...9b6ce34. Read the comment docs.

Copy link
Contributor

@mellery451 mellery451 left a comment

Choose a reason for hiding this comment

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

I'd like to see the hardcoded test port changed ...otherwise looks good to me

#include <test/jtx.h>
#include <test/jtx/TrustedPublisherServer.h>

#include <boost/format.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any evidence that this header is used...I might be missing something


public:
void
testPriviledges()
Copy link
Contributor

Choose a reason for hiding this comment

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

to be super nit-picky, this is mispelled (no 'd' in privilege) BUT I would not change unless you are messing the the code for some other reason.

http_sync_server server1(
ep1, ioService, pubSigningKeys1, manifest1, sequence,
expiration.time_since_epoch().count(), version, list1);
TrustedPublisherServer server1(
Copy link
Contributor

Choose a reason for hiding this comment

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

ioService (line 116) is now unused, FWIW

expectedKeys.insert(toStr(key));

// Publisher site information
std::uint16_t constexpr port = 7475;
Copy link
Contributor

Choose a reason for hiding this comment

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

for tests particularly, I would opt for OS port selection - it helps avoids spurious failures related to ports in use. You can use port 0 in for the bind endpoint and then provide a way on TrustedPublisherServer to get acceptor_.local_endpoint() after listen has been called, which you then pass to clients (probably the simplest approach is just make acceptor_ public).

Copy link
Contributor

@mellery451 mellery451 left a comment

Choose a reason for hiding this comment

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

👍

}
}

// Current validator keys
Copy link
Contributor

@wilsonianb wilsonianb Oct 13, 2017

Choose a reason for hiding this comment

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

This seems redundant since it's always accompanied by the individual published lists from above.
What if we instead just list the trusted keys (from trustedKeys_)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, that makes a lot more sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Call the JSON field validator_keys or trusted_validator_keys?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like trusted_validator_keys better

@@ -231,6 +232,9 @@ ValidatorSite::onSiteFetch(
body["signature"].asString(),
body["version"].asUInt());

sites_[siteIdx].lastRefreshStatus.emplace(
Site::Status{clock_type::now(), disp});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think lastRefreshStatus should also be updated with ListDisposition::invalid in the else below in the case that we fail to parse the JSON.

JSS ( url ); // in/out: Subscribe, Unsubscribe
JSS ( url_password ); // in: Subscribe
JSS ( url_username ); // in: Subscribe
JSS ( urlgravatar ); //
JSS ( username ); // in: Subscribe
JSS ( validated ); // out: NetworkOPs, RPCHelpers, AccountTx*
// Tx
JSS ( validator_list_expires) ; // out: NetworkOps
Copy link
Contributor

Choose a reason for hiding this comment

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

also in ValidatorList

Json::Value
doValidatorLists(RPC::Context& context)
{
auto lock = make_lock(context.app.getMasterMutex());
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the lock? (Same with ValidatorSites handler)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. I was just following other RPCs, but since the getJson calls are themselves safe, this is not needed.

BEAST_EXPECT(trustedKeys->listed(localCfgListed));
}

// Manifest published keys with expirations
Copy link
Contributor

Choose a reason for hiding this comment

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

Manifest

BEAST_EXPECT(
trustedKeys->load(emptyLocalKey, emptyCfgKeys, cfgKeys));

// Initially, no manifest has been published, so never stale
Copy link
Contributor

Choose a reason for hiding this comment

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

manifest --> list

void
testExpires()
{
testcase("Stale");
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider "Expires" or "Expiration"

{
// The current HTTP/S ServerHandler returns an HTTP 403
// error code here rather than a noPermission JSON error.
// The JSONRPCClient just eats that error and returns an
Copy link
Contributor

Choose a reason for hiding this comment

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

an --> a

// Publisher list site unavailable
{
// Publisher site information
endpoint_type ep{address_type::from_string("127.0.0.1"), 1234};
Copy link
Contributor

Choose a reason for hiding this comment

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

ep is unused

BEAST_EXPECT(js[jss::refresh_interval_min].asUInt() == 5);
BEAST_EXPECT(js[jss::uri] == siteURI);
BEAST_EXPECT(js[jss::last_refresh_status] == "accepted");
// The actual time of the udpate will vary run to run, so
Copy link
Contributor

Choose a reason for hiding this comment

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

--> update

else
res[jss::validator_list_expires] = "unknown";

// Publisher lists
Copy link
Contributor

Choose a reason for hiding this comment

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

Speaking of redundant information, what do you think about including each publisher list's version, which would always be 1?

curr[jss::pubkey_publisher] = strHex(p.first);
curr[jss::seq] = static_cast<Json::UInt>(p.second.sequence);
curr[jss::available] = p.second.available;
curr[jss::expiration] = to_string(p.second.expiration);
Copy link
Contributor

Choose a reason for hiding this comment

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

For the static list in the config, the entry shows up as:

         {
            "available" : true,
            "expiration" : "2136-Feb-07 06:28:15",
            "list" : [
               "n949f75evCHwgyP4fPVgaHqNHxUVN15PsJEZ3B3HnXPcPjcZAoy7",
               "n9MD5h24qrQqiyBC8aeqqCWvpiBiYQ3jxSr91uiDvmrkyHRdYLUj",
               "n9L81uNCaPgtUJfaHh89gmdvXKAmSt5Gdsw2g1iPWaPkAHW5Nm4C",
               "n9KiYM9CgngLvtRCQHZwgC2gjpdaZcCcbt3VboxiNFcKuwFVujzS",
               "n9LdgEtkmGB9E2h3K4Vp7iGUaKuq23Zr32ehxiU8FWY7xoxbWTSA"
            ],
            "pubkey_publisher" : "",
            "seq" : 0
         }

I'm wondering if these should instead be in a third list (static_validator_keys?) outside of publisher_lists, since available, expiration, pubkey_publisher, and seq are all meaningless.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe the three lists returned could be something like

  • local_static_keys
  • published_lists/dynamic_lists
  • trusted_keys

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like that distinction, but wasn't sure if the preference is to encourage the use of published_lists over local keys. That being said, its simple to include them.

Maybe the better ordering is:

  • trusted_keys
  • local_static_keys
  • published_lists
    ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since they're keys in a JSON object, I don't think the order is preserved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤦‍♂️

@@ -479,14 +479,11 @@ ValidatorList::getJson() const
}

// Current validator keys
Json::Value& jValidatorKeys = (res[jss::validator_keys] = Json::arrayValue);
Json::Value& jValidatorKeys =
(res[jss::trusted_validator_keys] = Json::arrayValue);
for (auto const& v : keyListings_)
Copy link
Contributor

Choose a reason for hiding this comment

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

keyListings_ --> trustedKeys_

@@ -2148,6 +2148,21 @@ Json::Value NetworkOPsImp::getServerInfo (bool human, bool admin)
info[jss::validation_quorum] = static_cast<Json::UInt>(
app_.validators ().quorum ());

if (auto when = app_.validators().expires())
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this should be admin-only.
However it does seem to be in a similar category as validation_quorum, which is currently not admin-only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could be convinced otherwise, but I like matching validation_quorum, so that if you ever do get stale and validation_quorum goes to "infinity", you would know when it happened.

if (auto when = app_.validators().expires())
{
if (human)
info[jss::validator_list_expires] = to_string(*when);
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about displaying something like "never" or "no expiration" if when equals TimeKeeper::time_point::max()? Same would apply to the validator_lists response

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like "never". I'll update it.

@@ -151,6 +151,8 @@ Handler handlerArray[] {
{ "unl_list", byRef (&doUnlList), Role::ADMIN, NO_CONDITION },
{ "validation_create", byRef (&doValidationCreate), Role::ADMIN, NO_CONDITION },
{ "validation_seed", byRef (&doValidationSeed), Role::ADMIN, NO_CONDITION },
{ "validator_lists", byRef (&doValidatorLists), Role::ADMIN, NO_CONDITION },
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about just calling this validators?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makse sense

Do you think validator_sites should become validator_list_sites to match https://github.com/ripple/rippled/blob/develop/doc/validators-example.txt#L26?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's clearer

@bachase
Copy link
Collaborator Author

bachase commented Oct 19, 2017

There were some issue with my use of asio in TrustedPublisherServer, so I went with @wilsonianb's advice and went back to providing the io_service in 661d72f.

ep2, ioService, pubSigningKeys2, manifest2, sequence,
expiration.time_since_epoch().count(), version, list2);
TrustedPublisherServer server1(
ep1,
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 switch to OS port selection for these servers too?

res[jss::validation_quorum] = static_cast<Json::UInt>(quorum());

if (auto when = expires())
res[jss::validator_list_expires] = to_string(*when);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also use "never" when when is max()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Uggh, sorry I missed this. Maybe I should just make expires return a string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, that doesn't work as well with the machine readable response in server_state. I'll just handle it directly.

@@ -85,7 +85,8 @@ Json::Value doWalletPropose (RPC::Context&);
Json::Value doWalletSeed (RPC::Context&);
Json::Value doWalletUnlock (RPC::Context&);
Json::Value doWalletVerify (RPC::Context&);

Json::Value doValidatorLists (RPC::Context&);
Copy link
Contributor

Choose a reason for hiding this comment

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

--> doValidators

@@ -85,7 +85,8 @@ Json::Value doWalletPropose (RPC::Context&);
Json::Value doWalletSeed (RPC::Context&);
Json::Value doWalletUnlock (RPC::Context&);
Json::Value doWalletVerify (RPC::Context&);

Json::Value doValidatorLists (RPC::Context&);
Json::Value doValidatorSites (RPC::Context&);
Copy link
Contributor

Choose a reason for hiding this comment

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

--> doValidatorListSites

auto const jrr = env.rpc("validators")[jss::result];
BEAST_EXPECT(
jrr[jss::validator_list_expires] ==
to_string(NetClock::time_point::max()));
Copy link
Contributor

Choose a reason for hiding this comment

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

--> "never"

BEAST_EXPECT(
jp[jss::pubkey_publisher] == strHex(publisherPublic));
BEAST_EXPECT(jp[jss::expiration] ==
to_string(TimeKeeper::time_point{}));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if seq and expiration should either be "unknown" or excluded in this situation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a way to distinguish a list that was previously loaded and is now unavailable versus one that has not been successfully loaded yet?

Copy link
Contributor

Choose a reason for hiding this comment

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

The expiration would be TimeKeeper::time_point{} in the latter case. (We would never accept a list with that expiration without a time machine.)

@bachase
Copy link
Collaborator Author

bachase commented Oct 22, 2017

Rebased on 0.80.0

Copy link
Contributor

@wilsonianb wilsonianb left a comment

Choose a reason for hiding this comment

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

Speaking of new ListDispositions, this commit adds same_sequence to distinguish between a fetched list with a previous sequence (and/or expired expiration) from a fetched list with the current sequence, which are both currently treated as stale.
wilsonianb@cfb2ef6
How do you feel about including the change in this PR since it depends on your to_string for ListDisposition?

for (auto const& p : publisherLists_)
{
if (p.second.available &&
TimeKeeper::time_point{} < p.second.expiration)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this should return boost::none if a list has a TimeKeeper::time_point{} expiration (meaning it hasn't been fetched yet).
As is, we have this subset of possible validator_list_expires values:
-unfetched list: unknown
-unfetched list & one or more fetched lists: the timestamp of the soonest expiration
-unfetched list & static validator keys: never

I'm leaning toward always making validator_list_expires to be unknown if any list has yet to be successfully fetched.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even though the quorum becomes infinite if a list is unavailable, don't you still process/forward validations for whatever trusted nodes are available? If that is the case, I think providing the expiration of the trusted keys you are using is still useful.

But I don't hold that opinion particularly strongly, so if you prefer the above, I will switch.

Copy link
Contributor

Choose a reason for hiding this comment

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

We'd still have the expiration in each publisher_lists entry for the validators response to tell us when we might stop trusting and forwarding certain validators.
I've been thinking of validator_list_expires as indicating the earliest point at which you might have an incomplete trusted validator list and be unable to reach quorum, but we would need to return something like unknown whenever if we already have an incomplete list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 I'll make the change then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On a second take, I'm not sure why I was using the available field in this function. That looks to be used to indicate whether a list has been loaded and is unexpired, but if it expires, we don't want to stop reporting its expiry.

Is the better change to just

  • Return boost::none if any expiration is TimeKeeper::time_point{}
  • Otherwise, return the earliest expiration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. Changing available back to false when the list expires was just added in #2240
Your suggested better change looks correct.

Copy link
Contributor

@wilsonianb wilsonianb Oct 24, 2017

Choose a reason for hiding this comment

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

It might be worth adding a test to testExpires and/or testDynamicUNL to check the when/validator_list_expires after a list has expired.
https://github.com/ripple/rippled/blob/cafe18c59279e7de447f25a0e00d0562d6441c7a/src/test/app/ValidatorList_test.cpp#L745-L746

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 Already working on one in testExpires.

@@ -267,6 +271,9 @@ ValidatorSite::onSiteFetch(
JLOG (j_.warn()) <<
"Unable to parse JSON response from " <<
sites_[siteIdx].uri;

sites_[siteIdx].lastRefreshStatus.emplace(
Site::Status{clock_type::now(), ListDisposition::invalid});
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We currently don't record a lastRefreshStatus if there was an error fetching the list.
@mtrippled added an else for that case here (should this pr be rebased on that branch as well? 😬)
https://github.com/ripple/rippled/pull/2243/files#diff-746977a6800a792fee7608383bca05deR282
I don't know if we should use ListDisposition::invalid or a new ListDisposition value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, will add. I think invalid works for now. I don't think a new name would any useful actionable information; clients would likely need to querying the publisher site either way to figure out what is going on.

@bachase
Copy link
Collaborator Author

bachase commented Oct 23, 2017

I don't mind adding the same_sequence disposition. I'll rebase to include #2243 and we can always squash #2243, #2240 and this into one commit if this is approved in time. Or I'll continue to rebase, its not that painful.

@bachase
Copy link
Collaborator Author

bachase commented Oct 24, 2017

Rebased to include #2243. I went with invalid disposition for the case @mtrippled added in that PR.

Copy link
Contributor

@wilsonianb wilsonianb left a comment

Choose a reason for hiding this comment

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

Observation: If a configured trusted publisher key is revoked via its manifest, we simply remove the key and its list.
https://github.com/ripple/rippled/blob/cafe18c59279e7de447f25a0e00d0562d6441c7a/src/ripple/app/misc/impl/ValidatorList.cpp#L279-L283
I don't see a way for us to convey that this happened in the validators rpc response.


// Advance past the first list's expiration, but it remains the
// earliest expiration
env.timeKeeper().set(prep1.expiration + 1s);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary, but we could add a call to onConsensusStart here since that is what updates list information (such as available) when a list expires.

BEAST_EXPECT(js[jss::refresh_interval_min].asUInt() == 5);
BEAST_EXPECT(js[jss::uri] == siteURI);
BEAST_EXPECT(!js.isMember(jss::last_refresh_time));
BEAST_EXPECT(!js.isMember(jss::last_refresh_status));
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like these last two checks need to be updated

@@ -294,6 +294,9 @@ ValidatorSite::onSiteFetch(
}
else
{
sites_[siteIdx].lastRefreshStatus.emplace(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to get the sites_mutex_ lock here

@@ -294,6 +294,9 @@ ValidatorSite::onSiteFetch(
}
else
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized we probably also want to set lastRefreshStatus as invalid in the if for this else
https://github.com/ripple/rippled/blob/develop/src/ripple/app/misc/impl/ValidatorSite.cpp#L208-L214

@bachase
Copy link
Collaborator Author

bachase commented Oct 24, 2017

I suppose we convey revoking by not having it in the response when the user might expect it. I'm tempted to say that is good enough for now and we can revisit if needed.

Copy link
Contributor

@wilsonianb wilsonianb left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Thanks!

JSS ( public_key ); // out: OverlayImpl, PeerImp, WalletPropose
JSS ( public_key_hex ); // out: WalletPropose
JSS ( published_ledger ); // out: NetworkOPs
JSS ( publisher_lists); // out: ValidatorList
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ( publisher_lists) --> ( publisher_lists )
also for refresh_interval_min and validator_list_expires below

#include <BeastConfig.h>
#include <ripple/app/main/Application.h>
#include <ripple/app/misc/ValidatorList.h>
#include <ripple/basics/make_lock.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the make_lock.h include isn't necessary. Ditto for ValidatorListSites.cpp

@bachase
Copy link
Collaborator Author

bachase commented Oct 24, 2017

Thanks for the thorough review and suggestions @wilsonianb! I'll fix the remaining nits and squash it down.

@wilsonianb
Copy link
Contributor

It looks like this branch has a distinct 0.80.0 version commit, probably due to rebasing on those other branches.

mtrippled and others added 3 commits October 24, 2017 16:55
In support of dynamic validator list, this changeset:

1. Adds a new `validator_list_expires` field to `server_info` that
indicates when the current validator list will become stale.
2. Adds a new admin only `validator_lists` RPC that returns the
current list of known validators and the most recent published validator
lists.
3. Adds a new admin only `validator_sites` RPC that returns the list of
configured validator publisher sites and when they were most recently
queried.
@bachase bachase added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 28, 2017
@bachase
Copy link
Collaborator Author

bachase commented Nov 30, 2017

Merged as 044dd53

@bachase bachase closed this Nov 30, 2017
@mDuo13
Copy link
Collaborator

mDuo13 commented Dec 15, 2017

For posterity, the two new methods are documented here:
https://ripple.com/build/rippled-apis/#validators
https://ripple.com/build/rippled-apis/#validator-list-sites

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.

None yet

6 participants