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.

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

Report additional fields in validation stream #3865

Closed
wants to merge 1 commit into from

Conversation

scottschurr
Copy link
Collaborator

This branch was initially created by @nbougalis, who is currently too busy to shepherd the pull request through the review. So I'm re-opening the pull request in a form where I can easily handle any requests for changes.

Overview

The HardenedValidations amendment introduces additional fields in validations:

  • sfValidatedHash, if present, is the hash the of last ledger that the validator considers to be fully validated.
  • sfCookie, if present, is a 64-bit cookie (the default implementation selects it randomly at startup but other implementations are possible), which can be used to improve the detection and classification of duplicate validations.
  • sfServerVersion, if present, reports the version of the software that the validator is running. By surfacing this information, server operators gain additional insight about variety of software on the network.

If merged, this commit fixes #3797 by adding the fields to the validations stream as shown below:

  • sfValidateHash as validated_hash: a 256-bit hex string;
  • sfCookie as cookie: a 64-bit integer as a string; and
  • sfServerVersion as server_version: a 64-bit integer as a string.

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation updates are required

Tagging @wilsonianb who originally opened #3797.

Copy link
Contributor

@cjcobb23 cjcobb23 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for adding the tests.

Comment on lines 2012 to 2020
if (auto version = (*val)[~sfServerVersion]; version)
jvObj[jss::server_version] = std::to_string(*version);

if (auto cookie = (*val)[~sfCookie]; cookie)
jvObj[jss::cookie] = std::to_string(*cookie);

if (auto hash = (*val)[~sfValidatedHash]; hash)
jvObj[jss::validated_hash] = strHex(*hash);

Copy link
Collaborator

Choose a reason for hiding this comment

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

The second check in all of these are redundant. In other words,

if(auto x = X(); x)
  foo;

can be written as:

if(auto x = X())
  foo;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @ximinez, I believe I've addressed your comment with the most recent commit.

Choose a reason for hiding this comment

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

So that was a human that called me today my bad bud

Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

Looks good

@scottschurr
Copy link
Collaborator Author

@miguelportilla, would you like to weigh in on this pull request? Or are you happy enough with reviews from @cjcobb23 and @ximinez? Whatever you'd like. Thanks.

@scottschurr
Copy link
Collaborator Author

Private communications suggest that @miguelportilla is currently tied up with other matters. Given that we have approval from two reviewers, I'm going to squash this branch and, assuming it passes CI after the squash, mark it as passed.

The HardenedValidations amendment introduces additional fields
in validations:

- `sfValidatedHash`, if present, is the hash the of last ledger that
  the validator considers to be fully validated.
- `sfCookie`, if present, is a 64-bit cookie (the default
  implementation selects it randomly at startup but other
  implementations are possible), which can be used to improve the
  detection and classification of duplicate validations.
- `sfServerVersion`, if present, reports the version of the software
  that the validator is running. By surfacing this information,
  server operators gain additional insight about variety of software
  on the network.

If merged, this commit fixes XRPLF#3797 by adding the fields to the
`validations` stream as shown below:

- `sfValidateHash` as `validated_hash`: a 256-bit hex string;
- `sfCookie` as `cookie`: a 64-bit integer as a string; and
- `sfServerVersion` as `server_version`: a 64-bit integer as
  a string.
@scottschurr scottschurr added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jul 23, 2021
@manojsdoshi manojsdoshi mentioned this pull request Jul 27, 2021
@scottschurr scottschurr deleted the nik-fix3797 branch August 17, 2021 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include new validation fields in subscription stream
7 participants