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

enrich denied reports with denylist tagname #604

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

andymck
Copy link
Contributor

@andymck andymck commented Aug 17, 2023

Linked proto PR: helium/proto#366

The proto adds an invalid_details field to invalid witness and beacon reports and the verified witness report

Enables poc verifications to return additional context on why a report was declared invalid and for those details to be carried through to the protos hitting S3.

Currently the only poc verification check which is populating this field is the denylist check. If a report is found to be on the denylist invalid_details will be set to the tagname of the list in use

A tag name looks like 2023072901

@andymck andymck marked this pull request as ready for review August 17, 2023 13:53
Copy link
Contributor

@jeffgrunewald jeffgrunewald left a comment

Choose a reason for hiding this comment

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

not sure how we'd enforce this, as a trait maybe with a single function, but perhaps we should ensure the invalid reason string is no more than a certain number of characters and the details get truncated if they go too long?

Comment on lines 471 to 479
tracing::debug!(
"report verification failed, reason: {:?}.
pubkey: {}",
pubkey: {}, tagname: {}",
InvalidReason::Denied,
pub_key,
deny_list.tag_name
);
Copy link
Contributor

Choose a reason for hiding this comment

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

tracing::debug!(pub_key, tagname = deny_list.tag_name, reason = ?InvalidReason::Denied, "report verification failed");

@andymck andymck force-pushed the andymck/include-denylist-version branch from 8e3a728 to 54e9188 Compare August 17, 2023 16:21
@andymck andymck merged commit ef3bf27 into main Aug 17, 2023
1 check passed
@andymck andymck deleted the andymck/include-denylist-version branch August 17, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants