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

Specify source type for stored UNLs #3984

Closed
wants to merge 1 commit into from
Closed

Specify source type for stored UNLs #3984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions src/ripple/app/misc/ValidatorList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <ripple/json/json_value.h>
#include <ripple/overlay/Message.h>
#include <ripple/protocol/PublicKey.h>
#include <ripple/rpc/Context.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This violates levelization. I know RPC is included all over the place in the app, but RPC should be at a higher level, so let's not make it worse. Instead, rewrite getJson to take a std::optional<unsigned int> apiVersion parameter.

#include <boost/iterator/counting_iterator.hpp>
#include <boost/range/adaptors.hpp>
#include <boost/thread/shared_mutex.hpp>
Expand Down Expand Up @@ -180,7 +181,10 @@ class ValidatorList
std::size_t sequence;
TimeKeeper::time_point validFrom;
TimeKeeper::time_point validUntil;
std::string siteUri;
// the peer that provided the list, if applicable
std::string sourcePeer;
// the website or filesystem uri that provided the list, if applicable
std::string sourceUri;
// base-64 encoded JSON containing the validator list.
std::string rawBlob;
// hex-encoded signature of the blob using the publisher's signing key
Expand Down Expand Up @@ -382,7 +386,9 @@ class ValidatorList
@param blobs Vector of BlobInfos representing one or more encoded
validator lists and signatures (and optional manifests)

@param siteUri Uri of the site from which the list was validated
@param source The peer address, or site/file uri that provided the
lists. The formatting of this parameter is important as it is
used to determine the source type

@param hash Hash of the data parameters

Expand All @@ -406,7 +412,7 @@ class ValidatorList
std::string const& manifest,
std::uint32_t version,
std::vector<ValidatorBlobInfo> const& blobs,
std::string siteUri,
std::string source,
Comment on lines -409 to +415
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are only two non-test callers to applyListsAndBroadcast, in ValidatorSite.cpp (line 405), and PeerImp.cpp (line 2298). Instead of the overhead of parsing source to figure out what type it is, why not add another parameter to this function, and applyLists and applyList? To keep it super explicit, you could make it an enum:

enum class SourceType
{
  Site,
  Peer
};

uint256 const& hash,
Overlay& overlay,
HashRouter& hashRouter,
Expand All @@ -419,14 +425,16 @@ class ValidatorList
@param version Version of published list format

@param blobs Vector of BlobInfos representing one or more encoded
validator lists and signatures (and optional manifests)
validator lists and signatures (and optional manifests)

@param siteUri Uri of the site from which the list was validated
@param source The peer address, or site/file uri that provided the
lists. The formatting of this parameter is important as it is
used to determine the source type

@param hash Optional hash of the data parameters

@return `ListDisposition::accepted`, plus some of the publisher
information, if list was successfully applied
information, if list was successfully applied

@par Thread Safety

Expand All @@ -437,7 +445,7 @@ class ValidatorList
std::string const& manifest,
std::uint32_t version,
std::vector<ValidatorBlobInfo> const& blobs,
std::string siteUri,
std::string source,
std::optional<uint256> const& hash = {});

/* Attempt to read previously stored list files. Expected to only be
Expand Down Expand Up @@ -647,7 +655,7 @@ class ValidatorList
May be called concurrently
*/
Json::Value
getJson() const;
getJson(std::optional<RPC::JsonContext> context = std::nullopt) const;

using QuorumKeys = std::pair<std::size_t const, hash_set<PublicKey>>;
/** Get the quorum and all of the trusted keys.
Expand Down Expand Up @@ -743,7 +751,9 @@ class ValidatorList

@param version Version of published list format

@param siteUri Uri of the site from which the list was validated
@param source The peer address, or site/file uri that provided the
lists. The formatting of this parameter is important as it is
used to determine the source type

@param hash Optional hash of the data parameters.
Defaults to uninitialized
Expand All @@ -762,7 +772,7 @@ class ValidatorList
std::string const& blob,
std::string const& signature,
std::uint32_t version,
std::string siteUri,
std::string source,
std::optional<uint256> const& hash,
lock_guard const&);

Expand Down
47 changes: 37 additions & 10 deletions src/ripple/app/misc/impl/ValidatorList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,23 @@

namespace ripple {

bool
isSite(std::string const& source)
{
parsedURL parsed;
if (!parseUrl(parsed, source))
// Assume that the source is a peer address in the
// event that parsing fails.
return false;

const std::unordered_set<std::string> schemes{"file", "http", "https"};

if (schemes.find(parsed.scheme) != schemes.end())
return true;

return false;
}

std::string
to_string(ListDisposition disposition)
{
Expand Down Expand Up @@ -870,14 +887,14 @@ ValidatorList::applyListsAndBroadcast(
std::string const& manifest,
std::uint32_t version,
std::vector<ValidatorBlobInfo> const& blobs,
std::string siteUri,
std::string source,
uint256 const& hash,
Overlay& overlay,
HashRouter& hashRouter,
NetworkOPs& networkOPs)
{
auto const result =
applyLists(manifest, version, blobs, std::move(siteUri), hash);
applyLists(manifest, version, blobs, std::move(source), hash);
auto const disposition = result.bestDisposition();

if (disposition == ListDisposition::accepted)
Expand Down Expand Up @@ -923,7 +940,7 @@ ValidatorList::applyLists(
std::string const& manifest,
std::uint32_t version,
std::vector<ValidatorBlobInfo> const& blobs,
std::string siteUri,
std::string source,
std::optional<uint256> const& hash /* = {} */)
{
if (std::count(
Expand All @@ -943,7 +960,7 @@ ValidatorList::applyLists(
blobInfo.blob,
blobInfo.signature,
version,
siteUri,
source,
hash,
lock);

Expand Down Expand Up @@ -1063,7 +1080,7 @@ ValidatorList::applyList(
std::string const& blob,
std::string const& signature,
std::uint32_t version,
std::string siteUri,
std::string source,
std::optional<uint256> const& hash,
ValidatorList::lock_guard const& lock)
{
Expand Down Expand Up @@ -1137,7 +1154,9 @@ ValidatorList::applyList(
list.isMember(jss::effective) ? list[jss::effective].asUInt() : 0}};
publisher.validUntil = TimeKeeper::time_point{
TimeKeeper::duration{list[jss::expiration].asUInt()}};
publisher.siteUri = std::move(siteUri);
auto& sourceField =
isSite(source) ? publisher.sourceUri : publisher.sourcePeer;
sourceField = std::move(source);
publisher.rawBlob = blob;
publisher.rawSignature = signature;
publisher.rawManifest = localManifest;
Expand Down Expand Up @@ -1504,7 +1523,7 @@ ValidatorList::expires() const
}

Json::Value
ValidatorList::getJson() const
ValidatorList::getJson(std::optional<RPC::JsonContext> context) const
{
Json::Value res(Json::objectValue);

Expand Down Expand Up @@ -1563,9 +1582,17 @@ ValidatorList::getJson() const
curr[jss::available] =
pubCollection.status == PublisherStatus::available;

auto appendList = [](PublisherList const& publisherList,
Json::Value& target) {
target[jss::uri] = publisherList.siteUri;
auto appendList = [&context](
PublisherList const& publisherList,
Json::Value& target) {
if ((context && context->apiVersion == 1) || !context)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be written as if (!context || context->apiVersion == 1). Or if you get rid of the context object as I suggested above, it would be if (!apiVersion || *apiVersion == 1).

target[jss::uri] = publisherList.sourceUri.empty()
? publisherList.sourcePeer
: publisherList.sourceUri;
if (!publisherList.sourceUri.empty())
target[jss::source_uri] = publisherList.sourceUri;
Comment on lines +1589 to +1593
Copy link
Collaborator

Choose a reason for hiding this comment

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

You know, @RichardAH has a good point in his comment #3984 (comment).

What do you think about completely removing the new jss::source_uri field, and just including the sourceUri member as jss::uri? You could get rid of context entirely.

if (!publisherList.sourcePeer.empty())
target[jss::source_peer] = publisherList.sourcePeer;
if (publisherList.validUntil != TimeKeeper::time_point{})
{
target[jss::seq] =
Expand Down
2 changes: 2 additions & 0 deletions src/ripple/protocol/jss.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,9 @@ JSS(snapshot); // in: Subscribe
JSS(source_account); // in: PathRequest, RipplePathFind
JSS(source_amount); // in: PathRequest, RipplePathFind
JSS(source_currencies); // in: PathRequest, RipplePathFind
JSS(source_peer); // out: ValidatorSites
JSS(source_tag); // out: AccountChannels
JSS(source_uri); // out: ValidatorSites
JSS(stand_alone); // out: NetworkOPs
JSS(start); // in: TxHistory
JSS(started);
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/rpc/handlers/Validators.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ doValidators(RPC::JsonContext& context)
if (context.app.config().reporting())
return rpcError(rpcREPORTING_UNSUPPORTED);

return context.app.validators().getJson();
return context.app.validators().getJson(context);
}

} // namespace ripple
8 changes: 4 additions & 4 deletions src/test/app/ValidatorList_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ class ValidatorList_test : public beast::unit_test::suite
testcase("Apply list");
using namespace std::chrono_literals;

std::string const siteUri = "testApplyList.test";
std::string const siteUri = "http://testApplyList.test";

auto checkAvailable =
[this](
Expand Down Expand Up @@ -926,7 +926,7 @@ class ValidatorList_test : public beast::unit_test::suite
testcase("GetAvailable");
using namespace std::chrono_literals;

std::string const siteUri = "testApplyList.test";
std::string const siteUri = "http://testApplyList.test";

ManifestCache manifests;
jtx::Env env(*this);
Expand Down Expand Up @@ -1062,7 +1062,7 @@ class ValidatorList_test : public beast::unit_test::suite
{
testcase("Update trusted");

std::string const siteUri = "testUpdateTrusted.test";
std::string const siteUri = "http://testUpdateTrusted.test";

PublicKey emptyLocalKeyOuter;
ManifestCache manifestsOuter;
Expand Down Expand Up @@ -1615,7 +1615,7 @@ class ValidatorList_test : public beast::unit_test::suite
{
testcase("Expires");

std::string const siteUri = "testExpires.test";
std::string const siteUri = "http://testExpires.test";

jtx::Env env(*this);
auto& app = env.app();
Expand Down