Specify source type for stored UNLs #3984

undertome · 2021-11-17T14:56:58Z

High Level Overview of Change

This pull request resolves #3949 by specifying the source of stored validator lists as displayed in the response to the rpc command validators.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

Before / After

This change introduces two field names to replace an existing name. Consequently, this change may compel updates to applications that expect the old name. Specifically uri will become either source_uri or source_peer depending on the source of the validator list.

Test Plan

Unit tests and integration tests were conducted to confirm expected behavior.

undertome · 2021-11-17T14:59:22Z

@mDuo13 Doc changes

ximinez · 2021-11-30T19:56:25Z

src/ripple/app/misc/impl/ValidatorList.cpp

-            target[jss::uri] = publisherList.siteUri;
+            if (!publisherList.sourceUri.empty())
+                target[jss::source_uri] = publisherList.sourceUri;
+            if (!publisherList.sourcePeer.empty())
+                target[jss::source_peer] = publisherList.sourcePeer;


Something I hadn't considered until I saw this block, but this is an API breaking change, and probably needs to be versioned. I haven't actually messed with the versioning code a lot myself, but I know RPC::JsonContext has an apiVersion member that you can pass to this function, or post-process in the caller.

I think what would make the most sense is to include the uri if and only if apiVersion == 1. You could define it as uri = sourceUri.empty() ? sourcePeer : sourceUri. (It's not spec'd as optional, so let it be set to an empty string if both values are empty.)

I think that regardless of version, it would be safe to add both source_uri and source_peer in the result.

I had same concern about the API breaking change. Here is an example we can probably follow, #3770. Note how the config is enabled in unit tests.
(@undertome I forgot it when we chatted.)

What about when this method gets called via OverlayImpl::processCrawl -> OverlayImpl::getUnlInfo. In the former function the version for the response message (response<json_body>) is being set to "2". In this case should I use 2 as the API version?

Let's say the existing code returns API version 1 response and the new code returns API version 2 response. I think we need to keep both code paths, so that the correct response can be returned.

If a RPC request has "api_version": 2, we should return API version 2 response.
If a RPC request has "api_version": 1 or if this field is missing, API version 1 response should be returned. OverlayImpl::processCrawl should also return API version 1 response.

I think #3770 is a good and simple example. Please take a look.

That doesn't really answer my question. I'm asking whether the version in the response message sent from the OverlayImpl should be treated the same as the RPC api_version number. As for as I can tell #3770 doesn't address this specific question. BTW, in OverlayImpl::processCrawl the version is being hard coded to 2 regardless of any version info in the request. Is that intended? At @ximinez maybe you can shed some light?

"I'm asking whether the version in the response message sent from the OverlayImpl should be treated the same as the RPC api_version number." I think not. Let's see what @ximinez thinks.

The UNL version (returned from OverlayImpl) is entirely separate from the API version.

@ximinez For lists sent via the OverlaImpl, is it ok to just always send the new fields instead of uri?

I didn't even realize that crawl had versioning until now, and the rules don't seem to be documented other than https://xrpl.org/peer-crawler.html#response-format noting the version changed in 1.2.1. Maybe somebody else can weigh in, but my thinking is that if you don't change the version number, just add the new fields without removing any.

pwang200

We probably can follow this example to handle the API breaking change. Looks good otherwise.

intelliot · 2022-06-01T03:54:35Z

@alloynetworks @WietseWind could you have a look too?

RichardAH · 2022-06-02T07:57:41Z

Why does it need to be a breaking change? Retain the old behaviour by continuing to emit the uri key as before and then add the new fields in addition? No json parser will break for the presence of unused new fields.

ximinez · 2022-06-03T22:49:47Z

@intelliot I don't think anything has changed since I last looked at this, so why did you request a re-review?

intelliot · 2022-06-05T01:45:03Z

It looked like the branch was force-pushed after your request for changes

ximinez · 2022-06-06T22:38:38Z

src/ripple/app/misc/ValidatorList.h

-        std::string siteUri,
+        std::string source,


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 };

ximinez · 2022-06-06T22:45:12Z

src/ripple/app/misc/ValidatorList.h

@@ -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>


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.

ximinez · 2022-06-06T22:46:38Z

src/ripple/app/misc/impl/ValidatorList.cpp

+        auto appendList = [&context](
+                              PublisherList const& publisherList,
+                              Json::Value& target) {
+            if ((context && context->apiVersion == 1) || !context)


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).

