feat: Add validator-keys to xrpld project#7555
Open
legleux wants to merge 3 commits into
Open
Conversation
legleux
added
the
DraftRunCI
legleux
force-pushed
the
legleux/validator-keys-tool-conan-test
branch
from
0633707 to
bd8351c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7555 +/- ##
=========================================
- Coverage 82.0% 81.9% -0.0%
=========================================
Files 1007 1007
Lines 76854 76854
Branches 8984 8984
=========================================
- Hits 62990 62980 -10
- Misses 13855 13865 +10
Partials 9 9 see 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
legleux
force-pushed
the
legleux/validator-keys-tool-conan-test
branch
from
4c17216 to
894f690
Compare
legleux
removed
the
DraftRunCI
Contributor
There was a problem hiding this comment.
Pull request overview
This PR vendors the validator-keys utility into the xrpld repo, wires it up as an opt-in CMake target (-Dvalidator_keys=ON), includes it in Linux packaging, and upgrades the Conan test_package to build and run validator-keys --unittest as a real downstream consumer of xrpl::libxrpl.
Changes:
- Add in-tree
validator-keys-toolsource (utility + unit tests) and expose avalidator-keysexecutable target. - Update Conan
tests/conanto buildvalidator-keysagainst the exportedxrplpackage and run its unit tests. - Include
validator-keysin Debian/RPM packaging and ensure CI uploads/stages the additional binary.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
validator-keys-tool/src/ValidatorKeysTool.h |
Declares validator-keys tool entrypoints. |
validator-keys-tool/src/ValidatorKeysTool.cpp |
Implements CLI commands and main() for validator-keys. |
validator-keys-tool/src/ValidatorKeys.h |
Declares xrpl::ValidatorKeys and ValidatorToken. |
validator-keys-tool/src/ValidatorKeys.cpp |
Implements key file parsing/writing, token generation, revocation, signing, domain validation. |
validator-keys-tool/src/test/ValidatorKeysTool_test.cpp |
Adds unit tests for tool commands. |
validator-keys-tool/src/test/ValidatorKeys_test.cpp |
Adds unit tests for ValidatorKeys behavior and serialization. |
validator-keys-tool/src/test/KeyFileGuard.h |
Test helper for creating/removing a temporary key directory. |
validator-keys-tool/README.md |
Build instructions for the vendored tool and mention of Conan test usage. |
validator-keys-tool/doc/validator-keys-tool-guide.md |
Operator-facing guide for validator key/token workflows. |
validator-keys-tool/CMakeLists.txt |
Builds validator-keys target and registers CTest invocation. |
validator-keys-tool/cmake/KeysSanity.cmake |
Sanity checks and configuration setup for the tool build. |
validator-keys-tool/cmake/KeysInterface.cmake |
Defines an interface target for compile/link options consistent with xrpld settings. |
validator-keys-tool/cmake/KeysCov.cmake |
Adds optional coverage report target for validator-keys. |
validator-keys-tool/.git-blame-ignore-revs |
Adds a blame-ignore file for formatting-only revs. |
tests/conan/src/example.cpp |
Removes the previous synthetic “link-only” test consumer. |
tests/conan/conanfile.py |
Updates test_package recipe to build/run validator-keys unit tests. |
tests/conan/CMakeLists.txt |
Builds vendored validator-keys-tool as the Conan consumer. |
tests/conan/.gitignore |
Ignores Conan test_package build output directory. |
package/rpm/xrpld.spec |
Installs validator-keys into RPM and lists it in packaged files. |
package/README.md |
Documents that packages include validator-keys and how to build locally with -Dvalidator_keys=ON. |
package/debian/rules |
Installs validator-keys into Debian package staging. |
package/debian/control |
Notes validator-keys inclusion in Debian package description. |
package/build_pkg.sh |
Stages both xrpld and validator-keys binaries for packaging. |
CMakeLists.txt |
Adjusts include order to pull in XrplValidatorKeys earlier. |
cmake/XrplValidatorKeys.cmake |
Switches from FetchContent to vendored add_subdirectory + install rules. |
cmake/XrplPackaging.cmake |
Makes packaging target depend on both xrpld and validator-keys. |
.github/workflows/reusable-test-conan-package.yml |
Adds a reusable workflow to export and conan test the package. |
.github/workflows/reusable-package.yml |
Ensures both binaries are made executable before packaging. |
.github/workflows/reusable-build-test-config.yml |
Uploads/stages both binaries as artifacts; adjusts patchelf handling. |
.github/workflows/on-trigger.yml |
Runs Conan package test workflow and includes validator-keys-tool in watched paths. |
.github/workflows/on-tag.yml |
Runs Conan package test before uploading recipe on tags. |
.github/workflows/on-pr.yml |
Runs Conan package test workflow and includes validator-keys-tool in watched paths. |
.github/scripts/strategy-matrix/linux.json |
Enables -Dvalidator_keys=ON for Linux packaging builds. |
Comments suppressed due to low confidence (1)
tests/conan/conanfile.py:43
- The test runs
validator-keysfromself.cpp.build.bindir, but the CMakeLists setsvalidator-keysRUNTIME_OUTPUT_DIRECTORY to${CMAKE_BINARY_DIR}(build folder root). This mismatch can makeconan testfail to find the executable. Point the command at the actual output location (or align the CMake output dir with bindir).
def test(self):
if can_run(self):
cmd = os.path.join(self.cpp.build.bindir, "validator-keys")
self.run(f'"{cmd}" --unittest', env="conanrun")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+3
| #include <boost/optional.hpp> | ||
|
|
||
| #include <vector> |
Comment on lines
+1
to
+9
| #include <xrpl/protocol/KeyType.h> | ||
| #include <xrpl/protocol/SecretKey.h> | ||
|
|
||
| #include <boost/optional.hpp> | ||
|
|
||
| #include <cstdint> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
Comment on lines
+1
to
+14
| #include <ValidatorKeys.h> | ||
|
|
||
| #include <xrpl/basics/StringUtilities.h> | ||
| #include <xrpl/basics/base64.h> | ||
| #include <xrpl/json/json_reader.h> | ||
| #include <xrpl/json/to_string.h> | ||
| #include <xrpl/protocol/HashPrefix.h> | ||
| #include <xrpl/protocol/Sign.h> | ||
|
|
||
| #include <boost/algorithm/clamp.hpp> | ||
| #include <boost/filesystem.hpp> | ||
| #include <boost/regex.hpp> | ||
|
|
||
| #include <fstream> |
Comment on lines
+83
to
+86
| catch (std::exception const& e) | ||
| { | ||
| BEAST_EXPECT(e.what() == expectedError); | ||
| } |
Comment on lines
+170
to
+173
| catch (std::exception const& e) | ||
| { | ||
| BEAST_EXPECT(e.what() == expectedError); | ||
| } |
Comment on lines
+225
to
+228
| catch (std::exception const& e) | ||
| { | ||
| BEAST_EXPECT(e.what() == expectedError); | ||
| } |
Comment on lines
+34
to
+37
| catch (std::runtime_error& e) | ||
| { | ||
| BEAST_EXPECT(e.what() == expectedError); | ||
| } |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
One issue flagged inline — FORCE-writing CMAKE_CONFIGURATION_TYPES without a root-project guard can silently clobber the parent project's cache when this file is included via add_subdirectory().
Review by ReviewBot 🤖
Review by Claude Sonnet 4.6 · Prompt: V15
| endif() | ||
| endif() | ||
|
|
||
| set(CMAKE_CONFIGURATION_TYPES "Debug;Release" CACHE STRING "" FORCE) |
Contributor
There was a problem hiding this comment.
FORCE overwrites CMAKE_CONFIGURATION_TYPES globally — guard it so it only runs at the root:
if(is_root_project)
set(CMAKE_CONFIGURATION_TYPES "Debug;Release" CACHE STRING "" FORCE)
endif()
Note: is_root_project is computed later (line 41–46); move that block above this line, or inline the get_directory_property check here.
mathbunnyru
requested changes
Jun 19, 2026
mathbunnyru
left a comment
mathbunnyru
left a comment
Contributor
There was a problem hiding this comment.
I haven't read the code much, but I think after fixing existing issues, it will change too.
Comment on lines
+263
to
+266
| cp "${BUILD_DIR}/xrpld" "${BUILD_DIR}/artifacts/xrpld" | ||
| if [ -x "${BUILD_DIR}/validator-keys" ]; then | ||
| cp "${BUILD_DIR}/validator-keys" "${BUILD_DIR}/artifacts/validator-keys" | ||
| fi |
Contributor
There was a problem hiding this comment.
Suggested change
| cp "${BUILD_DIR}/xrpld" "${BUILD_DIR}/artifacts/xrpld" | |
| if [ -x "${BUILD_DIR}/validator-keys" ]; then | |
| cp "${BUILD_DIR}/validator-keys" "${BUILD_DIR}/artifacts/validator-keys" | |
| fi | |
| cp "${BUILD_DIR}/xrpld" "${BUILD_DIR}/artifacts/" | |
| if [ -x "${BUILD_DIR}/validator-keys" ]; then | |
| cp "${BUILD_DIR}/validator-keys" "${BUILD_DIR}/artifacts/" | |
| fi |
No need to duplicate the binary name, I think
| with: | ||
| name: xrpld-${{ inputs.config_name }} | ||
| path: ${{ env.BUILD_DIR }}/xrpld | ||
| path: ${{ env.BUILD_DIR }}/artifacts/* |
Contributor
There was a problem hiding this comment.
This changes the interface we provide, so outside scripts relying on our binaries might fail.
| --profile:all ci \ | ||
| --build=missing \ | ||
| --settings:all build_type=Release \ | ||
| --conf:all tools.build:jobs="$(nproc)" |
Contributor
There was a problem hiding this comment.
We don't use nproc directly
| @@ -1,26 +1,22 @@ | |||
| option( | |||
Contributor
There was a problem hiding this comment.
Maybe it would make sense to move this file content to validator-kets-tool/CMakeLists.txt?
| xrpld is the reference implementation of the XRP Ledger protocol. It | ||
| participates in the peer-to-peer XRP Ledger network, processes | ||
| transactions, and maintains the ledger database. | ||
| This package also includes the validator-keys tool for validator key |
Contributor
There was a problem hiding this comment.
I don't like that this definition is different from debian one, even though they describe a similar thing. Can we unify them?
I don't mind duplication in this case, but at least let's keep the same description
Contributor
There was a problem hiding this comment.
Maybe let's just add this file to README.md in the validator-keys-tool directory, instead of creating a directory for one file?
Comment on lines
+21
to
+43
| //------------------------------------------------------------------------------ | ||
| // The build version number. You must edit this for each release | ||
| // and follow the format described at http://semver.org/ | ||
| //-------------------------------------------------------------------------- | ||
| char const* const versionString = | ||
| "0.3.2" | ||
|
|
||
| #if defined(DEBUG) || defined(SANITIZER) | ||
| "+" | ||
| #ifdef DEBUG | ||
| "DEBUG" | ||
| #ifdef SANITIZER | ||
| "." | ||
| #endif | ||
| #endif | ||
|
|
||
| #ifdef SANITIZER | ||
| BOOST_PP_STRINGIZE(SANITIZER) | ||
| #endif | ||
| #endif | ||
|
|
||
| //-------------------------------------------------------------------------- | ||
| ; |
Contributor
There was a problem hiding this comment.
This duplicates the existing logic
Comment on lines
+17
to
+19
| #ifdef BOOST_MSVC | ||
| #include <Windows.h> | ||
| #endif |
Contributor
There was a problem hiding this comment.
I don't think you need it
| std::cout << "[validator_token]\n"; | ||
|
|
||
| auto const tokenStr = token->toString(); | ||
| auto const len = 72; |
Contributor
There was a problem hiding this comment.
Magic number, let's put it as a static constexpr variable with a better name
| << "\n\n"; | ||
| std::cout << "[validator_key_revocation]\n"; | ||
|
|
||
| auto const len = 72; |
Contributor
There was a problem hiding this comment.
Please, search in your PR, 72 is only in one place.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
Adds the
validator-keysutility to thexrpldrepository as an optional CMake target and includes it in Linux packages.This also changes the Conan package test from a synthetic example executable to a real downstream consumer: the test exports the
xrplpackage, buildsvalidator-keysagainstxrpl::libxrpl, and runs the tool’s unit tests.Context of Change
The previous Conan test package only verified that a tiny example target could link against
xrpl::libxrpl. That is useful, but it does not exercise the kind of real consumer we expect downstream users to build.validator-keysis a good fit for that role because it is:libxrplheaders, libraries, and build settings,The tool is vendored in this repository so the Conan test is hermetic and tests the source in this PR instead of fetching another repository at build time.
The target is opt-in through
-Dvalidator_keys=ON. Normal builds are unchanged unless that option is enabled. Debian/RHEL package build configs enable it so the producedxrpldpackages include/usr/bin/validator-keys.API Impact
libxrplchangeNo public API, peer protocol, or
libxrplAPI changes.This PR adds a real Conan package consumer test for
xrpl::libxrpl; it does not changelibxrplitself.Before / After
Before:
tests/conanexample consumer.xrpld.validator-keystarget.After:
xrplConan package and runsconan test.validator-keysagainstxrpl::libxrpl.validator-keys --unittest.xrpldandvalidator-keys.