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: #3802

Closed
wants to merge 1 commit into from
Closed

Report additional fields in validation stream: #3802

wants to merge 1 commit into from

Conversation

nbougalis
Copy link
Contributor

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

  • 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

Tagging @wilsonianb's who originally opened #3797.

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.
@nbougalis nbougalis added API Change Documentation README changes, code comments, etc. labels Mar 29, 2021
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.

Code looks good. Can we add a unit test for this?

@nbougalis
Copy link
Contributor Author

Code looks good. Can we add a unit test for this?

I'll check.

@scottschurr
Copy link
Collaborator

This pull request has been sitting for a while.

The request was for unit tests, so I added to a preexisting test to see if I could un-stall the pull request: scottschurr@eeb1d41 Consider adding that unit test or something like it. You are welcome to cherry-pick that one if you'd like.

The code itself looks good to me. I'll thumbs up once the test is added.

@Darkac3-ux
Copy link

Darkac3-ux commented May 12, 2021 via email

@scottschurr
Copy link
Collaborator

@nbougalis, this pull request has been sitting untended for close to two months. It's missing unit tests, which I believe you can cherry-pick from here: scottschurr@eeb1d41

I know that a) you're really busy, b) you're working with only one hand, and c) this pull request is not your highest priority.

If you would like I can take over this pull request and shepherd it through the process so we can get this code into develop. Let me know if that would be helpful.

@scottschurr
Copy link
Collaborator

I'm closing this pull request so I can take over shepherding it through the review process. @nbougalis, who created this pull request, is too busy to tend it right now. I'll be opening a new pull request for this code momentarily.

@scottschurr scottschurr closed this Jun 9, 2021
Copy link

@SirJosh1987 SirJosh1987 left a comment

Choose a reason for hiding this comment

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

Hey guys thank you for the baby steps

@nbougalis nbougalis deleted the fix3797 branch October 16, 2023 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change Documentation README changes, code comments, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include new validation fields in subscription stream
6 participants