ximinez · 2022-06-06T22:54:05Z

src/ripple/app/misc/impl/ValidatorList.cpp

+                target[jss::uri] = publisherList.sourceUri.empty()
+                    ? publisherList.sourcePeer
+                    : publisherList.sourceUri;
+            if (!publisherList.sourceUri.empty())
+                target[jss::source_uri] = publisherList.sourceUri;


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.

intelliot · 2022-11-28T19:45:36Z

(notes: this may have been added already in a different way; @nbougalis will check on that later)

intelliot · 2023-04-12T00:18:50Z

@pwang200 @ChronusZ could one (or both) of you evaluate whether this change still makes sense to do?

intelliot · 2023-10-06T05:20:53Z

@nbougalis @pwang200 - any thoughts on this PR? Does it still make sense to merge, or should this PR be closed without merging?

pwang200 · 2023-10-16T18:33:45Z

I think Ed's comments should be addressed. So it should not be merged as is. My vote is close the PR without merging.

intelliot · 2023-10-25T17:37:01Z

Closing this PR due to inactivity. #3949 is still open and should be addressed with a new PR.

undertome added API Change Documentation README changes, code comments, etc. labels Nov 17, 2021

undertome requested review from pwang200 and ximinez November 17, 2021 14:56

ximinez requested changes Nov 30, 2021

View reviewed changes

pwang200 reviewed Dec 1, 2021

View reviewed changes

undertome force-pushed the validators-uri-field branch from 2e88564 to 99682e8 Compare January 12, 2022 18:24

Specify source type for stored UNLs

d57db1f

undertome force-pushed the validators-uri-field branch from 99682e8 to d57db1f Compare January 13, 2022 18:31

palomixtli approved these changes May 9, 2022

View reviewed changes

intelliot approved these changes Jun 1, 2022

View reviewed changes

intelliot requested a review from ximinez June 1, 2022 03:54

ximinez requested changes Jun 6, 2022

View reviewed changes

intelliot assigned nbougalis Nov 28, 2022

intelliot requested a review from ChronusZ April 12, 2023 00:17

intelliot assigned ChronusZ and pwang200 Apr 12, 2023

intelliot added the Will Need Documentation label Jun 28, 2023

intelliot added the Needs Discussion label Sep 6, 2023

intelliot unassigned ChronusZ Sep 6, 2023

intelliot removed the request for review from ChronusZ September 6, 2023 06:17

intelliot added this to the 2.0 milestone Oct 16, 2023

intelliot requested review from pwang200 and nbougalis October 16, 2023 07:11

intelliot modified the milestones: 2.0, 2024 release Oct 17, 2023

intelliot closed this Oct 25, 2023

intelliot removed this from the 2.1.0 (Feb 2024) milestone Jan 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify source type for stored UNLs #3984

Specify source type for stored UNLs #3984

undertome commented Nov 17, 2021

undertome commented Nov 17, 2021

ximinez Nov 30, 2021

pwang200 Nov 30, 2021

undertome Dec 7, 2021

pwang200 Dec 17, 2021

undertome Dec 21, 2021

pwang200 Dec 21, 2021

ximinez Jan 4, 2022

undertome Jan 8, 2022

ximinez Jan 11, 2022

undertome Jan 12, 2022

pwang200 left a comment

intelliot commented Jun 1, 2022

RichardAH commented Jun 2, 2022

ximinez commented Jun 3, 2022

intelliot commented Jun 5, 2022

ximinez Jun 6, 2022

ximinez Jun 6, 2022

ximinez Jun 6, 2022

ximinez Jun 6, 2022

intelliot commented Nov 28, 2022

intelliot commented Apr 12, 2023

intelliot commented Oct 6, 2023

pwang200 commented Oct 16, 2023

intelliot commented Oct 25, 2023

Specify source type for stored UNLs #3984

Specify source type for stored UNLs #3984

Conversation

undertome commented Nov 17, 2021

High Level Overview of Change

Type of Change

Before / After

Test Plan

undertome commented Nov 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwang200 left a comment

Choose a reason for hiding this comment

intelliot commented Jun 1, 2022

RichardAH commented Jun 2, 2022

ximinez commented Jun 3, 2022

intelliot commented Jun 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intelliot commented Nov 28, 2022

intelliot commented Apr 12, 2023

intelliot commented Oct 6, 2023

pwang200 commented Oct 16, 2023

intelliot commented Oct 25, 2023