feat: add user version of feature RPC

[mvadari]

High Level Overview of Change

This PR modifies the feature RPC to have a non-admin mode that allows conversion from the hex encodings of amendments to human-readable strings.

Example:

> {
    "command": "feature",
    "feature": "2E2FB9CF8A44EB80F4694D38AADAE9B8B7ADAFD2F092E10068E61C98C4F092B0"
  }
< {
    "result": {
      "2E2FB9CF8A44EB80F4694D38AADAE9B8B7ADAFD2F092E10068E61C98C4F092B0": {
        "enabled": false,
        "name": "fixUniversalNumber",
        "supported": true
      }
    },
    "type": "response"
  }

Context of Change

This has been requested by many explorer operators, so that the on-ledger hex amendment names can be decoded.

Type of Change

• New feature (non-breaking change which adds functionality)

API Impact

• Public API: New feature (new methods and/or new fields)

Test Plan

Added tests to handle this case.

[JST5000]

As an additional note, this is a feature I tried to find when I had when trying to set up our docker container for xrpl.js and xrpl-py CI since the Amendments stanza in the rippled.cfg file to force-enable amendments is really hard to read without the human-readable versions of amendments.

[dangell7]

This seems overkill.

import hashlib

def hash_string(k: str) -> str:
    return hashlib.sha512(k.encode('utf-8')).hexdigest().upper()[:64]

import * as crypto from 'crypto';

function sha512Hash(k: string): string {
  return crypto.createHash('sha512').update(k, 'utf8').digest('hex').toUpperCase().substring(0, 64);
}

[mvadari]

This seems overkill.

This works for going from the name to the hex, but not the other way around, since you can't reverse a hash. That's where this is needed, because that's what the ledger stores.

[pkcs8]

Would the non-admin version of this command have similar response shape as the admin version? I.e., if I skip the feature property, and just send {"command":"feature"}, it should return a map of all amendments known to this rippled sans voting data.

[mvadari]

Would the non-admin version of this command have similar response shape as the admin version? I.e., if I skip the feature property, and just send {"command":"feature"}, it should return a map of all amendments known to this rippled sans voting data.

I didn't add that here because it's a bit harder to do. But if that's a need, I can add it.

[pkcs8]

I read rippled websocket url from the environment, and often switch and use admin and non-admin ports interchangeably. I'm not sure now complicated it would be, but would be great if response shape remains same so I don't have to write conditions. I like how account_objects command works with its type filter.

[shortthefomo]

I didn't add that here because it's a bit

I'm actually a +1 for this I need a way to figure the list of amendments on the network. Currently using external API for mainnet or hardcoded. Would help a lot.

[mvadari]

@pkcs8 @lathanbritz I have switched to the existing admin feature format (removing the admin fields of course) and added the ability to get a list of all features in 43a1ce7.

[mvadari]

@intelliot can I add @ximinez and @thejohnfreeman as reviewers (if they're okay with that)?

[intelliot]

@mvadari I requested ximinez and thejohnfreeman but the PRs listed here are higher priority: https://github.com/XRPLF/rippled/milestone/8 <- these are 2.0 release blockers

This PR (#4781) is currently planned for 2.1.0

[dangell7]

We don't like this on feature endpoint. We believe opening up feature like this is dangerous. We have opted to move this to the ServerDefinitions.cpp file.

What are your thoughts on this?

I know this goes against what XRPScan asked for but in our opinion its a "safer" solution.

[mvadari]

We don't like this on feature endpoint. We believe opening up feature like this is dangerous. We have opted to move this to the ServerDefinitions.cpp file.

What are your thoughts on this?

I know this goes against what XRPScan asked for but in our opinion its a "safer" solution.

This code change is pretty small and therefore easy to audit and test from a security perspective. There is also prior art for functions that have separate user versions and admin versions, like server_info, and there have been no issues with those. I'm personally not concerned, but there's also a reason we go through code review 🙂

[ximinez]

Looks good and useful

[Bronek]

I recommend adding something as simple as this (note location, right after doRound at line 628):

--- a/src/test/app/AmendmentTable_test.cpp
+++ b/src/test/app/AmendmentTable_test.cpp
@@ -638,6 +638,20 @@ public:
         BEAST_EXPECT(enabled.empty());
         BEAST_EXPECT(majority.empty());

+        uint256 const unsupportedID = amendmentId(unsupported_[0]);
+        {
+            Json::Value const unsupp =
+                table->getJson(unsupportedID, false)[to_string(unsupportedID)];
+            BEAST_EXPECT(unsupp.size() == 0);
+        }
+
+        table->veto(unsupportedID);
+        {
+            Json::Value const unsupp =
+                table->getJson(unsupportedID, false)[to_string(unsupportedID)];
+            BEAST_EXPECT(!unsupp[jss::vetoed].asBool());
+        }
+
         votes.emplace_back(testAmendment, validators.size());

         doRound(

This will cover the one missing test case for isAdmin=false after we have voted; the other missing branches are around fs.enabled and are unrelated to this PR, and so should be ignored here.

[Bronek]

You are missing case case for lastVote_=true and isAdmin=false

[Bronek]

(cc @Bronek to double check since he's already taken a look at some of the code)

Thank you @intelliot ; I looked some more and there is one thing that can be improved in unit tests; added it in my feedback.

I use cmake -Dcoverage-format=json-details -Dcoverage=ON . . . to prepare coverage.json report and see from it that the proposed addition will increase branch coverage of line 929 in AmendmentTable.cpp from 6/8 to 7/8 .

[ximinez]

@mvadari Is this ready to merge?

[mvadari]

@mvadari Is this ready to merge?

@ximinez with Bronek's ✅ , yep - I think it was just waiting on the 2.1 release.

[ximinez]

[see following comment]

[mvadari]

Proposed commit message:

feat: add user version of `feature` RPC (#4781)

* hides potentially sensitive data
* uses same formatting as admin RPC

@ximinez nit: I'd switch the order of the bullets

feat: add user version of `feature` RPC (#4781):

* uses same formatting as admin RPC
* hides potentially sensitive data

[intelliot]

From my perspective, @ximinez or @seelabs are welcome to merge this. (no big rush though)