New validator_info and manifest RPC methods #3197

movitto · 2019-12-30T02:34:18Z

This pull request addresses issue #3099 by adding two new RPC methods to allow the inspection of validator state.

validator_info will return information pertaining to the current validators keys, manifest sequence, and domain
manifest looks up manifest information for the specified key (either master or ephemeral)

Utility methods have been added to other modules to support this functionality

nbougalis

Nice. Thanks for your contribution. There's a few things that I'd like to see addressed before I sign off on this.

src/ripple/app/misc/Manifest.h

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

src/ripple/net/impl/RPCCall.cpp

src/ripple/rpc/handlers/ValidatorInfo.cpp

src/ripple/rpc/handlers/Manifest.cpp

src/ripple/rpc/handlers/ValidatorInfo.cpp

src/ripple/app/main/Application.h

nbougalis · 2019-12-31T02:58:53Z

src/ripple/rpc/handlers/ValidatorInfo.cpp

+    ret[jss::master_key]        = toBase58(TokenType::NodePublic, pk);
+
+    auto ek = context.app.validatorManifests().getSigningKey(pk);
+    ret[jss::ephemeral_key]     = toBase58(TokenType::NodePublic, ek);


You should detect and account for the use case where a validator is configured with a fixed key and not a manifest. If that happens, this field and those that follow are not relevant and should not be included.

I believe I addressed this by checking if the ephemeral key does exists, and if not using this to imply the manifest is not present. Please correct if this is the wrong assumption

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

movitto

@nbougalis many thanks for the review. I believe I addressed all feedback save an item above. Let me know it there is anything else!

Edit: meant to leave this as a comment... not a review of my own patches 🤦‍♂️

nbougalis

I believe you've addressed all my concerns, thanks! We need one more review for this to be considered Passed.

seelabs

This looks good. There are some formatting nits we should probably resolve. To resolve the formatting, you can try running clang format over Manifest.cpp and ValidatorIndo.cpp with the .clang_format rules checked in with rippled. (I'm also happy to do this and submit a patch if it's a pain for you).

src/ripple/net/impl/RPCCall.cpp

src/ripple/rpc/handlers/Manifest.cpp

src/ripple/rpc/handlers/ValidatorInfo.cpp

seelabs · 2020-01-06T22:21:28Z

I also noticed there are no unit tests for this. It would be good to have some simple tests for this as well.

seelabs · 2020-01-06T23:29:29Z

Non-unity build is broken. I think if both ValidatorInfo and Manifest have the following includes it should fix it:

#include <ripple/app/main/Application.h>
#include <ripple/basics/base64.h>
#include <ripple/json/json_value.h>
#include <ripple/protocol/ErrorCodes.h>
#include <ripple/protocol/jss.h>
#include <ripple/rpc/Context.h>

movitto · 2020-01-08T21:46:39Z

@seelabs thanks for the review. Resolved the style issues and added missing includes for the unity build.

As far as static_cast, making the change results in the following error:

/home/mmorsi/Workspace/rippled/src/ripple/rpc/handlers/Manifest.cpp:55:94: error: invalid static_cast from type ‘const char*’ to type ‘const uint8_t*’ {aka ‘const unsigned char*’}
   55 |         ret[jss::manifest] = base64_encode(static_cast<std::uint8_t const*>(manifest->c_str()),

Could you advice a fix or should this be left as is?

Will look into adding some tests in the near future.

codecov-io · 2020-01-08T22:35:59Z

Codecov Report

Merging #3197 into develop will decrease coverage by <.01%.
The diff coverage is 71.08%.

@@             Coverage Diff             @@
##           develop    #3197      +/-   ##
===========================================
- Coverage    70.33%   70.32%   -0.01%     
===========================================
  Files          675      677       +2     
  Lines        53178    53250      +72     
===========================================
+ Hits         37402    37450      +48     
- Misses       15776    15800      +24

Impacted Files	Coverage Δ
src/ripple/app/misc/Manifest.h	`55.55% <ø> (ø)`	⬆️
src/ripple/app/main/Application.h	`100% <ø> (ø)`	⬆️
src/ripple/rpc/impl/Handler.cpp	`96.49% <ø> (ø)`	⬆️
src/ripple/protocol/ErrorCodes.h	`100% <100%> (ø)`	⬆️
src/ripple/net/impl/RPCCall.cpp	`93.47% <52.63%> (-1.25%)`	⬇️
src/ripple/rpc/handlers/Manifest.cpp	`57.69% <57.69%> (ø)`
src/ripple/app/misc/impl/Manifest.cpp	`85.95% <83.33%> (-0.22%)`	⬇️
src/ripple/rpc/handlers/ValidatorInfo.cpp	`94.44% <94.44%> (ø)`

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 cc4cefa...2e5705d. Read the comment docs.

jwbusch · 2020-01-09T19:52:22Z

/home/mmorsi/Workspace/rippled/src/ripple/rpc/handlers/Manifest.cpp:55:94: error: invalid static_cast from type ‘const char*’ to type ‘const uint8_t*’ {aka ‘const unsigned char*’}
   55 |         ret[jss::manifest] = base64_encode(static_cast<std::uint8_t const*>(manifest->c_str()),

Could you advice a fix or should this be left as is?

Will look into adding some tests in the near future.

ret[jss::manifest] = base64_encode(*manifest);

Should work as @seelabs mentioned.

movitto · 2020-01-09T20:10:57Z

@seelabs @jwbusch cool thanks for the feedback. Called the simpler base64_encode method, removed the cast, and added some tests. Let me know if there is anything else!

jwbusch

Looked through the tests you added, LGTM.

jwbusch · 2020-01-09T21:46:25Z

src/ripple/rpc/handlers/ValidatorInfo.cpp

+
+    Json::Value ret;
+
+    auto pk = context.app.getValidationPublicKey();


nit: Would like to see more const used if possible. i.e. auto const pk = ...

Makes it clear that the variable won't be modified later (which is the case for a lot of these).

src/ripple/rpc/handlers/ValidatorInfo.cpp

movitto · 2020-01-10T03:12:10Z

@jwbusch pushed a patch constantizing the fields. As far as the scoped ephimeral_key, that method (getSigningKey, already existing before this PR) works a bit different than the new ones in the sense that if the key is not found, the input param (the master key) is simply returned. Since this is not a boost::optional we can't check it's 'truthyness' like the other fields.

Builds/CMake/RippledCore.cmake

seelabs · 2020-01-15T22:51:42Z

I think this is ready to go, we just need to fix the non-unity build issues. I think we just need to patch two files. I think this patch should fix things: seelabs@b47e374

movitto · 2020-01-21T18:52:37Z

I think this is ready to go, we just need to fix the non-unity build issues. I think we just need to patch two files. I think this patch should fix things: seelabs@b47e374

@seelabs cherry picked onto this branch, thanks!

seelabs

This looks good. Thanks @movitto. I'm approving this, tho we do need to rebase this onto 1.5.0-b3.

seelabs · 2020-01-21T22:10:16Z

@movitto FYI: Rebasing should be pretty straight-forward. One change to look out for is the parameters for doValidatorInfo and doManifest need to be changed from RPC::Context to RPC::JsonContext.

nbougalis · 2020-01-27T18:19:02Z

@movitto: I can handle the rebase and squashing down if you want, and would be happy to jump on a call with you to walk you through the commands and explain what I'm doing so you know for the next time.

movitto · 2020-01-27T23:57:11Z

@nbougalis sure that works. With the XRP meetup tomorrow won't have time to do the call until later on in the week, but perhaps we can schedule it for then (lets coordinate on that one via email)

Returns local validator details and specified manifest information respectively. Folded and rebased on latest develop

movitto · 2020-02-06T19:47:13Z

@seelabs @nbougalis rebased and pushed a followup patch w/ the 'Context' -> 'JsonContext' substitution. Using @mellery451's protobuf fix from #3244 (not included in this PR) I was able to build and test locally.

seelabs · 2020-02-07T17:54:50Z

@movitto Things looks good; I just added a "passed" label. Thanks for (yet another) contribution!

nbougalis suggested changes Dec 31, 2019

View reviewed changes

nbougalis reviewed Dec 31, 2019

View reviewed changes

src/ripple/app/misc/impl/Manifest.cpp Outdated Show resolved Hide resolved

movitto commented Jan 2, 2020

View reviewed changes

nbougalis approved these changes Jan 2, 2020

View reviewed changes

nbougalis requested review from jwbusch and seelabs January 2, 2020 06:17

seelabs reviewed Jan 6, 2020

View reviewed changes

jwbusch approved these changes Jan 9, 2020

View reviewed changes

seelabs reviewed Jan 10, 2020

View reviewed changes

Builds/CMake/RippledCore.cmake Outdated Show resolved Hide resolved

scottschurr assigned nbougalis, seelabs and jwbusch Jan 16, 2020

seelabs approved these changes Jan 21, 2020

View reviewed changes

seelabs added the Rebase label Jan 21, 2020

movitto mentioned this pull request Jan 22, 2020

develop build error: target pattern contains no '%' #3232

Open

movitto added 2 commits February 6, 2020 14:38

Add new validator_info and manifest rpc methods

d56eb24

Returns local validator details and specified manifest information respectively. Folded and rebased on latest develop

s/RPC::Context/RPC::JsonContext

2e5705d

seelabs added Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. and removed Rebase labels Feb 7, 2020

This was referenced Feb 12, 2020

Proposed 1.5.0-b5 #3257

Closed

Proposed 1.5.0-b5 #3259

Merged

manojsdoshi closed this in #3259 Feb 13, 2020

intelliot added API Change Documentation README changes, code comments, etc. labels Mar 11, 2020

This was referenced Mar 25, 2020

manifest command CLI format: publicMalformed error (Version: 1.5.0-rc1) #3317

Closed

CLI usage statement doesn't include manifest and validator_info (Version: 1.5.0-rc1) #3318

Closed

New methods: manifest, validator_info XRPLF/xrpl-dev-portal#806

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New validator_info and manifest RPC methods #3197

New validator_info and manifest RPC methods #3197

movitto commented Dec 30, 2019

nbougalis left a comment

nbougalis Dec 31, 2019

movitto Jan 2, 2020

movitto left a comment •

edited

Loading

nbougalis left a comment

seelabs left a comment

seelabs commented Jan 6, 2020

seelabs commented Jan 6, 2020 •

edited

Loading

movitto commented Jan 8, 2020 •

edited

Loading

codecov-io commented Jan 8, 2020 •

edited

Loading

jwbusch commented Jan 9, 2020 •

edited

Loading

movitto commented Jan 9, 2020

jwbusch left a comment

jwbusch Jan 9, 2020 •

edited

Loading

movitto commented Jan 10, 2020 •

edited

Loading

seelabs commented Jan 15, 2020

movitto commented Jan 21, 2020

seelabs left a comment

seelabs commented Jan 21, 2020

nbougalis commented Jan 27, 2020

movitto commented Jan 27, 2020

movitto commented Feb 6, 2020

seelabs commented Feb 7, 2020


		Json::Value ret;

		auto pk = context.app.getValidationPublicKey();

New validator_info and manifest RPC methods #3197

New validator_info and manifest RPC methods #3197

Conversation

movitto commented Dec 30, 2019

nbougalis left a comment

Choose a reason for hiding this comment

nbougalis Dec 31, 2019

Choose a reason for hiding this comment

movitto Jan 2, 2020

Choose a reason for hiding this comment

movitto left a comment • edited Loading

Choose a reason for hiding this comment

nbougalis left a comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

seelabs commented Jan 6, 2020

seelabs commented Jan 6, 2020 • edited Loading

movitto commented Jan 8, 2020 • edited Loading

codecov-io commented Jan 8, 2020 • edited Loading

Codecov Report

jwbusch commented Jan 9, 2020 • edited Loading

movitto commented Jan 9, 2020

jwbusch left a comment

Choose a reason for hiding this comment

jwbusch Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

movitto commented Jan 10, 2020 • edited Loading

seelabs commented Jan 15, 2020

movitto commented Jan 21, 2020

seelabs left a comment

Choose a reason for hiding this comment

seelabs commented Jan 21, 2020

nbougalis commented Jan 27, 2020

movitto commented Jan 27, 2020

movitto commented Feb 6, 2020

seelabs commented Feb 7, 2020

movitto left a comment •

edited

Loading

seelabs commented Jan 6, 2020 •

edited

Loading

movitto commented Jan 8, 2020 •

edited

Loading

codecov-io commented Jan 8, 2020 •

edited

Loading

jwbusch commented Jan 9, 2020 •

edited

Loading

jwbusch Jan 9, 2020 •

edited

Loading

movitto commented Jan 10, 2020 •

edited

Loading